From ba322d7696c1d24e1421454b6834961691d83ba4 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 22:14:06 +0000 Subject: [PATCH 1/4] =?UTF-8?q?feat(core):=20Phase=201=20seam=20scaffoldin?= =?UTF-8?q?g=20=E2=80=94=20add=5Ffrom=5Forigin/reindex=20contracts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inject points on FastSkillService (embedding_service, repository_manager + builders/accessors) and stub the two seams: - core/install.rs: AddMode/AddOutcome/UpdatePreflight/Fetched + add_from_origin = commit(fetch(origin), mode) + preflight (bodies TODO). - core/reindex.rs: ReindexProgress/ReindexOutcome + reindex(skills_dir,observer) (skips silently without a provider; real body TODO). Compiles green; seams not yet callable (return not-implemented). Bodies land next. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/fastskill-core/src/core/install.rs | 112 ++++++++++++++++++++++ crates/fastskill-core/src/core/mod.rs | 8 ++ crates/fastskill-core/src/core/reindex.rs | 60 ++++++++++++ crates/fastskill-core/src/core/service.rs | 41 ++++++++ 4 files changed, 221 insertions(+) create mode 100644 crates/fastskill-core/src/core/install.rs create mode 100644 crates/fastskill-core/src/core/reindex.rs diff --git a/crates/fastskill-core/src/core/install.rs b/crates/fastskill-core/src/core/install.rs new file mode 100644 index 0000000..06e1c09 --- /dev/null +++ b/crates/fastskill-core/src/core/install.rs @@ -0,0 +1,112 @@ +//! The core install seam (ADR-0005): `add_from_origin(origin, mode)`. +//! +//! `add_from_origin = commit(fetch(origin), origin, mode)`. `fetch` is the only +//! per-variant part (match-dispatched, produces a temp dir + the [`Resolved`] +//! facts); `commit` is the shared pipeline (validate → atomic-move into the +//! skills dir → upsert Manifest → write Lock → reindex-if-provider). `mode` only +//! governs the id-conflict policy. `add`/`update` are one operation. + +use crate::core::origin::{Origin, Resolved}; +use crate::core::service::{FastSkillService, ServiceError}; +use std::path::PathBuf; +use tempfile::TempDir; + +/// Whether an install is a fresh add (409 on an existing id) or an update +/// (overwrite the recorded skill). See ADR-0005. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AddMode { + /// Adding a new skill: fail if the resolved id is already installed. + Fresh, + /// Re-installing an already-recorded skill from its origin: overwrite. + Update, +} + +/// Result of a successful `add_from_origin`. +#[derive(Debug, Clone)] +pub struct AddOutcome { + pub id: String, + pub origin: Origin, + pub resolved: Resolved, + /// Whether the auto-reindex ran (false = skipped, e.g. no embedding provider). + pub reindexed: bool, +} + +/// The outcome of the update preflight (ADR-0005 §Q6). Only `Updatable` proceeds +/// to a re-fetch; the others are honest no-ops with a reason. +#[derive(Debug, Clone)] +pub enum UpdatePreflight { + UpToDate, + Immutable { reason: String }, + Updatable, +} + +/// A fetched skill sitting in a temp dir, plus the facts the fetch resolved. +/// The `TempDir` guards the extracted contents until `commit` moves them. +pub struct Fetched { + pub temp_dir: TempDir, + pub skill_path: PathBuf, + pub resolved: Resolved, +} + +impl FastSkillService { + /// Install a single skill from an [`Origin`] (ADR-0005). `Fresh` adds a new + /// skill (409-style error if the id already exists); `Update` re-resolves the + /// origin and overwrites. Equivalent to `commit(fetch(origin), origin, mode)`. + pub async fn add_from_origin( + &self, + origin: Origin, + mode: AddMode, + ) -> Result { + let fetched = self.fetch(&origin).await?; + self.commit(fetched, origin, mode).await + } + + /// Fetch a skill described by `origin` into a temp dir, capturing the resolved + /// facts. The only per-variant step (git clone / local copy-or-unzip / remote + /// zip download / registry download). + async fn fetch(&self, origin: &Origin) -> Result { + let _ = origin; + // TODO(phase1-agent-B): match on the four Origin variants and fetch into a + // temp dir using the core primitives (storage::git::clone_repository, + // storage::zip::ZipHandler::extract_to_dir, the registry clients via + // `self.repository_manager()`), returning `Fetched{temp_dir, skill_path, + // resolved}`. Move the orchestration from the CLI add_from_*/install_from_*. + Err(ServiceError::InvalidOperation( + "add_from_origin: fetch not yet implemented".to_string(), + )) + } + + /// The shared post-fetch pipeline: validate → atomic-move into the skills dir → + /// upsert Manifest → write Lock (origin + resolved) → reindex-if-provider. + /// Ordering is skills-dir → manifest → lock → reindex (never reference a skill + /// before it exists); each store write is atomic; recovery is idempotent + /// re-run + reconcile (ADR-0005 §Q3). + async fn commit( + &self, + fetched: Fetched, + origin: Origin, + mode: AddMode, + ) -> Result { + let _ = (fetched, origin, mode); + // TODO(phase1-agent-B): validate the fetched skill, derive its id, enforce + // the mode's id-conflict policy (Fresh → error on existing), atomic-rename + // into the skills dir, upsert the manifest entry, write the lock entry + // (origin + resolved), then `self.reindex(None, None)` best-effort and set + // `reindexed`. Return the AddOutcome. + Err(ServiceError::InvalidOperation( + "add_from_origin: commit not yet implemented".to_string(), + )) + } + + /// Update preflight (ADR-0005 §Q6): decide whether the recorded origin has + /// anything to update before doing any fetch. `repository` → newest allowed + /// via UpdateService; immutable git tag/commit + editable local → `Immutable`; + /// git branch / local copy / zip-url → `Updatable` (re-fetch; commit is idempotent). + pub async fn preflight(&self, origin: &Origin) -> Result { + let _ = origin; + // TODO(phase1-agent-B): implement the per-variant preflight table. + Err(ServiceError::InvalidOperation( + "update preflight not yet implemented".to_string(), + )) + } +} diff --git a/crates/fastskill-core/src/core/mod.rs b/crates/fastskill-core/src/core/mod.rs index 170e1be..edd6e5b 100644 --- a/crates/fastskill-core/src/core/mod.rs +++ b/crates/fastskill-core/src/core/mod.rs @@ -8,6 +8,7 @@ pub mod dependencies; pub mod dependency_resolver; pub mod embedding; pub mod frontmatter; +pub mod install; pub mod lock; pub mod manifest; pub mod metadata; @@ -17,6 +18,7 @@ pub mod project_config; pub mod reconciliation; pub mod registry; pub mod registry_index; +pub mod reindex; pub mod repository; pub mod resolver; pub mod routing; @@ -65,6 +67,12 @@ pub use metadata::{ // origin pub use origin::{GitRef, Origin, Resolved}; +// install seam +pub use install::{AddMode, AddOutcome, Fetched, UpdatePreflight}; + +// reindex seam +pub use reindex::{ReindexOutcome, ReindexProgress}; + // project_config pub use project_config::{load_project_config, ProjectConfig}; diff --git a/crates/fastskill-core/src/core/reindex.rs b/crates/fastskill-core/src/core/reindex.rs new file mode 100644 index 0000000..c57cff2 --- /dev/null +++ b/crates/fastskill-core/src/core/reindex.rs @@ -0,0 +1,60 @@ +//! Core reindex seam (ADR-0002/0005). +//! +//! Reindex is domain logic (skills dir → Vector index) and lives in core. It runs +//! only when an [`EmbeddingService`](crate::core::embedding::EmbeddingService) has +//! been injected (edge-constructed with the API key); otherwise it **skips +//! silently**. The CLI/serve edge injects the provider via +//! [`FastSkillService::with_embedding_service`]. + +use crate::core::service::{FastSkillService, ServiceError}; +use std::path::Path; + +/// Progress datum emitted per skill as reindex proceeds. The core emits neutral +/// data; the caller (CLI) decides how to render it (HTTP passes no observer). +#[derive(Debug, Clone)] +pub struct ReindexProgress { + pub current: usize, + pub total: usize, + pub skill_id: String, +} + +/// Outcome of a reindex call. `reindexed=false` + a `reason` means it was skipped +/// (no provider), which is a success, not a failure (ADR-0002). +#[derive(Debug, Clone)] +pub struct ReindexOutcome { + pub reindexed: bool, + pub count: usize, + pub reason: Option, +} + +impl ReindexOutcome { + pub(crate) fn skipped(reason: &str) -> Self { + ReindexOutcome { + reindexed: false, + count: 0, + reason: Some(reason.to_string()), + } + } +} + +impl FastSkillService { + /// Reindex the vector index for `skills_dir` (defaults to the configured skill + /// storage path). Skips silently with an outcome reason when no embedding + /// provider is injected. `observer`, if provided, is called once per skill. + pub async fn reindex( + &self, + skills_dir: Option<&Path>, + observer: Option<&(dyn Fn(ReindexProgress) + Send + Sync)>, + ) -> Result { + let _ = (skills_dir, observer); + // No provider ⇒ skip silently (the common, non-configured case). + if self.embedding_service().is_none() { + return Ok(ReindexOutcome::skipped("no embedding provider configured")); + } + // TODO(phase1-agent-A): move the real reindex logic here from the CLI + // `execute_reindex` (find SKILL.md files, embed each via the injected + // provider, write to the VectorIndexService), emitting `observer` + // progress. Return `{reindexed:true, count, reason:None}`. + Ok(ReindexOutcome::skipped("reindex not yet implemented")) + } +} diff --git a/crates/fastskill-core/src/core/service.rs b/crates/fastskill-core/src/core/service.rs index 947002b..2c09332 100644 --- a/crates/fastskill-core/src/core/service.rs +++ b/crates/fastskill-core/src/core/service.rs @@ -289,6 +289,15 @@ pub struct FastSkillService { /// Vector index service (optional, for embedding search) vector_index_service: Option>, + /// Embedding provider (optional), injected at the CLI/serve edge where the + /// API key is loaded. `None` ⇒ reindex skips silently (ADR-0002/0005). + embedding_service: Option>, + + /// Repository access (optional), injected at the edge from the resolved + /// `repos` config. Needed to fetch `Origin::Repository` skills; `None` ⇒ + /// a repository-origin install returns a clear "no repositories configured" error. + repository_manager: Option>, + /// Skill storage backend storage: Arc, @@ -355,12 +364,44 @@ impl FastSkillService { skill_manager, metadata_service, vector_index_service, + embedding_service: None, + repository_manager: None, storage, hot_reload_manager, initialized: false, }) } + /// Inject an embedding provider (edge-constructed, holds the API key). Enables + /// the core reindex seam; without it reindex skips silently. + pub fn with_embedding_service( + mut self, + embedding: Arc, + ) -> Self { + self.embedding_service = Some(embedding); + self + } + + /// Inject repository access resolved from the edge `repos` config. Enables + /// fetching `Origin::Repository` skills. + pub fn with_repository_manager( + mut self, + manager: Arc, + ) -> Self { + self.repository_manager = Some(manager); + self + } + + /// The injected embedding provider, if any. + pub fn embedding_service(&self) -> Option<&Arc> { + self.embedding_service.as_ref() + } + + /// The injected repository manager, if any. + pub fn repository_manager(&self) -> Option<&Arc> { + self.repository_manager.as_ref() + } + /// Initialize the service pub async fn initialize(&mut self) -> Result<(), ServiceError> { if self.initialized { From 286b3de24e835c68232cd162ea53059d499d8d8d Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 22:58:55 +0000 Subject: [PATCH 2/4] feat(core): implement add_from_origin + reindex seams (Phase 1 core) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fill the Phase-1 core seams (built in parallel worktrees, merged): - reindex.rs: real reindex driven by the injected embedding provider (skip silently without one), hash-skip unchanged skills, prune stale index entries, emit per-skill progress. Moved from the CLI execute_reindex. - install.rs: fetch (4-variant dispatch → temp dir + Resolved) + commit (validate → atomic-move/symlink → manifest → lock → best-effort reindex) + preflight. add_from_origin = commit(fetch(origin), mode). - change_detection.rs: fix a test-isolation bug both agents hit — git subprocess calls inherited GIT_DIR/GIT_WORK_TREE from the pre-commit hook and a private CWD lock raced the shared DIR_MUTEX; harden with env_remove + shared mutex. Core green: build + clippy -D warnings + 478 tests. Not yet wired into the CLI. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/core/change_detection.rs | 48 +- crates/fastskill-core/src/core/install.rs | 1207 ++++++++++++++++- crates/fastskill-core/src/core/reindex.rs | 439 +++++- 3 files changed, 1648 insertions(+), 46 deletions(-) diff --git a/crates/fastskill-core/src/core/change_detection.rs b/crates/fastskill-core/src/core/change_detection.rs index 155eeec..196a3b0 100644 --- a/crates/fastskill-core/src/core/change_detection.rs +++ b/crates/fastskill-core/src/core/change_detection.rs @@ -19,9 +19,21 @@ pub fn detect_changed_skills_git( use std::process::Command; - // Use git diff to get changed files + // Use git diff to get changed files. This runs against the ambient cwd (the + // caller is expected to be inside the target repo), so any + // GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE inherited from an *enclosing* git + // invocation (e.g. this crate's own pre-commit hook, which shells out to + // `cargo test`) must be cleared — otherwise `git diff` silently resolves + // against that other repository instead of the caller's cwd. let output = Command::new("git") .args(["diff", "--name-only", base_ref, head_ref]) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_INDEX_FILE") + .env_remove("GIT_OBJECT_DIRECTORY") + .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES") + .env_remove("GIT_COMMON_DIR") + .env_remove("GIT_CEILING_DIRECTORIES") .output() .map_err(|e| ServiceError::Custom(format!("Failed to execute git diff: {}", e)))?; @@ -172,14 +184,10 @@ pub fn calculate_skill_hash(skill_path: &Path) -> Result { mod tests { use super::*; use crate::core::build_cache::BuildCache; + use crate::test_utils::DIR_MUTEX as CWD_LOCK; use std::fs; - use std::sync::Mutex; use tempfile::TempDir; - /// Serializes tests that mutate the process-wide current directory (the git - /// helper runs `git` in the ambient cwd and has no directory parameter). - static CWD_LOCK: Mutex<()> = Mutex::new(()); - // --- BUG-7: git-diff path parsing on whole components --- #[test] @@ -352,10 +360,31 @@ mod tests { // --- detect_changed_skills_git (real temporary git repo) --- + /// A `git` `Command` rooted at `repo`, immune to an *enclosing* git + /// invocation's environment. A caller that itself runs under a git hook + /// (e.g. this crate's own pre-commit hook, which shells out to + /// `cargo test`) may have GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE set so its + /// own git invocations resolve without a `-C`/cwd. Those vars are + /// inherited by child processes and take precedence over `current_dir`, + /// so without clearing them every git subprocess these tests spawn would + /// actually operate on *this* crate's own repository instead of `repo` — + /// corrupting its index/refs. Clear the whole family defensively. + fn git_cmd_in(repo: &Path) -> std::process::Command { + let mut cmd = std::process::Command::new("git"); + cmd.current_dir(repo) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_INDEX_FILE") + .env_remove("GIT_OBJECT_DIRECTORY") + .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES") + .env_remove("GIT_COMMON_DIR") + .env_remove("GIT_CEILING_DIRECTORIES"); + cmd + } + fn run_git(repo: &Path, args: &[&str]) { - let status = std::process::Command::new("git") + let status = git_cmd_in(repo) .args(args) - .current_dir(repo) .env("GIT_AUTHOR_NAME", "t") .env("GIT_AUTHOR_EMAIL", "t@example.com") .env("GIT_COMMITTER_NAME", "t") @@ -381,9 +410,8 @@ mod tests { run_git(repo, &["add", "-A"]); run_git(repo, &["commit", "-q", "-m", "init"]); let base = String::from_utf8( - std::process::Command::new("git") + git_cmd_in(repo) .args(["rev-parse", "HEAD"]) - .current_dir(repo) .output() .unwrap() .stdout, diff --git a/crates/fastskill-core/src/core/install.rs b/crates/fastskill-core/src/core/install.rs index 06e1c09..1306140 100644 --- a/crates/fastskill-core/src/core/install.rs +++ b/crates/fastskill-core/src/core/install.rs @@ -6,9 +6,19 @@ //! skills dir → upsert Manifest → write Lock → reindex-if-provider). `mode` only //! governs the id-conflict policy. `add`/`update` are one operation. -use crate::core::origin::{Origin, Resolved}; -use crate::core::service::{FastSkillService, ServiceError}; -use std::path::PathBuf; +use crate::core::lock::{project_lock_path, ProjectSkillsLock}; +use crate::core::manifest::{ + DependenciesSection, DependencySpec, ProjectContext, SkillProjectToml, +}; +use crate::core::metadata::{parse_yaml_frontmatter, SkillFrontmatter}; +use crate::core::origin::{GitRef, Origin, Resolved}; +use crate::core::project::{detect_context_from_content, resolve_project_file}; +use crate::core::repository::RepositoryManager; +use crate::core::service::{FastSkillService, ServiceError, SkillId}; +use crate::core::skill_manager::SkillDefinition; +use crate::core::version::{is_newer, newest_version, VersionConstraint}; +use std::collections::HashMap; +use std::path::{Path, PathBuf}; use tempfile::TempDir; /// Whether an install is a fresh add (409 on an existing id) or an update @@ -65,15 +75,229 @@ impl FastSkillService { /// facts. The only per-variant step (git clone / local copy-or-unzip / remote /// zip download / registry download). async fn fetch(&self, origin: &Origin) -> Result { - let _ = origin; - // TODO(phase1-agent-B): match on the four Origin variants and fetch into a - // temp dir using the core primitives (storage::git::clone_repository, - // storage::zip::ZipHandler::extract_to_dir, the registry clients via - // `self.repository_manager()`), returning `Fetched{temp_dir, skill_path, - // resolved}`. Move the orchestration from the CLI add_from_*/install_from_*. - Err(ServiceError::InvalidOperation( - "add_from_origin: fetch not yet implemented".to_string(), - )) + match origin { + Origin::Git { url, r#ref, subdir } => { + self.fetch_git(url, r#ref, subdir.as_deref()).await + } + Origin::Local { path, editable } => self.fetch_local(path, *editable).await, + Origin::ZipUrl { url } => self.fetch_zip_url(url).await, + Origin::Repository { + repo, + skill, + version, + } => self.fetch_repository(repo, skill, version.as_ref()).await, + } + } + + async fn fetch_git( + &self, + url: &str, + git_ref: &GitRef, + subdir: Option<&Path>, + ) -> Result { + let (branch, tag) = match git_ref { + GitRef::Default => (None, None), + GitRef::Branch(b) => (Some(b.as_str()), None), + GitRef::Tag(t) => (None, Some(t.as_str())), + GitRef::Commit(_) => { + // No clone-by-commit primitive exists yet (`clone_repository` only + // takes branch/tag). Surface a clear error rather than mishandling it. + return Err(ServiceError::InvalidOperation( + "Installing a skill pinned to a git commit is not yet supported (no \ + clone-by-commit primitive)" + .to_string(), + )); + } + }; + + let temp_dir = crate::storage::git::clone_repository(url, branch, tag, None).await?; + + let skill_base = if let Some(subdir) = subdir { + let joined = safe_subdir_join(temp_dir.path(), subdir)?; + if !joined.exists() { + return Err(ServiceError::InvalidOperation(format!( + "Specified subdirectory '{}' does not exist in cloned repository", + subdir.display() + ))); + } + joined + } else { + temp_dir.path().to_path_buf() + }; + let skill_path = crate::storage::git::validate_cloned_skill(&skill_base)?; + + let commit_hash = git_head_commit(temp_dir.path()).await?; + let frontmatter = read_skill_frontmatter(&skill_path).await?; + let (_, version) = derive_skill_id_and_version(&skill_path, &frontmatter)?; + + Ok(Fetched { + temp_dir, + skill_path, + resolved: Resolved { + version, + commit_hash: Some(commit_hash), + checksum: None, + }, + }) + } + + async fn fetch_local(&self, path: &Path, editable: bool) -> Result { + let resolved_path = if path.is_absolute() { + path.to_path_buf() + } else { + std::env::current_dir()?.join(path) + }; + if !resolved_path.exists() { + return Err(ServiceError::InvalidOperation(format!( + "Local path does not exist: {}", + resolved_path.display() + ))); + } + + let temp_dir = TempDir::new()?; + let is_zip = resolved_path.is_file() + && resolved_path.extension().and_then(|e| e.to_str()) == Some("zip"); + + let skill_path = if is_zip { + let extract_path = temp_dir.path().join("extracted"); + tokio::fs::create_dir_all(&extract_path).await?; + let zip_handler = crate::storage::zip::ZipHandler::new()?; + zip_handler.extract_to_dir(&resolved_path, &extract_path)?; + crate::storage::git::validate_cloned_skill(&extract_path)? + } else if editable { + // `commit` will symlink this path in place; the original directory + // must survive untouched (it stays the live, user-owned source), so + // `skill_path` points straight at it rather than a temp-dir copy. + crate::storage::git::validate_cloned_skill(&resolved_path)? + } else { + // Non-editable directory: copy into the (throwaway) temp dir so + // `commit`'s atomic-move step consumes the copy, never the caller's + // original source directory. + let validated_source = crate::storage::git::validate_cloned_skill(&resolved_path)?; + let copy_dest = temp_dir.path().join("copied"); + copy_dir_recursive(&validated_source, ©_dest).await?; + copy_dest + }; + + let frontmatter = read_skill_frontmatter(&skill_path).await?; + let (_, version) = derive_skill_id_and_version(&skill_path, &frontmatter)?; + + Ok(Fetched { + temp_dir, + skill_path, + resolved: Resolved { + version, + commit_hash: None, + checksum: None, + }, + }) + } + + async fn fetch_zip_url(&self, url: &str) -> Result { + let response = reqwest::get(url) + .await + .map_err(|e| { + ServiceError::InvalidOperation(format!("Failed to download '{url}': {e}")) + })? + .error_for_status() + .map_err(|e| { + ServiceError::InvalidOperation(format!("Failed to download '{url}': {e}")) + })?; + let bytes = response + .bytes() + .await + .map_err(|e| ServiceError::InvalidOperation(format!("Failed to read '{url}': {e}")))?; + + let temp_dir = TempDir::new()?; + let zip_path = temp_dir.path().join("package.zip"); + let extract_path = temp_dir.path().join("extracted"); + tokio::fs::write(&zip_path, &bytes).await?; + tokio::fs::create_dir_all(&extract_path).await?; + + let zip_handler = crate::storage::zip::ZipHandler::new()?; + zip_handler.extract_to_dir(&zip_path, &extract_path)?; + let skill_path = crate::storage::git::validate_cloned_skill(&extract_path)?; + + let frontmatter = read_skill_frontmatter(&skill_path).await?; + let (_, version) = derive_skill_id_and_version(&skill_path, &frontmatter)?; + + Ok(Fetched { + temp_dir, + skill_path, + resolved: Resolved { + version, + commit_hash: None, + checksum: None, + }, + }) + } + + async fn fetch_repository( + &self, + repo: &str, + skill: &str, + version: Option<&VersionConstraint>, + ) -> Result { + let repo_manager = self.repository_manager().ok_or_else(|| { + ServiceError::Config( + "No repositories configured; cannot fetch an Origin::Repository skill".to_string(), + ) + })?; + let repo_name = resolve_repo_name(repo_manager, repo)?; + let client = repo_manager.get_client(&repo_name).await?; + + let available = client + .get_versions(skill) + .await + .map_err(|e| ServiceError::Config(format!("Failed to get versions: {e}")))?; + let candidates: Vec = match version { + Some(constraint) => available + .into_iter() + .filter(|v| constraint.satisfies(v).unwrap_or(false)) + .collect(), + None => available, + }; + let resolved_version = newest_version(&candidates).ok_or_else(|| { + ServiceError::Config(format!( + "No version of '{skill}' satisfies the requested constraint in repository \ + '{repo_name}'" + )) + })?; + + let zip_data = client + .download(skill, &resolved_version) + .await + .map_err(|e| ServiceError::Config(format!("Failed to download package: {e}")))?; + + let temp_dir = TempDir::new()?; + let extract_path = temp_dir.path().join("extracted"); + tokio::fs::create_dir_all(&extract_path).await?; + let zip_path = temp_dir + .path() + .join(format!("package-{resolved_version}.zip")); + tokio::fs::write(&zip_path, &zip_data).await?; + + let zip_handler = crate::storage::zip::ZipHandler::new()?; + zip_handler.extract_to_dir(&zip_path, &extract_path)?; + let skill_path = crate::storage::git::validate_cloned_skill(&extract_path)?; + + let frontmatter = read_skill_frontmatter(&skill_path).await?; + // The registry-resolved version selected the package to download; the + // *recorded* resolved version is whatever the downloaded SKILL.md / + // skill-project.toml declares (matching the pre-seam CLI behavior, where + // the lock's `Resolved.version` always came from the installed skill's + // own metadata, not the registry's version string). + let (_, version_from_skill) = derive_skill_id_and_version(&skill_path, &frontmatter)?; + + Ok(Fetched { + temp_dir, + skill_path, + resolved: Resolved { + version: version_from_skill, + commit_hash: None, + checksum: None, + }, + }) } /// The shared post-fetch pipeline: validate → atomic-move into the skills dir → @@ -87,26 +311,953 @@ impl FastSkillService { origin: Origin, mode: AddMode, ) -> Result { - let _ = (fetched, origin, mode); - // TODO(phase1-agent-B): validate the fetched skill, derive its id, enforce - // the mode's id-conflict policy (Fresh → error on existing), atomic-rename - // into the skills dir, upsert the manifest entry, write the lock entry - // (origin + resolved), then `self.reindex(None, None)` best-effort and set - // `reindexed`. Return the AddOutcome. - Err(ServiceError::InvalidOperation( - "add_from_origin: commit not yet implemented".to_string(), - )) + let Fetched { + temp_dir, + skill_path, + resolved, + } = fetched; + + let frontmatter = read_skill_frontmatter(&skill_path).await?; + let (id, _version) = derive_skill_id_and_version(&skill_path, &frontmatter)?; + + let existing = self.skill_manager().get_skill(&id).await?; + if mode == AddMode::Fresh && existing.is_some() { + return Err(ServiceError::AlreadyIndexed(id.into_string())); + } + + let storage_dir = self.config().skill_storage_path.join(id.as_str()); + let editable = matches!(&origin, Origin::Local { editable: true, .. }); + if editable { + symlink_into_storage(&skill_path, &storage_dir).await?; + } else { + move_or_copy_into_storage(&skill_path, &storage_dir).await?; + } + // The fetched contents now live at `storage_dir` (moved, copied, or + // symlinked-to); the temp dir (if anything of it remains) can go. + drop(temp_dir); + + let fetched_at = chrono::Utc::now(); + let mut skill_def = SkillDefinition::new( + id.clone(), + frontmatter.name, + frontmatter.description, + resolved.version.clone(), + origin.clone(), + ); + skill_def.skill_file = storage_dir.join("SKILL.md"); + skill_def.author = frontmatter.author; + skill_def.commit_hash = resolved.commit_hash.clone(); + skill_def.fetched_at = Some(fetched_at); + + self.skill_manager() + .force_register_skill(skill_def.clone()) + .await?; + + self.upsert_manifest_and_lock(&skill_def)?; + + let reindexed = match self.reindex(None, None).await { + Ok(outcome) => outcome.reindexed, + Err(e) => { + // A reindex failure must not fail the commit (ADR-0005 §Q3): the + // skill is already installed and recorded; reindexing is retried + // by any subsequent `reindex` call. + tracing::warn!("post-install reindex failed (non-fatal): {}", e); + false + } + }; + + Ok(AddOutcome { + id: id.into_string(), + origin, + resolved, + reindexed, + }) + } + + /// Upsert the skill-project.toml `[dependencies]` entry and the project + /// `skills.lock` entry for a just-installed skill. Resolves the project file + /// from the current working directory (mirrors the CLI's + /// `manifest_utils::add_skill_to_project_toml` / `update_lock_file`). + fn upsert_manifest_and_lock(&self, skill_def: &SkillDefinition) -> Result<(), ServiceError> { + let current_dir = std::env::current_dir()?; + let project_file_result = resolve_project_file(¤t_dir); + if !project_file_result.found { + return Err(ServiceError::Config( + "skill-project.toml not found in this directory or any parent. Run \ + `fastskill init` at the project root before adding skills." + .to_string(), + )); + } + let project_file_path = project_file_result.path; + + let mut project = SkillProjectToml::load_from_file(&project_file_path) + .map_err(|e| ServiceError::Config(format!("Failed to load skill-project.toml: {e}")))?; + + let mut context = project_file_result.context; + if context == ProjectContext::Ambiguous { + context = detect_context_from_content(&project); + } + if context == ProjectContext::Skill { + return Err(ServiceError::Config( + "Cannot add dependencies to a skill-level skill-project.toml (this directory \ + contains SKILL.md); run the add from the project root instead." + .to_string(), + )); + } + project.validate_for_context(context).map_err(|e| { + ServiceError::Config(format!("skill-project.toml validation failed: {e}")) + })?; + + if project.dependencies.is_none() { + project.dependencies = Some(DependenciesSection { + dependencies: HashMap::new(), + }); + } + if let Some(deps) = project.dependencies.as_mut() { + // Safety net: re-canonicalize a local path to an absolute path before + // persisting it, in case the caller passed a relative one. + let origin_for_manifest = match &skill_def.origin { + Origin::Local { path, editable } => { + let canonical = path.canonicalize()?; + Origin::Local { + path: canonical, + editable: *editable, + } + } + other => other.clone(), + }; + deps.dependencies.insert( + skill_def.id.to_string(), + DependencySpec::Inline { + origin: origin_for_manifest, + groups: None, + }, + ); + } + + project + .save_to_file(&project_file_path) + .map_err(|e| ServiceError::Config(format!("Failed to save skill-project.toml: {e}")))?; + + let lock_path = project_lock_path(&project_file_path); + let mut lock = if lock_path.exists() { + ProjectSkillsLock::load_from_file(&lock_path) + .map_err(|e| ServiceError::Config(format!("Failed to load skills.lock: {e}")))? + } else { + ProjectSkillsLock::new_empty() + }; + lock.update_skill(skill_def); + lock.save_to_file(&lock_path) + .map_err(|e| ServiceError::Config(format!("Failed to save skills.lock: {e}")))?; + + Ok(()) } /// Update preflight (ADR-0005 §Q6): decide whether the recorded origin has /// anything to update before doing any fetch. `repository` → newest allowed - /// via UpdateService; immutable git tag/commit + editable local → `Immutable`; - /// git branch / local copy / zip-url → `Updatable` (re-fetch; commit is idempotent). + /// via the repository client; immutable git tag/commit + editable local → + /// `Immutable`; git branch / local copy / zip-url → `Updatable` (re-fetch; + /// commit is idempotent). pub async fn preflight(&self, origin: &Origin) -> Result { - let _ = origin; - // TODO(phase1-agent-B): implement the per-variant preflight table. - Err(ServiceError::InvalidOperation( - "update preflight not yet implemented".to_string(), - )) + match origin { + Origin::Git { r#ref, .. } => match r#ref { + GitRef::Tag(t) => Ok(UpdatePreflight::Immutable { + reason: format!("pinned to git tag '{t}'; tags do not move"), + }), + GitRef::Commit(c) => Ok(UpdatePreflight::Immutable { + reason: format!("pinned to git commit '{c}'; commits do not move"), + }), + GitRef::Branch(_) | GitRef::Default => Ok(UpdatePreflight::Updatable), + }, + Origin::Local { editable, .. } => { + if *editable { + Ok(UpdatePreflight::Immutable { + reason: "editable local install is a live symlink to the source \ + directory; there is nothing to re-fetch" + .to_string(), + }) + } else { + Ok(UpdatePreflight::Updatable) + } + } + Origin::ZipUrl { .. } => Ok(UpdatePreflight::Updatable), + Origin::Repository { + repo, + skill, + version, + } => { + let repo_manager = self.repository_manager().ok_or_else(|| { + ServiceError::Config( + "No repositories configured; cannot preflight an Origin::Repository \ + update" + .to_string(), + ) + })?; + let repo_name = resolve_repo_name(repo_manager, repo)?; + let client = repo_manager.get_client(&repo_name).await?; + let available = client + .get_versions(skill) + .await + .map_err(|e| ServiceError::Config(format!("Failed to get versions: {e}")))?; + let candidates: Vec = match version { + Some(constraint) => available + .into_iter() + .filter(|v| constraint.satisfies(v).unwrap_or(false)) + .collect(), + None => available, + }; + let Some(target_version) = newest_version(&candidates) else { + // Nothing satisfies the constraint: no update to offer. + return Ok(UpdatePreflight::UpToDate); + }; + + // Best-effort: the currently-installed `SkillId` is normally the + // last path segment of the registry reference (`scope/id`). + let local_id = skill.rsplit('/').next().unwrap_or(skill.as_str()); + let installed_version = match SkillId::new(local_id.to_string()) { + Ok(id) => self + .skill_manager() + .get_skill(&id) + .await? + .map(|s| s.version), + Err(_) => None, + }; + + match installed_version { + Some(current) if !is_newer(&target_version, ¤t).unwrap_or(true) => { + Ok(UpdatePreflight::UpToDate) + } + _ => Ok(UpdatePreflight::Updatable), + } + } + } + } +} + +// ── Free helper functions ───────────────────────────────────────────────────── + +/// Resolve a `repo` name (the `Origin::Repository.repo` field) against a +/// [`RepositoryManager`]: `"default"` resolves to the configured default +/// repository's name, anything else is used verbatim. +fn resolve_repo_name(repo_manager: &RepositoryManager, repo: &str) -> Result { + if repo == "default" { + repo_manager + .get_default_repository() + .map(|r| r.name.clone()) + .ok_or_else(|| ServiceError::Config("No default repository configured".to_string())) + } else { + Ok(repo.to_string()) + } +} + +/// Read and parse `SKILL.md`'s frontmatter from a fetched skill directory. +async fn read_skill_frontmatter(skill_path: &Path) -> Result { + let content = tokio::fs::read_to_string(skill_path.join("SKILL.md")).await?; + parse_yaml_frontmatter(&content) +} + +/// Derive `(SkillId, version)` from a fetched skill directory: `skill-project.toml` +/// `[metadata]` wins when present, else `SKILL.md` frontmatter (`metadata.id`/ +/// `.version` sub-map, else `name`/top-level `version`, else `"1.0.0"`). Mirrors +/// `fastskill-cli`'s `create_skill_from_path` precedence. +fn derive_skill_id_and_version( + skill_path: &Path, + frontmatter: &SkillFrontmatter, +) -> Result<(SkillId, String), ServiceError> { + let toml_path = skill_path.join("skill-project.toml"); + let mut id_from_toml = None; + let mut version_from_toml = None; + if toml_path.exists() { + let content = std::fs::read_to_string(&toml_path)?; + let project: SkillProjectToml = toml::from_str(&content).map_err(|e| { + ServiceError::Validation(format!("Failed to parse skill-project.toml: {e}")) + })?; + if let Some(metadata) = project.metadata { + id_from_toml = metadata.id; + version_from_toml = metadata.version; + } + } + + let id_str = id_from_toml.unwrap_or_else(|| { + frontmatter + .metadata + .as_ref() + .and_then(|m| m.get("id").cloned()) + .unwrap_or_else(|| frontmatter.name.clone()) + }); + let id = SkillId::new(id_str)?; + + let version = version_from_toml + .or_else(|| { + frontmatter + .metadata + .as_ref() + .and_then(|m| m.get("version").cloned()) + }) + .or_else(|| frontmatter.version.clone()) + .unwrap_or_else(|| "1.0.0".to_string()); + + Ok((id, version)) +} + +/// Safely join an untrusted `subdir` (from a git tree reference) onto a trusted +/// clone `root`, rejecting path traversal. Mirrors the CLI's +/// `install_utils::safe_subdir_join`. +fn safe_subdir_join(root: &Path, subdir: &Path) -> Result { + use std::path::Component; + + let mut joined = root.to_path_buf(); + for component in subdir.components() { + match component { + Component::Normal(part) => { + let part = part.to_str().ok_or_else(|| { + ServiceError::InvalidOperation(format!( + "Subdirectory '{}' contains a non-UTF-8 path component", + subdir.display() + )) + })?; + crate::security::path::validate_path_component(part).map_err(|e| { + ServiceError::InvalidOperation(format!( + "Invalid subdirectory '{}': {}", + subdir.display(), + e + )) + })?; + joined.push(part); + } + // `..`, root (`/`), prefix (`C:`), etc. are all traversal / absolute markers. + _ => { + return Err(ServiceError::InvalidOperation(format!( + "Subdirectory '{}' must be a relative path without '..' components", + subdir.display() + ))); + } + } + } + + if joined.exists() { + let canonical_root = root.canonicalize()?; + let canonical_joined = joined.canonicalize()?; + if !canonical_joined.starts_with(&canonical_root) { + return Err(ServiceError::InvalidOperation(format!( + "Subdirectory '{}' escapes the cloned repository", + subdir.display() + ))); + } + } + + Ok(joined) +} + +/// Resolve `HEAD`'s commit SHA in a freshly-cloned git repository. +async fn git_head_commit(repo_dir: &Path) -> Result { + let output = tokio::process::Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(repo_dir) + .output() + .await?; + if !output.status.success() { + return Err(ServiceError::Custom(format!( + "git rev-parse HEAD failed: {}", + String::from_utf8_lossy(&output.stderr) + ))); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) +} + +/// Remove whatever currently sits at `path` (file, symlink, or directory), if +/// anything, so a fresh move/copy/symlink can take its place. +async fn remove_existing_storage_path(path: &Path) -> Result<(), ServiceError> { + if path.is_symlink() || path.is_file() { + tokio::fs::remove_file(path).await?; + } else if path.exists() { + tokio::fs::remove_dir_all(path).await?; + } + Ok(()) +} + +/// Move `skill_path` into `storage_dir` (same-filesystem rename when possible, +/// falling back to a recursive copy across filesystems/temp-dir boundaries). +async fn move_or_copy_into_storage( + skill_path: &Path, + storage_dir: &Path, +) -> Result<(), ServiceError> { + remove_existing_storage_path(storage_dir).await?; + if let Some(parent) = storage_dir.parent() { + tokio::fs::create_dir_all(parent).await?; + } + if tokio::fs::rename(skill_path, storage_dir).await.is_ok() { + return Ok(()); + } + // Cross-device (or other rename failure): fall back to a recursive copy. + copy_dir_recursive(skill_path, storage_dir).await +} + +/// Symlink `storage_dir` -> `skill_path` (editable local installs). +async fn symlink_into_storage(skill_path: &Path, storage_dir: &Path) -> Result<(), ServiceError> { + remove_existing_storage_path(storage_dir).await?; + if let Some(parent) = storage_dir.parent() { + tokio::fs::create_dir_all(parent).await?; + } + #[cfg(unix)] + { + tokio::fs::symlink(skill_path, storage_dir).await?; + } + #[cfg(windows)] + { + std::os::windows::fs::symlink_dir(skill_path, storage_dir)?; + } + #[cfg(all(not(unix), not(windows)))] + { + return Err(ServiceError::InvalidOperation( + "Editable installations are not supported on this platform. Use a Unix-based \ + system or Docker." + .to_string(), + )); + } + Ok(()) +} + +/// Recursively copy a directory from `src` to `dst`, rejecting symlink entries +/// (SEC-4: a symlink inside the source tree must not be silently dereferenced +/// and its target's contents exfiltrated into the copy). +async fn copy_dir_recursive(src: &Path, dst: &Path) -> Result<(), ServiceError> { + tokio::fs::create_dir_all(dst).await?; + let mut entries = tokio::fs::read_dir(src).await?; + while let Some(entry) = entries.next_entry().await? { + let ty = entry.file_type().await?; + let src_path = entry.path(); + let dst_path = dst.join(entry.file_name()); + + if ty.is_symlink() { + return Err(ServiceError::Validation(format!( + "refusing to copy symlink: {}", + src_path.display() + ))); + } + if ty.is_dir() { + Box::pin(copy_dir_recursive(&src_path, &dst_path)).await?; + } else { + tokio::fs::copy(&src_path, &dst_path).await?; + } + } + Ok(()) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +mod tests { + use super::*; + use crate::{FastSkillService, ServiceConfig}; + use tempfile::TempDir as TestTempDir; + + const VALID_SKILL_MD: &str = + "---\nname: test-skill\nversion: \"1.0.0\"\ndescription: A test skill\n---\nBody\n"; + + fn write_valid_skill(parent: &Path, dir_name: &str) -> PathBuf { + let dir = parent.join(dir_name); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("SKILL.md"), VALID_SKILL_MD).unwrap(); + dir + } + + /// Set up a project directory (skill-project.toml + skills dir) and chdir + /// into it, returning a guard that restores the cwd and holds the temp dir. + fn setup_project() -> (TestTempDir, crate::test_utils::DirGuard, PathBuf) { + let tmp = TestTempDir::new().unwrap(); + let original_dir = std::env::current_dir().ok(); + std::env::set_current_dir(tmp.path()).unwrap(); + std::fs::write( + tmp.path().join("skill-project.toml"), + "[tool.fastskill]\nskills_directory = \".claude/skills\"\n\n[dependencies]\n", + ) + .unwrap(); + let skills_dir = tmp.path().join(".claude/skills"); + std::fs::create_dir_all(&skills_dir).unwrap(); + (tmp, crate::test_utils::DirGuard(original_dir), skills_dir) + } + + async fn make_service(storage: &Path) -> FastSkillService { + let config = ServiceConfig { + skill_storage_path: storage.to_path_buf(), + ..Default::default() + }; + let mut service = FastSkillService::new(config).await.unwrap(); + service.initialize().await.unwrap(); + service + } + + // ── add_from_origin: Local, end-to-end ──────────────────────────────────── + + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_add_from_origin_local_end_to_end() { + let _lock = crate::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (tmp, _guard, skills_dir) = setup_project(); + let src = write_valid_skill(tmp.path(), "src-skill"); + let service = make_service(&skills_dir).await; + + let origin = Origin::Local { + path: src.clone(), + editable: false, + }; + let outcome = service + .add_from_origin(origin, AddMode::Fresh) + .await + .expect("add should succeed"); + + assert_eq!(outcome.id, "test-skill"); + assert_eq!(outcome.resolved.version, "1.0.0"); + assert!(skills_dir.join("test-skill/SKILL.md").exists()); + + // Manifest + lock were written. + let project = SkillProjectToml::load_from_file(&tmp.path().join("skill-project.toml")) + .expect("manifest should load"); + assert!(project + .dependencies + .expect("deps section") + .dependencies + .contains_key("test-skill")); + let lock = ProjectSkillsLock::load_from_file(&tmp.path().join("skills.lock")) + .expect("lock should load"); + assert_eq!(lock.skills.len(), 1); + assert_eq!(lock.skills[0].id, "test-skill"); + + // Registered with the skill manager. + let id = SkillId::new("test-skill".to_string()).unwrap(); + assert!(service + .skill_manager() + .get_skill(&id) + .await + .unwrap() + .is_some()); + } + + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_add_from_origin_fresh_conflict() { + let _lock = crate::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (tmp, _guard, skills_dir) = setup_project(); + let src = write_valid_skill(tmp.path(), "src-skill"); + let service = make_service(&skills_dir).await; + + let origin = Origin::Local { + path: src.clone(), + editable: false, + }; + service + .add_from_origin(origin.clone(), AddMode::Fresh) + .await + .expect("first add should succeed"); + + let result = service.add_from_origin(origin, AddMode::Fresh).await; + assert!(matches!(result, Err(ServiceError::AlreadyIndexed(_)))); + } + + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_add_from_origin_update_overwrites() { + let _lock = crate::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (tmp, _guard, skills_dir) = setup_project(); + let src = write_valid_skill(tmp.path(), "src-skill"); + let service = make_service(&skills_dir).await; + + let origin = Origin::Local { + path: src.clone(), + editable: false, + }; + service + .add_from_origin(origin.clone(), AddMode::Fresh) + .await + .expect("first add should succeed"); + + // Update the source content, then re-add via Update mode. + std::fs::write( + src.join("SKILL.md"), + "---\nname: test-skill\nversion: \"2.0.0\"\ndescription: updated\n---\nBody\n", + ) + .unwrap(); + + let outcome = service + .add_from_origin(origin, AddMode::Update) + .await + .expect("update should succeed"); + assert_eq!(outcome.resolved.version, "2.0.0"); + + let id = SkillId::new("test-skill".to_string()).unwrap(); + let skill = service + .skill_manager() + .get_skill(&id) + .await + .unwrap() + .unwrap(); + assert_eq!(skill.version, "2.0.0"); + } + + #[cfg(unix)] + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_add_from_origin_local_editable_symlinks() { + let _lock = crate::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (tmp, _guard, skills_dir) = setup_project(); + let src = write_valid_skill(tmp.path(), "src-skill"); + let service = make_service(&skills_dir).await; + + let origin = Origin::Local { + path: src.clone(), + editable: true, + }; + let outcome = service + .add_from_origin(origin, AddMode::Fresh) + .await + .expect("editable add should succeed"); + + let storage_path = skills_dir.join(&outcome.id); + assert!(storage_path.is_symlink(), "editable install must symlink"); + } + + #[tokio::test] + async fn test_add_from_origin_local_nonexistent_path() { + let tmp = TestTempDir::new().unwrap(); + let storage = tmp.path().join("storage"); + let service = make_service(&storage).await; + + let origin = Origin::Local { + path: tmp.path().join("does-not-exist"), + editable: false, + }; + let result = service.add_from_origin(origin, AddMode::Fresh).await; + assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); + } + + // ── add_from_origin: ZipUrl, end-to-end (mock HTTP) ─────────────────────── + + fn build_skill_zip() -> Vec { + use std::io::Write; + use zip::write::FileOptions; + let mut buf = Vec::new(); + { + let cursor = std::io::Cursor::new(&mut buf); + let mut writer = zip::ZipWriter::new(cursor); + let opts = FileOptions::default().compression_method(zip::CompressionMethod::Stored); + writer.start_file("test-skill/SKILL.md", opts).unwrap(); + writer.write_all(VALID_SKILL_MD.as_bytes()).unwrap(); + writer.finish().unwrap(); + } + buf + } + + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_add_from_origin_zip_url_end_to_end() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let _lock = crate::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (_tmp, _guard, skills_dir) = setup_project(); + let service = make_service(&skills_dir).await; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/pkg.zip")) + .respond_with(ResponseTemplate::new(200).set_body_bytes(build_skill_zip())) + .mount(&server) + .await; + + let origin = Origin::ZipUrl { + url: format!("{}/pkg.zip", server.uri()), + }; + let outcome = service + .add_from_origin(origin, AddMode::Fresh) + .await + .expect("zip-url add should succeed"); + assert_eq!(outcome.id, "test-skill"); + assert!(skills_dir.join("test-skill/SKILL.md").exists()); + } + + // ── add_from_origin: Repository without a repository manager ───────────── + + #[tokio::test] + async fn test_add_from_origin_repository_requires_manager() { + let tmp = TestTempDir::new().unwrap(); + let storage = tmp.path().join("storage"); + let service = make_service(&storage).await; + + let origin = Origin::Repository { + repo: "default".to_string(), + skill: "scope/skill".to_string(), + version: None, + }; + let result = service.add_from_origin(origin, AddMode::Fresh).await; + assert!(matches!(result, Err(ServiceError::Config(_)))); + } + + // ── fetch_git: GitRef::Commit is a clear, fast error (no clone-by-commit) ── + + #[tokio::test] + async fn test_add_from_origin_git_commit_ref_unsupported() { + let tmp = TestTempDir::new().unwrap(); + let storage = tmp.path().join("storage"); + let service = make_service(&storage).await; + + let origin = Origin::Git { + url: "https://example.com/x.git".to_string(), + r#ref: GitRef::Commit("deadbeef".to_string()), + subdir: None, + }; + let result = service.add_from_origin(origin, AddMode::Fresh).await; + assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); + } + + // ── preflight ────────────────────────────────────────────────────────────── + + #[tokio::test] + async fn test_preflight_git_tag_is_immutable() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::Git { + url: "u".to_string(), + r#ref: GitRef::Tag("v1.0.0".to_string()), + subdir: None, + }; + assert!(matches!( + service.preflight(&origin).await.unwrap(), + UpdatePreflight::Immutable { .. } + )); + } + + #[tokio::test] + async fn test_preflight_git_commit_is_immutable() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::Git { + url: "u".to_string(), + r#ref: GitRef::Commit("abc123".to_string()), + subdir: None, + }; + assert!(matches!( + service.preflight(&origin).await.unwrap(), + UpdatePreflight::Immutable { .. } + )); + } + + #[tokio::test] + async fn test_preflight_git_branch_is_updatable() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::Git { + url: "u".to_string(), + r#ref: GitRef::Branch("main".to_string()), + subdir: None, + }; + assert!(matches!( + service.preflight(&origin).await.unwrap(), + UpdatePreflight::Updatable + )); + } + + #[tokio::test] + async fn test_preflight_git_default_is_updatable() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::Git { + url: "u".to_string(), + r#ref: GitRef::Default, + subdir: None, + }; + assert!(matches!( + service.preflight(&origin).await.unwrap(), + UpdatePreflight::Updatable + )); + } + + #[tokio::test] + async fn test_preflight_local_editable_is_immutable() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::Local { + path: tmp.path().to_path_buf(), + editable: true, + }; + assert!(matches!( + service.preflight(&origin).await.unwrap(), + UpdatePreflight::Immutable { .. } + )); + } + + #[tokio::test] + async fn test_preflight_local_copy_is_updatable() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::Local { + path: tmp.path().to_path_buf(), + editable: false, + }; + assert!(matches!( + service.preflight(&origin).await.unwrap(), + UpdatePreflight::Updatable + )); + } + + #[tokio::test] + async fn test_preflight_zip_url_is_updatable() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::ZipUrl { + url: "https://example.com/x.zip".to_string(), + }; + assert!(matches!( + service.preflight(&origin).await.unwrap(), + UpdatePreflight::Updatable + )); + } + + #[tokio::test] + async fn test_preflight_repository_requires_manager() { + let tmp = TestTempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = Origin::Repository { + repo: "default".to_string(), + skill: "scope/skill".to_string(), + version: None, + }; + let result = service.preflight(&origin).await; + assert!(matches!(result, Err(ServiceError::Config(_)))); + } + + // ── resolve_repo_name ────────────────────────────────────────────────────── + + #[test] + fn test_resolve_repo_name_default_alias() { + use crate::core::repository::{RepositoryConfig, RepositoryDefinition, RepositoryType}; + let manager = RepositoryManager::from_definitions(vec![RepositoryDefinition { + name: "my-registry".to_string(), + repo_type: RepositoryType::HttpRegistry, + priority: 0, + config: RepositoryConfig::HttpRegistry { + index_url: "https://example.com/index".to_string(), + }, + auth: None, + storage: None, + }]); + assert_eq!( + resolve_repo_name(&manager, "default").unwrap(), + "my-registry" + ); + assert_eq!( + resolve_repo_name(&manager, "my-registry").unwrap(), + "my-registry" + ); + } + + #[test] + fn test_resolve_repo_name_no_repositories_errors() { + let manager = RepositoryManager::from_definitions(Vec::new()); + assert!(resolve_repo_name(&manager, "default").is_err()); + } + + // ── safe_subdir_join ─────────────────────────────────────────────────────── + + #[test] + fn test_safe_subdir_join_rejects_dotdot() { + let root = TestTempDir::new().unwrap(); + let result = safe_subdir_join(root.path(), Path::new("../../../etc")); + assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); + } + + #[test] + fn test_safe_subdir_join_rejects_absolute() { + let root = TestTempDir::new().unwrap(); + let result = safe_subdir_join(root.path(), Path::new("/etc/passwd")); + assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); + } + + #[test] + fn test_safe_subdir_join_accepts_nested_relative() { + let root = TestTempDir::new().unwrap(); + std::fs::create_dir_all(root.path().join("skills/inner")).unwrap(); + let joined = safe_subdir_join(root.path(), Path::new("skills/inner")).unwrap(); + assert_eq!(joined, root.path().join("skills").join("inner")); + } + + // ── copy_dir_recursive ───────────────────────────────────────────────────── + + #[cfg(unix)] + #[tokio::test] + async fn test_copy_dir_recursive_rejects_symlink() { + use std::os::unix::fs::symlink; + let tmp = TestTempDir::new().unwrap(); + let src = tmp.path().join("src"); + std::fs::create_dir_all(&src).unwrap(); + std::fs::write(src.join("SKILL.md"), "# skill\n").unwrap(); + let secret = tmp.path().join("secret.txt"); + std::fs::write(&secret, "TOP SECRET").unwrap(); + symlink(&secret, src.join("creds")).unwrap(); + + let dst = tmp.path().join("dst"); + let result = copy_dir_recursive(&src, &dst).await; + assert!(matches!(result, Err(ServiceError::Validation(_)))); + assert!(!dst.join("creds").exists()); + } + + #[tokio::test] + async fn test_copy_dir_recursive_copies_regular_tree() { + let tmp = TestTempDir::new().unwrap(); + let src = tmp.path().join("src"); + std::fs::create_dir_all(src.join("nested")).unwrap(); + std::fs::write(src.join("SKILL.md"), "# skill\n").unwrap(); + std::fs::write(src.join("nested/file.txt"), "data").unwrap(); + + let dst = tmp.path().join("dst"); + copy_dir_recursive(&src, &dst).await.unwrap(); + assert!(dst.join("SKILL.md").exists()); + assert!(dst.join("nested/file.txt").exists()); + } + + // ── derive_skill_id_and_version ──────────────────────────────────────────── + + #[test] + fn test_derive_skill_id_and_version_toml_wins() { + let tmp = TestTempDir::new().unwrap(); + std::fs::write( + tmp.path().join("SKILL.md"), + "---\nname: from-md\nversion: \"2.0.0\"\ndescription: d\n---\n", + ) + .unwrap(); + std::fs::write( + tmp.path().join("skill-project.toml"), + "[metadata]\nid = \"from-toml\"\nversion = \"1.5.0\"\n", + ) + .unwrap(); + let content = std::fs::read_to_string(tmp.path().join("SKILL.md")).unwrap(); + let frontmatter = parse_yaml_frontmatter(&content).unwrap(); + let (id, version) = derive_skill_id_and_version(tmp.path(), &frontmatter).unwrap(); + assert_eq!(id.as_str(), "from-toml"); + assert_eq!(version, "1.5.0"); + } + + #[test] + fn test_derive_skill_id_and_version_falls_back_to_frontmatter_name() { + let tmp = TestTempDir::new().unwrap(); + std::fs::write( + tmp.path().join("SKILL.md"), + "---\nname: fallback-name\ndescription: d\n---\n", + ) + .unwrap(); + let content = std::fs::read_to_string(tmp.path().join("SKILL.md")).unwrap(); + let frontmatter = parse_yaml_frontmatter(&content).unwrap(); + let (id, version) = derive_skill_id_and_version(tmp.path(), &frontmatter).unwrap(); + assert_eq!(id.as_str(), "fallback-name"); + assert_eq!(version, "1.0.0"); } } diff --git a/crates/fastskill-core/src/core/reindex.rs b/crates/fastskill-core/src/core/reindex.rs index c57cff2..d65d538 100644 --- a/crates/fastskill-core/src/core/reindex.rs +++ b/crates/fastskill-core/src/core/reindex.rs @@ -6,8 +6,13 @@ //! silently**. The CLI/serve edge injects the provider via //! [`FastSkillService::with_embedding_service`]. +use crate::core::embedding::EmbeddingService; +use crate::core::metadata::parse_yaml_frontmatter; use crate::core::service::{FastSkillService, ServiceError}; -use std::path::Path; +use crate::core::vector_index::VectorIndexService; +use sha2::{Digest, Sha256}; +use std::collections::HashSet; +use std::path::{Path, PathBuf}; /// Progress datum emitted per skill as reindex proceeds. The core emits neutral /// data; the caller (CLI) decides how to render it (HTTP passes no observer). @@ -46,15 +51,433 @@ impl FastSkillService { skills_dir: Option<&Path>, observer: Option<&(dyn Fn(ReindexProgress) + Send + Sync)>, ) -> Result { - let _ = (skills_dir, observer); // No provider ⇒ skip silently (the common, non-configured case). - if self.embedding_service().is_none() { + let Some(embedding_service) = self.embedding_service() else { return Ok(ReindexOutcome::skipped("no embedding provider configured")); + }; + + // The vector index is built alongside the embedding config, so this + // should always be present once an embedding provider is injected. Guard + // defensively anyway rather than unwrapping. + let Some(vector_index_service) = self.vector_index_service() else { + return Ok(ReindexOutcome::skipped("no vector index configured")); + }; + + let dir = skills_dir + .map(Path::to_path_buf) + .unwrap_or_else(|| self.config().skill_storage_path.clone()); + + tracing::info!( + "Reindexing vector index for skills directory: {}", + dir.display() + ); + + let skill_files = find_skill_files(&dir)?; + + if skill_files.is_empty() { + tracing::info!("No skills found in {}", dir.display()); + return Ok(ReindexOutcome { + reindexed: true, + count: 0, + reason: None, + }); + } + + let total = skill_files.len(); + + // Collect current skill IDs so stale index entries (skills removed from + // disk since the last reindex) can be pruned below. + let current_skill_ids: HashSet = skill_files + .iter() + .filter_map(|f| skill_id_from_path(f)) + .collect(); + + let mut count = 0usize; + for (idx, skill_file) in skill_files.into_iter().enumerate() { + let skill_id = skill_id_from_path(&skill_file).unwrap_or_else(|| "unknown".to_string()); + + if let Some(obs) = observer { + obs(ReindexProgress { + current: idx + 1, + total, + skill_id: skill_id.clone(), + }); + } + + match index_skill_file( + &skill_file, + &skill_id, + embedding_service.as_ref(), + vector_index_service.as_ref(), + ) + .await + { + Ok(true) => count += 1, + Ok(false) => { + // Unchanged hash: nothing to do. + } + Err(e) => { + // A single skill failing to index should not abort the whole + // reindex run; log and continue with the rest. + tracing::warn!("Failed to reindex skill {}: {}", skill_id, e); + } + } + } + + // Cleanup: remove skills from the index that are no longer on disk. + match vector_index_service.get_all_skills().await { + Ok(all_indexed_skills) => { + for indexed_skill in all_indexed_skills { + if !current_skill_ids.contains(&indexed_skill.id) { + tracing::info!("Removing stale index entry: {}", indexed_skill.id); + if let Err(e) = vector_index_service.remove_skill(&indexed_skill.id).await { + tracing::warn!( + "Failed to remove stale index entry {}: {}", + indexed_skill.id, + e + ); + } + } + } + } + Err(e) => { + tracing::warn!("Failed to retrieve all indexed skills for cleanup: {}", e); + } + } + + Ok(ReindexOutcome { + reindexed: true, + count, + reason: None, + }) + } +} + +/// Derive a skill ID from a `SKILL.md` path: the name of its parent directory. +fn skill_id_from_path(skill_file: &Path) -> Option { + skill_file + .parent() + .and_then(|parent_dir| parent_dir.file_name()) + .map(|name| name.to_string_lossy().to_string()) +} + +/// Find all `SKILL.md` files under `skills_dir`. +fn find_skill_files(skills_dir: &Path) -> Result, ServiceError> { + if !skills_dir.exists() { + return Err(ServiceError::Config(format!( + "Skills directory does not exist: {}", + skills_dir.display() + ))); + } + + let skill_files = walkdir::WalkDir::new(skills_dir) + .into_iter() + .filter_map(|e| e.ok()) + .filter(|e| e.file_type().is_file()) + .filter(|e| e.file_name() == "SKILL.md") + .map(|e| e.path().to_path_buf()) + .collect(); + + Ok(skill_files) +} + +/// Index a single skill file. Returns `Ok(true)` if the index was updated, +/// `Ok(false)` if the skill was already up to date (unchanged file hash). +async fn index_skill_file( + skill_file: &Path, + skill_id: &str, + embedding_service: &dyn EmbeddingService, + vector_index_service: &dyn VectorIndexService, +) -> Result { + let file_hash = calculate_file_hash(skill_file)?; + + // Skip re-embedding when the file is unchanged since the last index write. + if let Ok(Some(indexed_skill)) = vector_index_service.get_skill_by_id(skill_id).await { + if indexed_skill.file_hash == file_hash { + return Ok(false); + } + } + + let skill_dir = skill_file.parent().ok_or_else(|| { + ServiceError::Validation("Skill file has no parent directory".to_string()) + })?; + + let content = std::fs::read_to_string(skill_file)?; + let frontmatter = parse_yaml_frontmatter(&content)?; + + let frontmatter_json = serde_json::to_value(&frontmatter) + .map_err(|e| ServiceError::Validation(format!("Failed to serialize frontmatter: {}", e)))?; + + let embedding_text = format!("{}\n{}", frontmatter.name, frontmatter.description); + let embedding = embedding_service.embed_text(&embedding_text).await?; + + vector_index_service + .add_or_update_skill( + skill_id, + skill_dir.to_path_buf(), + frontmatter_json, + embedding, + &file_hash, + ) + .await?; + + Ok(true) +} + +/// Calculate the SHA256 hash of a file's contents. +fn calculate_file_hash(file_path: &Path) -> Result { + let content = std::fs::read(file_path)?; + let mut hasher = Sha256::new(); + hasher.update(&content); + let hash_bytes = hasher.finalize(); + Ok(format!("{:x}", hash_bytes)) +} + +#[cfg(test)] +#[allow( + clippy::unwrap_used, + clippy::panic, + clippy::expect_used, + clippy::assertions_on_constants +)] +mod tests { + use super::*; + use crate::core::service::{EmbeddingConfig, ServiceConfig}; + use async_trait::async_trait; + use std::fs; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + use tempfile::TempDir; + + /// Deterministic, network-free embedding provider for tests. + struct MockEmbeddingService { + calls: AtomicUsize, + } + + impl MockEmbeddingService { + fn new() -> Self { + Self { + calls: AtomicUsize::new(0), + } + } + + fn call_count(&self) -> usize { + self.calls.load(Ordering::SeqCst) } - // TODO(phase1-agent-A): move the real reindex logic here from the CLI - // `execute_reindex` (find SKILL.md files, embed each via the injected - // provider, write to the VectorIndexService), emitting `observer` - // progress. Return `{reindexed:true, count, reason:None}`. - Ok(ReindexOutcome::skipped("reindex not yet implemented")) + } + + #[async_trait] + impl EmbeddingService for MockEmbeddingService { + async fn embed_text(&self, text: &str) -> Result, ServiceError> { + self.calls.fetch_add(1, Ordering::SeqCst); + Ok(vec![text.len() as f32, 0.0, 0.0]) + } + + async fn embed_query(&self, query: &str) -> Result, ServiceError> { + self.embed_text(query).await + } + } + + fn create_test_skill(skills_dir: &Path, skill_id: &str, name: &str, description: &str) { + let skill_dir = skills_dir.join(skill_id); + fs::create_dir_all(&skill_dir).unwrap(); + let skill_content = format!( + r#"--- +name: {} +description: {} +version: 1.0.0 +--- + +# {} + +Test skill content"#, + name, description, name + ); + fs::write(skill_dir.join("SKILL.md"), skill_content).unwrap(); + } + + #[tokio::test] + async fn test_reindex_skips_without_embedding_provider() { + let temp_dir = TempDir::new().unwrap(); + let config = ServiceConfig { + skill_storage_path: temp_dir.path().to_path_buf(), + embedding: None, + ..Default::default() + }; + + let mut service = FastSkillService::new(config).await.unwrap(); + service.initialize().await.unwrap(); + + let outcome = service.reindex(None, None).await.unwrap(); + + assert!(!outcome.reindexed); + assert_eq!(outcome.count, 0); + assert!(outcome.reason.is_some()); + } + + #[tokio::test] + async fn test_reindex_indexes_skills_with_injected_provider() { + let temp_dir = TempDir::new().unwrap(); + let skills_dir = temp_dir.path().join("skills"); + fs::create_dir_all(&skills_dir).unwrap(); + + create_test_skill(&skills_dir, "skill-one", "Skill One", "First test skill"); + create_test_skill(&skills_dir, "skill-two", "Skill Two", "Second test skill"); + + let config = ServiceConfig { + skill_storage_path: skills_dir.clone(), + embedding: Some(EmbeddingConfig { + openai_base_url: "https://api.openai.com/v1".to_string(), + embedding_model: "text-embedding-3-small".to_string(), + index_path: None, + }), + ..Default::default() + }; + + let mock_embedding = Arc::new(MockEmbeddingService::new()); + let mut service = FastSkillService::new(config) + .await + .unwrap() + .with_embedding_service(mock_embedding.clone()); + service.initialize().await.unwrap(); + + let progress_calls = Arc::new(std::sync::Mutex::new(Vec::new())); + let progress_calls_clone = progress_calls.clone(); + let observer = move |p: ReindexProgress| { + progress_calls_clone.lock().unwrap().push(p.skill_id); + }; + + let outcome = service + .reindex(Some(&skills_dir), Some(&observer)) + .await + .unwrap(); + + assert!(outcome.reindexed); + assert_eq!(outcome.count, 2); + assert!(outcome.reason.is_none()); + assert_eq!(mock_embedding.call_count(), 2); + assert_eq!(progress_calls.lock().unwrap().len(), 2); + + // Verify the skills actually landed in the vector index. + let vector_index = service.vector_index_service().unwrap(); + assert!(vector_index + .get_skill_by_id("skill-one") + .await + .unwrap() + .is_some()); + assert!(vector_index + .get_skill_by_id("skill-two") + .await + .unwrap() + .is_some()); + + // Reindexing again without changes should skip re-embedding (unchanged hash). + let outcome2 = service.reindex(Some(&skills_dir), None).await.unwrap(); + assert!(outcome2.reindexed); + assert_eq!(outcome2.count, 0); + assert_eq!(mock_embedding.call_count(), 2); + } + + #[tokio::test] + async fn test_reindex_removes_stale_entries() { + let temp_dir = TempDir::new().unwrap(); + let skills_dir = temp_dir.path().join("skills"); + fs::create_dir_all(&skills_dir).unwrap(); + + create_test_skill(&skills_dir, "skill-one", "Skill One", "First test skill"); + let skill_two_dir = skills_dir.join("skill-two"); + create_test_skill(&skills_dir, "skill-two", "Skill Two", "Second test skill"); + + let config = ServiceConfig { + skill_storage_path: skills_dir.clone(), + embedding: Some(EmbeddingConfig { + openai_base_url: "https://api.openai.com/v1".to_string(), + embedding_model: "text-embedding-3-small".to_string(), + index_path: None, + }), + ..Default::default() + }; + + let mock_embedding = Arc::new(MockEmbeddingService::new()); + let mut service = FastSkillService::new(config) + .await + .unwrap() + .with_embedding_service(mock_embedding); + service.initialize().await.unwrap(); + + let outcome = service.reindex(Some(&skills_dir), None).await.unwrap(); + assert_eq!(outcome.count, 2); + + fs::remove_dir_all(&skill_two_dir).unwrap(); + + let outcome2 = service.reindex(Some(&skills_dir), None).await.unwrap(); + assert_eq!(outcome2.count, 0); + + let vector_index = service.vector_index_service().unwrap(); + assert!(vector_index + .get_skill_by_id("skill-one") + .await + .unwrap() + .is_some()); + assert!(vector_index + .get_skill_by_id("skill-two") + .await + .unwrap() + .is_none()); + } + + #[tokio::test] + async fn test_reindex_empty_skills_dir() { + let temp_dir = TempDir::new().unwrap(); + let skills_dir = temp_dir.path().join("skills"); + fs::create_dir_all(&skills_dir).unwrap(); + + let config = ServiceConfig { + skill_storage_path: skills_dir.clone(), + embedding: Some(EmbeddingConfig { + openai_base_url: "https://api.openai.com/v1".to_string(), + embedding_model: "text-embedding-3-small".to_string(), + index_path: None, + }), + ..Default::default() + }; + + let mock_embedding = Arc::new(MockEmbeddingService::new()); + let mut service = FastSkillService::new(config) + .await + .unwrap() + .with_embedding_service(mock_embedding); + service.initialize().await.unwrap(); + + let outcome = service.reindex(Some(&skills_dir), None).await.unwrap(); + assert!(outcome.reindexed); + assert_eq!(outcome.count, 0); + assert!(outcome.reason.is_none()); + } + + #[tokio::test] + async fn test_reindex_nonexistent_skills_dir_errors() { + let temp_dir = TempDir::new().unwrap(); + let config = ServiceConfig { + skill_storage_path: temp_dir.path().to_path_buf(), + embedding: Some(EmbeddingConfig { + openai_base_url: "https://api.openai.com/v1".to_string(), + embedding_model: "text-embedding-3-small".to_string(), + index_path: None, + }), + ..Default::default() + }; + + let mock_embedding = Arc::new(MockEmbeddingService::new()); + let mut service = FastSkillService::new(config) + .await + .unwrap() + .with_embedding_service(mock_embedding); + service.initialize().await.unwrap(); + + let nonexistent_dir = temp_dir.path().join("nonexistent"); + let result = service.reindex(Some(&nonexistent_dir), None).await; + + assert!(result.is_err()); } } From bab13783302057fedd9744686c5750e62f50e21e Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 22 Jul 2026 12:26:16 +0000 Subject: [PATCH 3/4] fix(core): inject project_root into install seam (opus-review blocker) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `upsert_manifest_and_lock` resolved the manifest/lock from the process cwd — correct for the CLI, but the `serve`/HTTP path has an arbitrary cwd and would write to the wrong project. Add an edge-injected `project_root` on FastSkillService; the seam uses it when present, else falls back to cwd (CLI behavior unchanged). Makes add_from_origin serve-ready for Phase 2. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/fastskill-core/src/core/install.rs | 11 +++++++++-- crates/fastskill-core/src/core/service.rs | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/crates/fastskill-core/src/core/install.rs b/crates/fastskill-core/src/core/install.rs index 1306140..61a60d9 100644 --- a/crates/fastskill-core/src/core/install.rs +++ b/crates/fastskill-core/src/core/install.rs @@ -379,8 +379,15 @@ impl FastSkillService { /// from the current working directory (mirrors the CLI's /// `manifest_utils::add_skill_to_project_toml` / `update_lock_file`). fn upsert_manifest_and_lock(&self, skill_def: &SkillDefinition) -> Result<(), ServiceError> { - let current_dir = std::env::current_dir()?; - let project_file_result = resolve_project_file(¤t_dir); + // Resolve the project from the injected root (the served project, for the + // `serve` path) if present; otherwise walk up from the process cwd, which + // is correct for a CLI invocation. Never resolve solely from cwd on the + // server, where cwd is arbitrary (would write to the wrong project). + let start_dir = match self.project_root() { + Some(root) => root.clone(), + None => std::env::current_dir()?, + }; + let project_file_result = resolve_project_file(&start_dir); if !project_file_result.found { return Err(ServiceError::Config( "skill-project.toml not found in this directory or any parent. Run \ diff --git a/crates/fastskill-core/src/core/service.rs b/crates/fastskill-core/src/core/service.rs index 2c09332..601562e 100644 --- a/crates/fastskill-core/src/core/service.rs +++ b/crates/fastskill-core/src/core/service.rs @@ -298,6 +298,13 @@ pub struct FastSkillService { /// a repository-origin install returns a clear "no repositories configured" error. repository_manager: Option>, + /// Project root the install seam writes the Manifest + Lock under, injected at + /// the edge. The CLI leaves this `None` (it resolves the project from the + /// process cwd, which is correct for a CLI); the `serve` path MUST inject the + /// served project's root so `add_from_origin` doesn't write relative to the + /// server's arbitrary working directory. + project_root: Option, + /// Skill storage backend storage: Arc, @@ -366,6 +373,7 @@ impl FastSkillService { vector_index_service, embedding_service: None, repository_manager: None, + project_root: None, storage, hot_reload_manager, initialized: false, @@ -402,6 +410,19 @@ impl FastSkillService { self.repository_manager.as_ref() } + /// Inject the project root the install seam writes Manifest/Lock under (the + /// served project's root for `serve`). When unset the seam falls back to the + /// process cwd (correct for the CLI). + pub fn with_project_root(mut self, root: PathBuf) -> Self { + self.project_root = Some(root); + self + } + + /// The injected project root, if any. + pub fn project_root(&self) -> Option<&PathBuf> { + self.project_root.as_ref() + } + /// Initialize the service pub async fn initialize(&mut self) -> Result<(), ServiceError> { if self.initialized { From 70448de5496479444479373fe7b7283db595e2b0 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 22 Jul 2026 12:56:07 +0000 Subject: [PATCH 4/4] feat(cli): wire add/update/reindex onto the core Origin seams (Phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Edge injection (config.rs/context.rs): build + inject the embedding provider (from the API key; skip → reindex silent) and a RepositoryManager into the service before use. - reindex: thin wrapper over service.reindex(dir, observer); deletes the CLI's duplicate find/embed/index loop. (--force via cache-clear; --max-concurrent now a no-op — core reindex is sequential.) - update: preflight() per skill → add_from_origin(Update) when Updatable; else report the no-op reason. --check/--dry-run use preflight. - add: common path (single skill, project-level) → add_from_origin; --global and --recursive keep the pre-seam path (seam is project-level single-skill). - detect_skill_source → Origin, with the deferred fix: a URL ending in .zip is now Origin::ZipUrl (installs via add_from_origin) instead of mis-detected git (wiremock e2e test proves it). Known seam gaps, worked around non-destructively (follow-ups): groups not plumbed through add_from_origin (reapplied post-call); --global not seam-backed. Full workspace green: build + clippy -D warnings + 684 tests, 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/fastskill-cli/src/commands/add/mod.rs | 455 +++++++++- .../fastskill-cli/src/commands/add/sources.rs | 43 + crates/fastskill-cli/src/commands/reindex.rs | 793 ++++-------------- crates/fastskill-cli/src/commands/update.rs | 233 ++--- crates/fastskill-cli/src/config.rs | 33 +- crates/fastskill-cli/src/context.rs | 1 + crates/fastskill-cli/src/utils.rs | 39 +- .../fastskill-cli/src/utils/manifest_utils.rs | 58 ++ 8 files changed, 893 insertions(+), 762 deletions(-) diff --git a/crates/fastskill-cli/src/commands/add/mod.rs b/crates/fastskill-cli/src/commands/add/mod.rs index 93e9733..7c2bfe8 100644 --- a/crates/fastskill-cli/src/commands/add/mod.rs +++ b/crates/fastskill-cli/src/commands/add/mod.rs @@ -6,15 +6,18 @@ pub mod sources; // Re-export public API consumed by install_utils.rs use crate::error::{manifest_required_message, CliError, CliResult}; -use crate::utils::{detect_skill_source, SkillSource}; +use crate::utils::{detect_skill_source, validate_skill_structure, SkillSource}; use chrono::Utc; use clap::Args; use cli_framework::command::{FromArgValueMap, IntoCommandSpec}; use cli_framework::spec::arg_spec::{ArgKind, ArgSpec, ArgValueType, Cardinality}; use cli_framework::spec::command_tree::CommandSpec; use cli_framework::spec::value::ArgValue; -use fastskill_core::core::origin::Origin; +use fastskill_core::core::origin::{GitRef, Origin}; use fastskill_core::core::project::resolve_project_file; +use fastskill_core::core::repository::RepositoryManager; +use fastskill_core::core::version::VersionConstraint; +use fastskill_core::core::AddMode; use fastskill_core::{FastSkillService, SkillDefinition}; pub use install::copy_dir_recursive; pub use skill_def::create_skill_from_path; @@ -353,6 +356,84 @@ fn validate_folder_has_skill(path: &Path) -> CliResult<()> { ))) } +/// Build the install-intent [`Origin`] for the common (single-skill, +/// project-level) add path from the classified `source` and parsed args +/// (ADR-0005: `detect_skill_source` → `Origin`, feeding `add_from_origin`). +fn build_origin(source: &SkillSource, args: &AddArgs) -> CliResult { + match source { + SkillSource::ZipFile(path) => { + let canonical = path.canonicalize().map_err(|e| { + CliError::InvalidSource(format!( + "Failed to resolve absolute path for '{}': {}", + path.display(), + e + )) + })?; + Ok(Origin::Local { + path: canonical, + editable: false, + }) + } + SkillSource::Folder(path) => { + let canonical = path.canonicalize().map_err(|e| { + CliError::InvalidSource(format!( + "Failed to resolve absolute path for '{}': {}", + path.display(), + e + )) + })?; + Ok(Origin::Local { + path: canonical, + editable: args.editable, + }) + } + SkillSource::GitUrl(url) => { + let git_info = crate::utils::parse_git_url(url)?; + let branch = args.branch.clone().or_else(|| git_info.branch.clone()); + let r#ref = match (&branch, &args.tag) { + (Some(b), _) => GitRef::Branch(b.clone()), + (None, Some(t)) => GitRef::Tag(t.clone()), + (None, None) => GitRef::Default, + }; + Ok(Origin::Git { + url: url.clone(), + r#ref, + subdir: git_info.subdir, + }) + } + SkillSource::RemoteZipUrl(url) => Ok(Origin::ZipUrl { url: url.clone() }), + SkillSource::SkillId(skill_id_input) => { + let (skill_id_full, _scope, _expected_id, version_opt) = + sources::parse_registry_scope_id(skill_id_input)?; + let version = version_opt + .as_deref() + .map(VersionConstraint::parse) + .transpose() + .map_err(|e| CliError::Config(format!("Invalid version constraint: {}", e)))?; + + // Resolve "default" to the concrete configured repository's name now + // (mirrors the pre-seam `add_from_registry`), so a missing repository + // config fails fast with a clear message rather than surfacing later + // from inside the seam. + let repositories = crate::config::load_repositories_from_project()?; + let repo_manager = RepositoryManager::from_definitions(repositories); + let default_repo = repo_manager.get_default_repository().ok_or_else(|| { + CliError::Config( + "No default repository configured. Use 'fastskill repos add' to add a \ + repository." + .to_string(), + ) + })?; + + Ok(Origin::Repository { + repo: default_repo.name.clone(), + skill: skill_id_full, + version, + }) + } + } +} + pub async fn execute_add(service: &FastSkillService, args: AddArgs, global: bool) -> CliResult<()> { if args.reindex && args.no_reindex { return Err(CliError::Validation( @@ -367,7 +448,10 @@ pub async fn execute_add(service: &FastSkillService, args: AddArgs, global: bool if args.editable { match &source { SkillSource::Folder(_) => {} - SkillSource::ZipFile(_) | SkillSource::GitUrl(_) | SkillSource::SkillId(_) => { + SkillSource::ZipFile(_) + | SkillSource::GitUrl(_) + | SkillSource::RemoteZipUrl(_) + | SkillSource::SkillId(_) => { return Err(CliError::Config( "--editable (-e) is only supported for local folder sources. \ Remove -e or use a local path." @@ -380,38 +464,133 @@ pub async fn execute_add(service: &FastSkillService, args: AddArgs, global: bool if !global { ensure_manifest()?; } - let groups = args.group.clone().map(|g| vec![g]).unwrap_or_default(); - let ctx = AddContext { - service, - force: args.force, - editable: args.editable, - groups, - global, - }; - if args.recursive { - let path = match &source { - SkillSource::Folder(p) => p, - _ => { - return Err(CliError::Config( - "Recursive add is only valid when source is a local directory".to_string(), - )); - } + // `--global` and `--recursive` are not (yet) expressible through the core + // `add_from_origin` seam — it is single-skill + project-level only (no + // global-lock concept, no directory-of-skills fan-out) — so both keep the + // pre-seam per-source-type install path below. + if global || args.recursive { + let groups = args.group.clone().map(|g| vec![g]).unwrap_or_default(); + let ctx = AddContext { + service, + force: args.force, + editable: args.editable, + groups, + global, }; - return install::handle_recursive_add(&ctx, path).await; - } - match source { - SkillSource::ZipFile(path) => sources::add_from_zip(&ctx, &path).await?, - SkillSource::Folder(path) => { - validate_folder_has_skill(&path)?; - sources::add_from_folder(&ctx, &path).await? + if args.recursive { + let path = match &source { + SkillSource::Folder(p) => p, + _ => { + return Err(CliError::Config( + "Recursive add is only valid when source is a local directory".to_string(), + )); + } + }; + install::handle_recursive_add(&ctx, path).await?; + } else { + match source { + SkillSource::ZipFile(path) => sources::add_from_zip(&ctx, &path).await?, + SkillSource::Folder(path) => { + validate_folder_has_skill(&path)?; + sources::add_from_folder(&ctx, &path).await? + } + SkillSource::GitUrl(url) => { + sources::add_from_git(&ctx, &url, args.branch.as_deref(), args.tag.as_deref()) + .await? + } + SkillSource::RemoteZipUrl(url) => sources::add_from_zip_url(&ctx, &url).await?, + SkillSource::SkillId(skill_id) => { + sources::add_from_registry(&ctx, &skill_id).await? + } + } } - SkillSource::GitUrl(url) => { - sources::add_from_git(&ctx, &url, args.branch.as_deref(), args.tag.as_deref()).await? + + let auto_reindex = crate::config_file::load_auto_reindex_config(); + return crate::utils::reindex_utils::maybe_auto_reindex( + service, + "add", + reindex, + no_reindex, + auto_reindex, + false, + ) + .await; + } + + // Common path: single skill, project-level → core install seam (ADR-0005). + // Local-folder sources keep the CLI's own pre-checks (the "did you mean + // --recursive?" hint and the StandardValidator compatibility warnings) since + // core's own `validate_cloned_skill` is a narrower, warning-free check. + if let SkillSource::Folder(path) = &source { + validate_folder_has_skill(path)?; + validate_skill_structure(path)?; + } + + let origin = build_origin(&source, &args)?; + let mode = if args.force { + AddMode::Update + } else { + AddMode::Fresh + }; + let outcome = service + .add_from_origin(origin, mode) + .await + .map_err(CliError::Service)?; + + // Core-seam gap: `add_from_origin`'s manifest/lock upsert has no `groups` + // parameter (always writes `groups: None` / `Vec::new()`), so `--group` is + // reapplied here, same as the update path. + if let Some(group) = &args.group { + let current_dir = env::current_dir() + .map_err(|e| CliError::Config(format!("Failed to get current directory: {}", e)))?; + let project_file_result = resolve_project_file(¤t_dir); + let lock_path = project_file_result + .path + .parent() + .map(|p| p.join("skills.lock")) + .unwrap_or_else(|| PathBuf::from("skills.lock")); + if let Err(e) = crate::utils::manifest_utils::reapply_groups_after_seam( + &project_file_result.path, + &lock_path, + &outcome.id, + vec![group.clone()], + ) { + eprintln!( + "{}", + crate::utils::messages::error(&format!( + "Added {} but failed to record its group: {}", + outcome.id, e + )) + ); } - SkillSource::SkillId(skill_id) => sources::add_from_registry(&ctx, &skill_id).await?, } + + // `AddOutcome` only carries the skill `id`, not its display `name`; look the + // freshly-registered skill back up for a nicer message, falling back to the + // id (which is always a valid, if less friendly, thing to print). + let display_name = match fastskill_core::SkillId::new(outcome.id.clone()) { + Ok(id) => service + .skill_manager() + .get_skill(&id) + .await + .ok() + .flatten() + .map(|s| s.name) + .unwrap_or_else(|| outcome.id.clone()), + Err(_) => outcome.id.clone(), + }; + + println!( + "Successfully added skill: {} (v{})", + display_name, outcome.resolved.version + ); + println!( + "{}", + crate::utils::messages::ok("Updated skill-project.toml and skills.lock") + ); + let auto_reindex = crate::config_file::load_auto_reindex_config(); crate::utils::reindex_utils::maybe_auto_reindex( service, @@ -530,4 +709,222 @@ mod tests { panic!("Expected Config error"); } } + + // ── Common path (project, non-recursive) → core `add_from_origin` seam ──── + + fn write_valid_skill_md(dir: &std::path::Path, name: &str) { + std::fs::create_dir_all(dir).unwrap(); + std::fs::write( + dir.join("SKILL.md"), + format!( + "---\nname: {name}\nversion: \"1.0.0\"\ndescription: A test skill\n---\nBody\n" + ), + ) + .unwrap(); + } + + struct DirGuard(Option); + impl Drop for DirGuard { + fn drop(&mut self) { + if let Some(dir) = &self.0 { + let _ = std::env::set_current_dir(dir); + } + } + } + + fn setup_common_path_project() -> (TempDir, DirGuard, PathBuf) { + let tmp = TempDir::new().unwrap(); + let guard = DirGuard(std::env::current_dir().ok()); + std::env::set_current_dir(tmp.path()).unwrap(); + std::fs::write( + tmp.path().join("skill-project.toml"), + "[tool.fastskill]\nskills_directory = \".claude/skills\"\n\n[dependencies]\n", + ) + .unwrap(); + let skills_dir = tmp.path().join(".claude/skills"); + std::fs::create_dir_all(&skills_dir).unwrap(); + (tmp, guard, skills_dir) + } + + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_execute_add_local_folder_common_path_end_to_end() { + let _lock = fastskill_core::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (tmp, _guard, skills_dir) = setup_common_path_project(); + + let src = tmp.path().join("src-skill"); + write_valid_skill_md(&src, "common-path-skill"); + + let config = ServiceConfig { + skill_storage_path: skills_dir.clone(), + ..Default::default() + }; + let mut service = FastSkillService::new(config).await.unwrap(); + service.initialize().await.unwrap(); + + let args = AddArgs { + source: src.display().to_string(), + source_type: None, + branch: None, + tag: None, + force: false, + editable: false, + group: Some("dev".to_string()), + recursive: false, + reindex: false, + no_reindex: false, + }; + + let result = execute_add(&service, args, false).await; + assert!(result.is_ok(), "add should succeed: {:?}", result); + assert!(skills_dir.join("common-path-skill/SKILL.md").exists()); + + // --group was reapplied onto both the manifest dependency and the lock + // entry (core-seam gap workaround: add_from_origin's own upsert has no + // groups parameter). + let mut project = fastskill_core::core::manifest::SkillProjectToml::load_from_file( + &tmp.path().join("skill-project.toml"), + ) + .unwrap(); + let dep = project + .dependencies + .as_mut() + .unwrap() + .dependencies + .remove("common-path-skill") + .unwrap(); + match dep { + fastskill_core::core::manifest::DependencySpec::Inline { groups, .. } => { + assert_eq!(groups, Some(vec!["dev".to_string()])); + } + other => panic!("expected Inline dependency spec, got {:?}", other), + } + + let lock = fastskill_core::core::lock::ProjectSkillsLock::load_from_file( + &tmp.path().join("skills.lock"), + ) + .unwrap(); + let entry = lock + .skills + .iter() + .find(|s| s.id == "common-path-skill") + .unwrap(); + assert_eq!(entry.groups, vec!["dev".to_string()]); + } + + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_execute_add_force_overwrites_via_common_path() { + let _lock = fastskill_core::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (tmp, _guard, skills_dir) = setup_common_path_project(); + + let src = tmp.path().join("src-skill"); + write_valid_skill_md(&src, "force-skill"); + + let config = ServiceConfig { + skill_storage_path: skills_dir.clone(), + ..Default::default() + }; + let mut service = FastSkillService::new(config).await.unwrap(); + service.initialize().await.unwrap(); + + let make_args = |force: bool| AddArgs { + source: src.display().to_string(), + source_type: None, + branch: None, + tag: None, + force, + editable: false, + group: None, + recursive: false, + reindex: false, + no_reindex: false, + }; + + execute_add(&service, make_args(false), false) + .await + .expect("first add should succeed"); + + // Without --force, re-adding the same skill must fail (AddMode::Fresh -> + // AlreadyIndexed). + let err = execute_add(&service, make_args(false), false).await; + assert!(err.is_err(), "re-add without --force must fail"); + + // With --force, it must succeed (mapped onto AddMode::Update). + execute_add(&service, make_args(true), false) + .await + .expect("re-add with --force should succeed"); + } + + // ── ADR-0005 zip-url fix: a URL ending in `.zip` now installs via + // Origin::ZipUrl instead of being misclassified as a git URL. ───────────── + + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_execute_add_remote_zip_url_common_path_end_to_end() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let _lock = fastskill_core::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let (_tmp, _guard, skills_dir) = setup_common_path_project(); + + let server = MockServer::start().await; + let zip_bytes = { + use std::io::Write; + use zip::write::FileOptions; + let mut buf = Vec::new(); + { + let cursor = std::io::Cursor::new(&mut buf); + let mut writer = zip::ZipWriter::new(cursor); + let opts = + FileOptions::default().compression_method(zip::CompressionMethod::Stored); + writer.start_file("zip-url-skill/SKILL.md", opts).unwrap(); + writer + .write_all( + b"---\nname: zip-url-skill\nversion: \"1.0.0\"\ndescription: d\n---\nBody\n", + ) + .unwrap(); + writer.finish().unwrap(); + } + buf + }; + Mock::given(method("GET")) + .and(path("/pkg.zip")) + .respond_with(ResponseTemplate::new(200).set_body_bytes(zip_bytes)) + .mount(&server) + .await; + + let config = ServiceConfig { + skill_storage_path: skills_dir.clone(), + ..Default::default() + }; + let mut service = FastSkillService::new(config).await.unwrap(); + service.initialize().await.unwrap(); + + let args = AddArgs { + source: format!("{}/pkg.zip", server.uri()), + source_type: None, + branch: None, + tag: None, + force: false, + editable: false, + group: None, + recursive: false, + reindex: false, + no_reindex: false, + }; + + // Before the fix, `detect_skill_source` classified this as `GitUrl` and + // the add would fail with a git-clone error; it must now install + // successfully via `Origin::ZipUrl`. + let result = execute_add(&service, args, false).await; + assert!(result.is_ok(), "zip-url add should succeed: {:?}", result); + assert!(skills_dir.join("zip-url-skill/SKILL.md").exists()); + } } diff --git a/crates/fastskill-cli/src/commands/add/sources.rs b/crates/fastskill-cli/src/commands/add/sources.rs index 3db6d1d..fd26c5d 100644 --- a/crates/fastskill-cli/src/commands/add/sources.rs +++ b/crates/fastskill-cli/src/commands/add/sources.rs @@ -75,6 +75,49 @@ pub(super) async fn add_from_zip(ctx: &super::AddContext<'_>, zip_path: &Path) - super::install::install_via_download(ctx, &skill_path, skill_def, target).await } +/// Add a skill from a remote `.zip` archive URL (the `--global`/`--recursive` +/// path; the project-level common path installs this via `Origin::ZipUrl` + +/// `add_from_origin` instead). Mirrors `add_from_zip`, but downloads the +/// archive first instead of reading it from a local path. +pub(super) async fn add_from_zip_url(ctx: &super::AddContext<'_>, url: &str) -> CliResult<()> { + info!("Adding skill from remote zip URL: {}", url); + let response = reqwest::get(url) + .await + .map_err(|e| CliError::InvalidSource(format!("Failed to download '{}': {}", url, e)))? + .error_for_status() + .map_err(|e| CliError::InvalidSource(format!("Failed to download '{}': {}", url, e)))?; + let bytes = response + .bytes() + .await + .map_err(|e| CliError::InvalidSource(format!("Failed to read '{}': {}", url, e)))?; + + let temp_dir = TempDir::new().map_err(CliError::Io)?; + let zip_path = temp_dir.path().join("package.zip"); + let extract_path = temp_dir.path().join("extracted"); + fs::write(&zip_path, &bytes).map_err(CliError::Io)?; + fs::create_dir_all(&extract_path).map_err(CliError::Io)?; + extract_zip(&zip_path, &extract_path)?; + + let skill_path = find_skill_in_directory(&extract_path)?; + validate_skill_structure(&skill_path)?; + let origin = Origin::ZipUrl { + url: url.to_string(), + }; + let skill_def = + super::skill_def::create_skill_from_path(&skill_path, origin.clone(), "zip-url", false)?; + let version = skill_def.version.clone(); + let target = super::InstallTarget { + storage_dir: ctx + .service + .config() + .skill_storage_path + .join(skill_def.id.as_str()), + meta: super::SourceMeta { origin }, + version_display: version, + }; + super::install::install_via_download(ctx, &skill_path, skill_def, target).await +} + pub(super) async fn add_from_folder( ctx: &super::AddContext<'_>, folder_path: &Path, diff --git a/crates/fastskill-cli/src/commands/reindex.rs b/crates/fastskill-cli/src/commands/reindex.rs index 36caefa..68b99e6 100644 --- a/crates/fastskill-cli/src/commands/reindex.rs +++ b/crates/fastskill-cli/src/commands/reindex.rs @@ -1,20 +1,21 @@ -//! Reindex command implementation +//! Reindex command implementation — a thin wrapper over the core reindex seam +//! (`FastSkillService::reindex`, ADR-0002/0005). All indexing logic (finding +//! `SKILL.md` files, hashing, embedding, updating the vector index, pruning +//! stale entries) lives in core; this module only renders progress and the +//! final summary from the `ReindexProgress` observer callbacks and the +//! returned `ReindexOutcome`. use crate::error::{CliError, CliResult}; use cli_framework::command::{FromArgValueMap, IntoCommandSpec}; use cli_framework::spec::arg_spec::{ArgKind, ArgSpec, ArgValueType, Cardinality}; use cli_framework::spec::command_tree::CommandSpec; use cli_framework::spec::value::ArgValue; -use fastskill_core::{FastSkillService, VectorIndexService}; -use sha2::{Digest, Sha256}; +use fastskill_core::core::reindex::ReindexProgress; +use fastskill_core::FastSkillService; use std::collections::HashMap; -use std::fs; use std::io::{IsTerminal, Write}; -use std::path::Path; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::Arc; -use tokio::sync::Mutex; -use tokio::sync::Semaphore; -use tracing::{info, warn}; /// Check if progress bars should be shown fn should_show_progress() -> bool { @@ -27,17 +28,6 @@ fn should_show_progress() -> bool { std::io::stdout().is_terminal() } -/// Check if colors should be used -#[allow(dead_code)] -fn should_use_colors() -> bool { - // NO_COLOR takes precedence over TTY detection - if std::env::var("NO_COLOR").is_ok() { - return false; - } - - std::io::stdout().is_terminal() -} - /// Controls how reindex progress is displayed. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum ProgressMode { @@ -73,6 +63,11 @@ pub struct ReindexArgs { pub force: bool, /// Maximum number of concurrent embedding requests + /// + /// NOTE (core-seam gap): the core `reindex` seam processes skills + /// sequentially and takes no concurrency parameter, so this is currently + /// accepted but has no effect. Kept for CLI/arg compatibility. + #[allow(dead_code)] pub max_concurrent: usize, /// Show progress bars and processing details @@ -177,111 +172,22 @@ impl FromArgValueMap for ReindexArgs { } } -#[derive(Debug)] -enum ProcessOutcome { - Indexed(String), - Skipped(String, String), - Failed(String, String), -} - -impl ProcessOutcome { - #[allow(dead_code)] - fn skill_id(&self) -> &str { - match self { - ProcessOutcome::Indexed(id) => id, - ProcessOutcome::Skipped(id, _) => id, - ProcessOutcome::Failed(id, _) => id, - } - } -} - -struct ProgressTracker { - total: usize, - completed: usize, - skipped: usize, - failed: usize, - start_time: std::time::Instant, - mode: ProgressMode, -} - -impl ProgressTracker { - fn new(total: usize, progress: bool, no_progress: bool) -> Self { - let mode = ProgressMode::from_flags(progress, no_progress); - - Self { - total, - completed: 0, - skipped: 0, - failed: 0, - start_time: std::time::Instant::now(), - mode, - } - } - - fn record_success(&mut self, skill_id: &str) { - self.completed += 1; - if self.mode == ProgressMode::Verbose { - println!(" ✓ Indexed: {}", skill_id); - } - self.print_progress(); - } - - fn record_skip(&mut self, skill_id: &str, reason: &str) { - self.skipped += 1; - if self.mode == ProgressMode::Verbose { - println!(" ⊘ Skipped: {} ({})", skill_id, reason); - } - self.print_progress(); - } - - fn record_failure(&mut self, skill_id: &str, error: &str) { - self.failed += 1; - if self.mode == ProgressMode::Verbose { - eprintln!(" ✗ Failed: {} - {}", skill_id, error); - } - self.print_progress(); - } - - fn print_progress(&self) { - if self.mode != ProgressMode::Live { - return; - } - - let elapsed = self.start_time.elapsed().as_secs(); - let processed = self.completed + self.skipped + self.failed; - let progress = if self.total > 0 { - (processed as f32 / self.total as f32 * 100.0) as usize - } else { - 0 - }; - let pending = self.total.saturating_sub(processed); - - print!( - "\r\x1B[KProgress: [{}/{}] ({}%) | Pending: {} | Skipped: {} | Failed: {} | Elapsed: {}s", - processed, self.total, progress, pending, self.skipped, self.failed, elapsed - ); - - let _ = std::io::stdout().flush(); - } - - fn finish(&self) { - if self.mode == ProgressMode::Quiet { - return; - } - - if self.mode == ProgressMode::Live { - println!(); - } - - println!("Reindex completed"); - println!(" Total skills: {}", self.total); - println!(" Successfully indexed: {}", self.completed); - println!(" Skipped (unchanged): {}", self.skipped); - println!(" Failed: {}", self.failed); - println!( - " Total time: {:.2}s", - self.start_time.elapsed().as_secs_f64() - ); +/// Wipe every entry currently in the vector index so the next `reindex` call +/// re-embeds everything, regardless of unchanged file hashes. +/// +/// This is how `--force` is honored through the core seam: `FastSkillService::reindex` +/// has no `force` parameter (it always skips unchanged-hash skills), so instead of +/// duplicating its indexing loop here, we use the existing public +/// `VectorIndexService` accessor to clear the cache it consults. +async fn clear_vector_index(service: &FastSkillService) { + let Some(vector_index_service) = service.vector_index_service() else { + return; + }; + let Ok(all_indexed) = vector_index_service.get_all_skills().await else { + return; + }; + for indexed in all_indexed { + let _ = vector_index_service.remove_skill(&indexed.id).await; } } @@ -293,315 +199,90 @@ pub async fn execute_reindex(service: &FastSkillService, args: ReindexArgs) -> C )); } - // Check if embedding is configured - if service.config().embedding.is_none() { - println!("Reindex skipped: no embedding provider configured. Run 'fastskill doctor' for setup guidance."); - return Ok(()); - } - let embedding_config = service.config().embedding.as_ref().unwrap(); - - // Get OpenAI API key - let api_key = crate::config_file::get_openai_api_key()?; - - // Initialize embedding service - let embedding_service = std::sync::Arc::new( - fastskill_core::OpenAIEmbeddingService::from_config(embedding_config, api_key), - ); - - // Initialize vector index service - let vector_index_service = - std::sync::Arc::new(fastskill_core::VectorIndexServiceImpl::with_config( - embedding_config, - &service.config().skill_storage_path, - )); - - // Determine skills directory - let skills_dir = args - .skills_dir - .unwrap_or_else(|| service.config().skill_storage_path.clone()); - - info!( - "Reindexing vector index for skills directory: {}", - skills_dir.display() - ); - - // Find all SKILL.md files - let skill_files = find_skill_files(&skills_dir)?; - - if !args.no_progress { - println!("Found {} skills to process", skill_files.len()); - } - - if skill_files.is_empty() { - if !args.no_progress { - println!("No skills found in {}", skills_dir.display()); - } - return Ok(()); - } - - // Collect current skill IDs for cleanup later - let current_skill_ids: std::collections::HashSet = skill_files - .iter() - .filter_map(|skill_file| { - skill_file - .parent() - .and_then(|parent_dir| parent_dir.file_name()) - .map(|name| name.to_string_lossy().to_string()) - }) - .collect(); - - // Initialize progress tracker with Arc for thread-safe updates - let progress = Arc::new(Mutex::new(ProgressTracker::new( - skill_files.len(), - args.progress, - args.no_progress, - ))); - - // Process skills concurrently with semaphore for rate limiting - let semaphore = Arc::new(Semaphore::new(args.max_concurrent)); - let mut tasks = Vec::new(); - - for skill_file in skill_files { - let embedding_service = embedding_service.clone(); - let vector_index_service = vector_index_service.clone(); - let semaphore = semaphore.clone(); - let progress = progress.clone(); - let force = args.force; - - let task = tokio::spawn(async move { - // Acquire semaphore permit for rate limiting - let _permit = semaphore - .acquire() - .await - .map_err(|_| CliError::Config("Semaphore closed unexpectedly".to_string()))?; - - // Process the skill file - let outcome = process_skill_file( - skill_file, - &*embedding_service, - &*vector_index_service, - force, - ) - .await; - - // Update progress tracker based on outcome - let mut tracker = progress.lock().await; - match outcome { - Ok(ProcessOutcome::Indexed(skill_id)) => { - tracker.record_success(&skill_id); - } - Ok(ProcessOutcome::Skipped(skill_id, reason)) => { - tracker.record_skip(&skill_id, &reason); - } - Ok(ProcessOutcome::Failed(skill_id, error)) => { - tracker.record_failure(&skill_id, &error); - } - Err(e) => { - // Extract skill ID from error context if possible - let skill_id = "unknown"; - tracker.record_failure(skill_id, &e.to_string()); - } - } - - Ok::<(), CliError>(()) - }); - - tasks.push(task); - } - - // Wait for all tasks to complete - for task in tasks { - if let Err(e) = task.await { - warn!("Task panicked: {}", e); - } - } - - // Print final summary - let tracker = progress.lock().await; - tracker.finish(); - let error_count = tracker.failed; - drop(tracker); // Release lock before cleanup - - // Cleanup: Remove skills from index that are no longer on disk - let mut cleanup_removed_count = 0; - if let Ok(all_indexed_skills) = vector_index_service.get_all_skills().await { - for indexed_skill in all_indexed_skills { - if !current_skill_ids.contains(&indexed_skill.id) { - info!("Removing stale index entry: {}", indexed_skill.id); - match vector_index_service.remove_skill(&indexed_skill.id).await { - Ok(_) => { - cleanup_removed_count += 1; - } - Err(e) => { - warn!( - "Failed to remove stale index entry {}: {}", - indexed_skill.id, e - ); - } - } - } + let mode = ProgressMode::from_flags(args.progress, args.no_progress); + let start_time = std::time::Instant::now(); + + // `--force`: only meaningful when reindex will actually run (an embedding + // provider is injected); otherwise leave the index untouched, matching the + // "no embedding configured" skip below. + if args.force && service.embedding_service().is_some() { + clear_vector_index(service).await; + } + + // The observer fires once per skill `reindex` finds, before it decides + // whether that skill needs re-embedding. Its first call is also the first + // point at which the total skill count is known, so we print + // "Found N skills to process" there — same information as before, just + // learned from the seam's callback instead of a CLI-side directory scan. + let found_total = Arc::new(AtomicUsize::new(0)); + let seen_any = Arc::new(AtomicBool::new(false)); + let printed_found = Arc::new(AtomicBool::new(false)); + let no_progress = args.no_progress; + let verbose = mode == ProgressMode::Verbose; + let live = mode == ProgressMode::Live; + + let found_total_cb = Arc::clone(&found_total); + let seen_any_cb = Arc::clone(&seen_any); + let printed_found_cb = Arc::clone(&printed_found); + + let observer = move |p: ReindexProgress| { + seen_any_cb.store(true, Ordering::SeqCst); + found_total_cb.store(p.total, Ordering::SeqCst); + if !no_progress && !printed_found_cb.swap(true, Ordering::SeqCst) { + println!("Found {} skills to process", p.total); } - - if cleanup_removed_count > 0 && !args.no_progress { - println!("Removed {} stale entries from index", cleanup_removed_count); + if verbose { + println!(" Processing: {} ({}/{})", p.skill_id, p.current, p.total); + } else if live { + let pct = p + .current + .checked_mul(100) + .and_then(|n| n.checked_div(p.total)) + .unwrap_or(0); + print!("\r\x1B[KProgress: [{}/{}] ({}%)", p.current, p.total, pct); + let _ = std::io::stdout().flush(); } - } else { - warn!("Failed to retrieve all indexed skills for cleanup"); - } - - if error_count > 0 { - Err(CliError::Validation(format!( - "Reindex completed with {} errors", - error_count - ))) - } else { - Ok(()) - } -} + }; -/// Find all SKILL.md files in the skills directory -fn find_skill_files(skills_dir: &Path) -> CliResult> { - let mut skill_files = Vec::new(); + let outcome = service + .reindex(args.skills_dir.as_deref(), Some(&observer)) + .await + .map_err(CliError::Service)?; - if !skills_dir.exists() { - return Err(CliError::Config(format!( - "Skills directory does not exist: {}", - skills_dir.display() - ))); + if live && seen_any.load(Ordering::SeqCst) { + println!(); } - // Walk through all subdirectories looking for SKILL.md - for entry in walkdir::WalkDir::new(skills_dir) - .into_iter() - .filter_map(|e| e.ok()) - .filter(|e| e.file_type().is_file()) - .filter(|e| e.file_name() == "SKILL.md") - { - skill_files.push(entry.path().to_path_buf()); + if !outcome.reindexed { + let reason = outcome + .reason + .as_deref() + .unwrap_or("no embedding provider configured"); + println!("Reindex skipped: {reason}. Run 'fastskill doctor' for setup guidance."); + return Ok(()); } - Ok(skill_files) -} - -/// Process a single skill file -async fn process_skill_file( - skill_file: std::path::PathBuf, - embedding_service: &dyn fastskill_core::EmbeddingService, - vector_index_service: &dyn fastskill_core::VectorIndexService, - force: bool, -) -> CliResult { - let skill_dir = skill_file - .parent() - .ok_or_else(|| CliError::Config("Skill file has no parent directory".to_string()))?; - let skill_id = skill_dir - .file_name() - .and_then(|n| n.to_str()) - .ok_or_else(|| { - CliError::Validation(format!( - "Invalid skill directory name: {}", - skill_dir.display() - )) - })? - .to_string(); - - // Calculate file hash - let file_hash = calculate_file_hash(&skill_file).map_err(|e| { - CliError::Validation(format!("Failed to calculate hash for {}: {}", skill_id, e)) - })?; - - // Check if skill is already indexed and up to date - if !force { - if let Ok(Some(indexed_skill)) = vector_index_service.get_skill_by_id(&skill_id).await { - if indexed_skill.file_hash == file_hash { - return Ok(ProcessOutcome::Skipped( - skill_id.clone(), - "unchanged hash".to_string(), - )); - } + if !seen_any.load(Ordering::SeqCst) { + // Nothing under the skills directory at all. + if !no_progress { + let dir = args + .skills_dir + .clone() + .unwrap_or_else(|| service.config().skill_storage_path.clone()); + println!("Found 0 skills to process"); + println!("No skills found in {}", dir.display()); } + return Ok(()); } - info!("Processing skill: {}", skill_id); - - // Read and parse SKILL.md - let content = match fs::read_to_string(&skill_file) { - Ok(content) => content, - Err(e) => { - let error_msg = format!("Failed to read {}: {}", skill_file.display(), e); - return Ok(ProcessOutcome::Failed(skill_id, error_msg)); - } - }; - - // Extract frontmatter - let frontmatter = match fastskill_core::parse_yaml_frontmatter(&content) { - Ok(frontmatter) => frontmatter, - Err(e) => { - return Ok(ProcessOutcome::Failed( - skill_id, - format!("Failed to parse frontmatter: {}", e), - )); - } - }; - - // Convert frontmatter to JSON for storage - let frontmatter_json = match serde_json::to_value(&frontmatter) { - Ok(json) => json, - Err(e) => { - return Ok(ProcessOutcome::Failed( - skill_id, - format!("Failed to serialize frontmatter: {}", e), - )); - } - }; - - // Generate text for embedding - let embedding_text = format!("{}\n{}", frontmatter.name, frontmatter.description); - - // Generate embedding - let embedding = match embedding_service.embed_text(&embedding_text).await { - Ok(embedding) => embedding, - Err(e) => { - return Ok(ProcessOutcome::Failed( - skill_id, - format!("Failed to generate embedding: {}", e), - )); - } - }; - - // Add/update in vector index - match vector_index_service - .add_or_update_skill( - &skill_id, - skill_dir.to_path_buf(), - frontmatter_json, - embedding, - &file_hash, - ) - .await - { - Ok(_) => Ok(ProcessOutcome::Indexed(skill_id)), - Err(e) => Ok(ProcessOutcome::Failed( - skill_id, - format!("Failed to update index: {}", e), - )), + if mode != ProgressMode::Quiet { + println!("Reindex completed"); + println!(" Total skills: {}", found_total.load(Ordering::SeqCst)); + println!(" Indexed/updated: {}", outcome.count); + println!(" Total time: {:.2}s", start_time.elapsed().as_secs_f64()); } -} -/// Calculate SHA256 hash of a file -fn calculate_file_hash(file_path: &Path) -> CliResult { - let content = fs::read(file_path).map_err(|e| { - CliError::Validation(format!( - "Failed to read file {}: {}", - file_path.display(), - e - )) - })?; - - let mut hasher = Sha256::new(); - hasher.update(&content); - let hash_bytes = hasher.finalize(); - - Ok(format!("{:x}", hash_bytes)) + Ok(()) } #[cfg(test)] @@ -614,9 +295,9 @@ fn calculate_file_hash(file_path: &Path) -> CliResult { mod tests { use super::*; use fastskill_core::{EmbeddingConfig, ServiceConfig}; + use std::fs; use tempfile::TempDir; - // Helper function to create a test skill file fn create_test_skill( skills_dir: &std::path::Path, skill_id: &str, @@ -640,127 +321,10 @@ Test skill content"#, fs::write(skill_dir.join("SKILL.md"), skill_content).unwrap(); } - #[tokio::test] - async fn test_progress_tracker_verbose_mode() { - let tracker = ProgressTracker::new(5, true, false); - - assert_eq!(tracker.total, 5); - assert_eq!(tracker.completed, 0); - assert_eq!(tracker.mode, ProgressMode::Verbose); - } - - #[tokio::test] - async fn test_progress_tracker_quiet_mode() { - let tracker = ProgressTracker::new(5, false, true); - - assert_eq!(tracker.total, 5); - assert_eq!(tracker.mode, ProgressMode::Quiet); - } - - #[tokio::test] - async fn test_progress_tracker_record_outcomes() { - let mut tracker = ProgressTracker::new(10, false, false); - - tracker.record_success("skill1"); - assert_eq!(tracker.completed, 1); - - tracker.record_skip("skill2", "unchanged"); - assert_eq!(tracker.skipped, 1); - - tracker.record_failure("skill3", "parse error"); - assert_eq!(tracker.failed, 1); - - assert_eq!(tracker.completed + tracker.skipped + tracker.failed, 3); - } - - #[tokio::test] - async fn test_reindex_with_verbose_quiet_args() { - let temp_dir = TempDir::new().unwrap(); - let skills_dir = temp_dir.path().join(".claude/skills"); - fs::create_dir_all(&skills_dir).unwrap(); - - // Create test skills - create_test_skill( - &skills_dir, - "test-skill-1", - "Test Skill One", - "First test skill", - ); - create_test_skill( - &skills_dir, - "test-skill-2", - "Test Skill Two", - "Second test skill", - ); - - let config = ServiceConfig { - skill_storage_path: skills_dir.clone(), - embedding: Some(EmbeddingConfig { - openai_base_url: "https://api.openai.com/v1".to_string(), - embedding_model: "text-embedding-3-small".to_string(), - index_path: None, - }), - ..Default::default() - }; - - let mut service = FastSkillService::new(config).await.unwrap(); - service.initialize().await.unwrap(); - - // Test progress mode - let progress_args = ReindexArgs { - skills_dir: Some(skills_dir.clone()), - force: true, - max_concurrent: 2, - progress: true, - no_progress: false, - }; - - // May fail due to missing API key, but args should be accepted - let _ = execute_reindex(&service, progress_args).await; - - // Test no_progress mode - let no_progress_args = ReindexArgs { - skills_dir: Some(skills_dir), - force: true, - max_concurrent: 2, - progress: false, - no_progress: true, - }; - - let _ = execute_reindex(&service, no_progress_args).await; - } - - #[tokio::test] - async fn test_process_outcome_skill_id_access() { - let indexed = ProcessOutcome::Indexed("skill1".to_string()); - assert_eq!(indexed.skill_id(), "skill1"); - - let skipped = ProcessOutcome::Skipped("skill2".to_string(), "unchanged".to_string()); - assert_eq!(skipped.skill_id(), "skill2"); - - let failed = ProcessOutcome::Failed("skill3".to_string(), "error".to_string()); - assert_eq!(failed.skill_id(), "skill3"); - } - - #[tokio::test] - async fn test_should_show_progress_respects_environment() { - // Save original env var - let original = std::env::var("FASTSKILL_NO_PROGRESS").ok(); - - // Test with FASTSKILL_NO_PROGRESS set - std::env::set_var("FASTSKILL_NO_PROGRESS", "1"); - assert!(!should_show_progress()); - - // Test without FASTSKILL_NO_PROGRESS (result depends on TTY) - std::env::remove_var("FASTSKILL_NO_PROGRESS"); - // Don't assert specific value as it depends on test environment TTY - - // Restore original state - if let Some(val) = original { - std::env::set_var("FASTSKILL_NO_PROGRESS", val); - } else { - std::env::remove_var("FASTSKILL_NO_PROGRESS"); - } + #[test] + fn test_progress_mode_from_flags() { + assert_eq!(ProgressMode::from_flags(true, false), ProgressMode::Verbose); + assert_eq!(ProgressMode::from_flags(false, true), ProgressMode::Quiet); } #[tokio::test] @@ -801,7 +365,11 @@ Test skill content"#, ..Default::default() }; - let mut service = FastSkillService::new(config).await.unwrap(); + let mock_embedding = Arc::new(MockEmbeddingService); + let mut service = FastSkillService::new(config) + .await + .unwrap() + .with_embedding_service(mock_embedding); service.initialize().await.unwrap(); let nonexistent_dir = temp_dir.path().join("nonexistent"); @@ -814,16 +382,17 @@ Test skill content"#, }; let result = execute_reindex(&service, args).await; - // May fail for various reasons (nonexistent dir, API key, etc.) - // Just verify it handles the error gracefully - assert!(result.is_err() || result.is_ok()); + assert!(result.is_err()); } #[tokio::test] async fn test_execute_reindex_empty_skills_dir() { let temp_dir = TempDir::new().unwrap(); + let skills_dir = temp_dir.path().join("skills"); + fs::create_dir_all(&skills_dir).unwrap(); + let config = ServiceConfig { - skill_storage_path: temp_dir.path().to_path_buf(), + skill_storage_path: skills_dir.clone(), embedding: Some(EmbeddingConfig { openai_base_url: "https://api.openai.com/v1".to_string(), embedding_model: "text-embedding-3-small".to_string(), @@ -832,39 +401,40 @@ Test skill content"#, ..Default::default() }; - let mut service = FastSkillService::new(config).await.unwrap(); + let mock_embedding = Arc::new(MockEmbeddingService); + let mut service = FastSkillService::new(config) + .await + .unwrap() + .with_embedding_service(mock_embedding); service.initialize().await.unwrap(); let args = ReindexArgs { - skills_dir: None, + skills_dir: Some(skills_dir), force: true, max_concurrent: 5, progress: false, no_progress: false, }; - // Should succeed with empty directory (no skills to index) - // May fail due to missing API key, but should handle empty directory gracefully let result = execute_reindex(&service, args).await; - // Result may be Ok (empty directory) or Err (missing API key) - assert!(result.is_ok() || result.is_err()); + assert!( + result.is_ok(), + "empty directory should be a no-op success: {:?}", + result + ); } #[tokio::test] - async fn test_execute_reindex_with_force_and_max_concurrent() { + async fn test_execute_reindex_indexes_and_force_clears_cache() { let temp_dir = TempDir::new().unwrap(); let skills_dir = temp_dir.path().join(".claude/skills"); fs::create_dir_all(&skills_dir).unwrap(); - - let skill_dir = skills_dir.join("test-skill"); - fs::create_dir_all(&skill_dir).unwrap(); - let skill_content = r#"# Test Skill - -Name: test-skill -Version: 1.0.0 -Description: A test skill for coverage -"#; - fs::write(skill_dir.join("SKILL.md"), skill_content).unwrap(); + create_test_skill( + &skills_dir, + "test-skill-1", + "Test Skill One", + "First test skill", + ); let config = ServiceConfig { skill_storage_path: skills_dir.clone(), @@ -876,80 +446,71 @@ Description: A test skill for coverage ..Default::default() }; - let mut service = FastSkillService::new(config).await.unwrap(); + let mock_embedding = Arc::new(MockEmbeddingService); + let mut service = FastSkillService::new(config) + .await + .unwrap() + .with_embedding_service(mock_embedding); service.initialize().await.unwrap(); let args = ReindexArgs { + skills_dir: Some(skills_dir.clone()), + force: false, + max_concurrent: 2, + progress: true, + no_progress: false, + }; + let result = execute_reindex(&service, args).await; + assert!(result.is_ok(), "reindex should succeed: {:?}", result); + + // Re-running with --force must re-embed even though the hash is unchanged. + let force_args = ReindexArgs { skills_dir: Some(skills_dir), force: true, max_concurrent: 2, progress: false, - no_progress: false, + no_progress: true, }; - - let result = execute_reindex(&service, args).await; - // May fail due to missing API key, but should process the args correctly - assert!(result.is_ok() || result.is_err()); + let result = execute_reindex(&service, force_args).await; + assert!( + result.is_ok(), + "forced reindex should succeed: {:?}", + result + ); } #[tokio::test] - async fn test_reindex_cleanup_removes_stale_entries() { + async fn test_reindex_progress_conflict_errors() { let temp_dir = TempDir::new().unwrap(); - let skills_dir = temp_dir.path().join(".claude/skills"); - fs::create_dir_all(&skills_dir).unwrap(); - - let skill1_dir = skills_dir.join("skill-one"); - fs::create_dir_all(&skill1_dir).unwrap(); - let skill1_content = r#"# Skill One - -Name: skill-one -Version: 1.0.0 -Description: First skill -"#; - fs::write(skill1_dir.join("SKILL.md"), skill1_content).unwrap(); - - let skill2_dir = skills_dir.join("skill-two"); - fs::create_dir_all(&skill2_dir).unwrap(); - let skill2_content = r#"# Skill Two - -Name: skill-two -Version: 1.0.0 -Description: Second skill -"#; - fs::write(skill2_dir.join("SKILL.md"), skill2_content).unwrap(); - let config = ServiceConfig { - skill_storage_path: skills_dir.clone(), - embedding: Some(EmbeddingConfig { - openai_base_url: "https://api.openai.com/v1".to_string(), - embedding_model: "text-embedding-3-small".to_string(), - index_path: None, - }), + skill_storage_path: temp_dir.path().to_path_buf(), ..Default::default() }; - let mut service = FastSkillService::new(config).await.unwrap(); service.initialize().await.unwrap(); let args = ReindexArgs { - skills_dir: Some(skills_dir), - force: true, - max_concurrent: 2, - progress: false, - no_progress: false, + skills_dir: None, + force: false, + max_concurrent: 5, + progress: true, + no_progress: true, }; + let result = execute_reindex(&service, args).await; + assert!(matches!(result, Err(CliError::Validation(_)))); + } - // First reindex with both skills - let _ = execute_reindex(&service, args.clone()).await; - - // Manually delete skill-two directory - fs::remove_dir_all(&skill2_dir).unwrap(); + /// Deterministic, network-free embedding provider for tests. + struct MockEmbeddingService; - // Reindex again - should clean up stale entry - let _ = execute_reindex(&service, args).await; + #[async_trait::async_trait] + impl fastskill_core::EmbeddingService for MockEmbeddingService { + async fn embed_text(&self, text: &str) -> Result, fastskill_core::ServiceError> { + Ok(vec![text.len() as f32, 0.0, 0.0]) + } - // Verify that only skill-one remains (or both if cleanup failed) - // This is a best-effort test since we don't have full control over the index - assert!(true); + async fn embed_query(&self, query: &str) -> Result, fastskill_core::ServiceError> { + self.embed_text(query).await + } } } diff --git a/crates/fastskill-cli/src/commands/update.rs b/crates/fastskill-cli/src/commands/update.rs index e711072..6bd601d 100644 --- a/crates/fastskill-cli/src/commands/update.rs +++ b/crates/fastskill-cli/src/commands/update.rs @@ -2,25 +2,21 @@ use crate::config::create_service_config; use crate::error::{manifest_required_message, CliError, CliResult}; -use crate::utils::{install_utils, manifest_utils, messages}; +use crate::utils::{manifest_utils, messages}; use cli_framework::command::{FromArgValueMap, IntoCommandSpec}; use cli_framework::spec::arg_spec::{ArgKind, ArgSpec, ArgValueType, Cardinality}; use cli_framework::spec::command_tree::CommandSpec; use cli_framework::spec::value::ArgValue; use fastskill_core::core::{ - lock::{global_lock_path, GlobalSkillsLock, ProjectSkillsLock}, + lock::{global_lock_path, GlobalSkillsLock}, manifest::SkillProjectToml, project::resolve_project_file, - repository::RepositoryManager, - resolver::PackageResolver, - sources::SourcesManager, - update::{UpdateService, UpdateStrategy}, + AddMode, UpdatePreflight, }; use fastskill_core::FastSkillService; use std::collections::HashMap; use std::env; use std::path::PathBuf; -use std::sync::Arc; /// Update skills to latest versions (behavior matrix affects manifest, lock, and installed state) /// @@ -195,15 +191,17 @@ pub async fn execute_update(args: UpdateArgs, global: bool) -> CliResult<()> { let config = create_service_config(global, None)?; if let Ok(mut svc) = fastskill_core::FastSkillService::new(config).await { if svc.initialize().await.is_ok() { - let _ = crate::utils::reindex_utils::maybe_auto_reindex( - &svc, - "update", - reindex, - no_reindex, - auto_reindex_config, - false, - ) - .await; + if let Ok(svc) = crate::config::inject_edge_services(svc) { + let _ = crate::utils::reindex_utils::maybe_auto_reindex( + &svc, + "update", + reindex, + no_reindex, + auto_reindex_config, + false, + ) + .await; + } } } } @@ -288,6 +286,21 @@ async fn execute_update_global(args: UpdateArgs) -> CliResult<()> { Ok(()) } +/// Updates skills recorded in the project's `skill-project.toml`/`skills.lock` by +/// routing each dependency's recorded `Origin` through the core install seam +/// (ADR-0005): `preflight(&origin)` decides whether there's anything to update +/// (`Updatable`/`UpToDate`/`Immutable { reason }`), and only `Updatable` entries +/// are re-fetched via `add_from_origin(origin, AddMode::Update)`. +/// +/// NOTE (core-seam gap / simplification): `--strategy`/`--version` are parsed and +/// validated for backward compatibility, but (as in the pre-seam CLI, where they +/// were likewise parsed yet never consulted by the actual, non-`--check`/`--dry-run` +/// update loop) they have no effect on which version is installed — the seam +/// always resolves each origin to what it considers current (newest allowed for a +/// registry/repository origin, a fresh re-fetch for git/local/zip-url). The +/// previous marketplace-aware `UpdateService::check_updates` version-diffing for +/// `--check`/`--dry-run` is likewise replaced by `preflight`'s coarser +/// Updatable/UpToDate/Immutable classification. async fn execute_update_project(args: UpdateArgs) -> CliResult<()> { println!("Updating skills..."); println!(); @@ -329,82 +342,64 @@ async fn execute_update_project(args: UpdateArgs) -> CliResult<()> { return Ok(()); } - // Parse update strategy - let strategy = match args.strategy.as_str() { - "latest" => UpdateStrategy::Latest, - "patch" => UpdateStrategy::Patch, - "minor" => UpdateStrategy::Minor, - "major" => UpdateStrategy::Major, + // Parse/validate the update strategy (kept for CLI compatibility; see the + // NOTE above — it has no bearing on the seam-based resolution below). + match args.strategy.as_str() { + "latest" | "patch" | "minor" | "major" => {} + _ if args.version.is_some() => {} _ => { - if let Some(version) = &args.version { - UpdateStrategy::Exact(version.clone()) - } else { - return Err(CliError::Config(format!( - "Invalid strategy: {}. Use: latest, patch, minor, major", - args.strategy - ))); - } + return Err(CliError::Config(format!( + "Invalid strategy: {}. Use: latest, patch, minor, major", + args.strategy + ))); } - }; - - // If version is specified, use Exact strategy - let strategy = if let Some(version) = &args.version { - UpdateStrategy::Exact(version.clone()) - } else { - strategy - }; + } - // T033: Load lock file from project root + // T033: Resolve the lock file path from the project root let lock_path = if let Some(parent) = project_file_path.parent() { parent.join("skills.lock") } else { PathBuf::from("skills.lock") }; - let lock = if lock_path.exists() { - ProjectSkillsLock::load_from_file(&lock_path) - .map_err(|e| CliError::Config(format!("Failed to load lock file: {}", e)))? - } else { + if !lock_path.exists() { return Err(CliError::Config( "skills.lock not found. Run 'fastskill install' first.".to_string(), )); - }; - - // Load repositories and create sources manager for marketplace-based repos - let repositories = crate::config::load_repositories_from_project()?; - let repo_manager = RepositoryManager::from_definitions(repositories); + } - // Create SourcesManager from marketplace-based repositories for PackageResolver - let sources_manager = create_sources_manager_from_repositories(&repo_manager)?; + // Initialize the (edge-injected) service: embedding provider + repository + // manager (ADR-0005). Needed even in --check/--dry-run mode, since + // `preflight` on an `Origin::Repository` entry consults the repository + // manager. + let config = create_service_config(false, None)?; + let mut service = FastSkillService::new(config) + .await + .map_err(CliError::Service)?; + service.initialize().await.map_err(CliError::Service)?; + let service = crate::config::inject_edge_services(service)?; if args.check || args.dry_run { - if let Some(sources_mgr) = sources_manager { - let mut resolver = PackageResolver::new(sources_mgr.clone()); - resolver - .build_index() - .await - .map_err(|e| CliError::Config(format!("Failed to build resolver index: {}", e)))?; - - let update_service = UpdateService::new(Arc::new(resolver), lock); - let updates = update_service - .check_updates(args.skill_id.as_deref(), strategy) - .map_err(|e| CliError::Config(format!("Failed to check updates: {}", e)))?; - - if updates.is_empty() { - println!("{}", messages::info("No updates available")); - } else { - println!("\nSkills that would be updated:\n"); - for update in &updates { - println!( - " • {}: {} -> {}", - update.skill_id, update.current_version, update.available_version - ); + println!("\nSkills that would be updated:\n"); + let mut any_reported = false; + for entry in &entries { + any_reported = true; + match service.preflight(&entry.origin).await { + Ok(UpdatePreflight::Updatable) => { + println!(" • {} (from {:?})", entry.id, entry.origin); + } + Ok(UpdatePreflight::UpToDate) => { + println!(" • {} is already up to date", entry.id); + } + Ok(UpdatePreflight::Immutable { reason }) => { + println!(" • {} is immutable: {}", entry.id, reason); + } + Err(e) => { + println!(" • {}: {}", entry.id, e); } } - } else { - println!("\nSkills that would be updated:\n"); - for entry in &entries { - println!(" • {} (from {:?})", entry.id, entry.origin); - } + } + if !any_reported { + println!("{}", messages::info("No updates available")); } if args.check { println!( @@ -415,42 +410,62 @@ async fn execute_update_project(args: UpdateArgs) -> CliResult<()> { return Ok(()); } - // Initialize service - // Note: update command doesn't have access to CLI sources_path, so uses env var or walk-up - let config = create_service_config(false, None)?; - let mut service = FastSkillService::new(config) - .await - .map_err(CliError::Service)?; - service.initialize().await.map_err(CliError::Service)?; - - // Load repositories and create sources manager for marketplace-based repos - let repositories = crate::config::load_repositories_from_project()?; - let repo_manager = RepositoryManager::from_definitions(repositories); - - // Create SourcesManager from marketplace-based repositories for PackageResolver - let sources_manager = create_sources_manager_from_repositories(&repo_manager)?; - - // Update each skill + // Update each skill: preflight first (an honest no-op with a reason for + // UpToDate/Immutable), then re-fetch only what's Updatable. let mut updated_count = 0; for entry in entries { println!(" Updating {}...", entry.id); - match install_utils::install_skill_from_entry( - &service, - entry.clone(), - sources_manager.as_ref().map(|arc| arc.as_ref()), - ) - .await - { - Ok(skill_def) => { - manifest_utils::update_lock_file(&lock_path, &skill_def, entry.groups.clone()) - .map_err(|e| CliError::Config(format!("Failed to update lock file: {}", e)))?; - updated_count += 1; - println!(" {}", messages::ok(&format!("Updated {}", entry.id))); + match service.preflight(&entry.origin).await { + Ok(UpdatePreflight::Updatable) => { + match service + .add_from_origin(entry.origin.clone(), AddMode::Update) + .await + { + Ok(outcome) => { + // Core-seam gap workaround: `add_from_origin`'s manifest/lock + // upsert has no `groups` parameter (see + // `reapply_groups_after_seam`'s doc comment). + if let Err(e) = manifest_utils::reapply_groups_after_seam( + &project_file_path, + &lock_path, + &outcome.id, + entry.groups.clone(), + ) { + eprintln!( + " {}", + messages::error(&format!( + "Updated {} but failed to reapply groups: {}", + entry.id, e + )) + ); + } + updated_count += 1; + println!(" {}", messages::ok(&format!("Updated {}", entry.id))); + } + Err(e) => { + eprintln!( + " {}", + messages::error(&format!("Failed to update {}: {}", entry.id, e)) + ); + } + } + } + Ok(UpdatePreflight::UpToDate) => { + println!( + " {}", + messages::info(&format!("{} is already up to date", entry.id)) + ); + } + Ok(UpdatePreflight::Immutable { reason }) => { + println!( + " {}", + messages::info(&format!("{} is immutable: {}", entry.id, reason)) + ); } Err(e) => { eprintln!( " {}", - messages::error(&format!("Failed to update {}: {}", entry.id, e)) + messages::error(&format!("Failed to check {}: {}", entry.id, e)) ); } } @@ -466,14 +481,6 @@ async fn execute_update_project(args: UpdateArgs) -> CliResult<()> { Ok(()) } -/// Create SourcesManager from RepositoryManager for marketplace-based repositories -pub fn create_sources_manager_from_repositories( - repo_manager: &RepositoryManager, -) -> CliResult>> { - install_utils::create_sources_manager_from_repositories(repo_manager) - .map(|opt| opt.map(Arc::new)) -} - #[cfg(test)] #[allow( clippy::unwrap_used, diff --git a/crates/fastskill-cli/src/config.rs b/crates/fastskill-cli/src/config.rs index 8821474..3133c50 100644 --- a/crates/fastskill-cli/src/config.rs +++ b/crates/fastskill-cli/src/config.rs @@ -3,11 +3,12 @@ use crate::error::{CliError, CliResult}; use fastskill_core::core::manifest::SkillProjectToml; use fastskill_core::core::project; -use fastskill_core::core::repository::RepositoryDefinition; +use fastskill_core::core::repository::{RepositoryDefinition, RepositoryManager}; use fastskill_core::core::service::HttpServerConfig; -use fastskill_core::ServiceConfig; +use fastskill_core::{FastSkillService, ServiceConfig}; use std::env; use std::path::PathBuf; +use std::sync::Arc; use tracing::debug; /// Load repositories from skill-project.toml [tool.fastskill.repositories] @@ -213,6 +214,34 @@ pub fn create_service_config( }) } +/// Inject the CLI-edge dependencies the core install/reindex seams need (ADR-0005): +/// an embedding provider (only ever constructed here, where the API key is loaded) +/// and a repository manager (resolved from the project's +/// `[tool.fastskill.repositories]`). Core never loads the key or reads the repos +/// config itself — only the edge (CLI/serve) does. +/// +/// Mirrors the construction `add_from_registry` already did inline: load +/// repositories → `RepositoryManager::from_definitions`. When no `OPENAI_API_KEY` +/// is set, the embedding provider is simply left unset; `reindex` skips silently +/// in that case (ADR-0002/0005) rather than erroring. +pub fn inject_edge_services(mut service: FastSkillService) -> CliResult { + if let Some(embedding_config) = service.config().embedding.clone() { + if let Ok(api_key) = crate::config_file::get_openai_api_key() { + let embedding_service = Arc::new(fastskill_core::OpenAIEmbeddingService::from_config( + &embedding_config, + api_key, + )); + service = service.with_embedding_service(embedding_service); + } + } + + let repositories = load_repositories_from_project()?; + let repo_manager = RepositoryManager::from_definitions(repositories); + service = service.with_repository_manager(Arc::new(repo_manager)); + + Ok(service) +} + /// Load HTTP server configuration from skill-project.toml [tool.fastskill.server] pub fn load_server_config() -> CliResult> { let current_dir = env::current_dir() diff --git a/crates/fastskill-cli/src/context.rs b/crates/fastskill-cli/src/context.rs index 1339e6c..cdb2259 100644 --- a/crates/fastskill-cli/src/context.rs +++ b/crates/fastskill-cli/src/context.rs @@ -50,6 +50,7 @@ impl FsState { .await .map_err(CliError::Service)?; s.initialize().await.map_err(CliError::Service)?; + let s = crate::config::inject_edge_services(s)?; Ok(Arc::new(s)) }) .await diff --git a/crates/fastskill-cli/src/utils.rs b/crates/fastskill-cli/src/utils.rs index 7d8b026..a287546 100644 --- a/crates/fastskill-cli/src/utils.rs +++ b/crates/fastskill-cli/src/utils.rs @@ -68,7 +68,7 @@ pub fn parse_skill_id(input: &str) -> (String, Option) { } } -/// Detect the type of skill source (zip, folder, git URL, or skill ID) +/// Detect the type of skill source (zip, folder, git URL, remote zip URL, or skill ID) pub fn detect_skill_source(path: &str) -> SkillSource { // Check if it's a skill ID first if is_skill_id(path) { @@ -78,6 +78,14 @@ pub fn detect_skill_source(path: &str) -> SkillSource { // Check if it's a URL (git or http) if let Ok(url) = Url::parse(path) { if url.scheme() == "git" || url.scheme() == "https" || url.scheme() == "http" { + // A URL whose path ends in `.zip` is a remote archive to download, not + // a git repository to clone. Previously this was misclassified as + // `GitUrl` (a `.zip`-URL add would fail with a git-clone error) — fixed + // as part of the ADR-0005 Origin/add_from_origin seam, which now + // supports `Origin::ZipUrl` natively. + if url.path().to_ascii_lowercase().ends_with(".zip") { + return SkillSource::RemoteZipUrl(path.to_string()); + } return SkillSource::GitUrl(path.to_string()); } } @@ -96,10 +104,16 @@ pub fn detect_skill_source(path: &str) -> SkillSource { /// Represents different skill source types #[derive(Debug, Clone)] pub enum SkillSource { + /// A `.zip` archive on the local filesystem. ZipFile(PathBuf), + /// A directory on the local filesystem. Folder(PathBuf), + /// A git repository URL (http/https/git scheme, not ending in `.zip`). GitUrl(String), - SkillId(String), // Format: skillid@version or skillid + /// An http(s) URL to a remote `.zip` archive (downloaded, not cloned). + RemoteZipUrl(String), + /// Format: `skillid@version`, `skillid`, `scope/skillid`, or `scope/skillid@version`. + SkillId(String), } /// Parse git URL to extract repository information @@ -306,6 +320,27 @@ mod tests { } } + #[test] + fn test_detect_skill_source_remote_zip_url() { + // ADR-0005 fix: a URL ending in `.zip` must not be classified as git + // (previously misclassified as `GitUrl`, causing a spurious git-clone + // failure instead of a zip download). + match detect_skill_source("https://example.com/skills/bundle.zip") { + SkillSource::RemoteZipUrl(url) => { + assert_eq!(url, "https://example.com/skills/bundle.zip") + } + other => panic!("expected RemoteZipUrl, got {:?}", other), + } + } + + #[test] + fn test_detect_skill_source_zip_url_query_string_still_detected() { + match detect_skill_source("https://example.com/download.zip?token=abc") { + SkillSource::RemoteZipUrl(_) => {} + other => panic!("expected RemoteZipUrl, got {:?}", other), + } + } + #[test] fn test_detect_skill_source_folder() { match detect_skill_source("./some/folder") { diff --git a/crates/fastskill-cli/src/utils/manifest_utils.rs b/crates/fastskill-cli/src/utils/manifest_utils.rs index fec7ee7..0980a43 100644 --- a/crates/fastskill-cli/src/utils/manifest_utils.rs +++ b/crates/fastskill-cli/src/utils/manifest_utils.rs @@ -145,6 +145,64 @@ pub fn remove_from_global_lock_file(skill_id: &str) -> Result<(), Box, +) -> Result<(), Box> { + if groups.is_empty() { + return Ok(()); + } + + if project_file_path.exists() { + let mut project = SkillProjectToml::load_from_file(project_file_path) + .map_err(|e| format!("Failed to load skill-project.toml: {}", e))?; + let mut changed = false; + if let Some(deps) = project.dependencies.as_mut() { + if let Some(DependencySpec::Inline { groups: g, .. }) = + deps.dependencies.get_mut(skill_id) + { + *g = Some(groups.clone()); + changed = true; + } + } + if changed { + project + .save_to_file(project_file_path) + .map_err(|e| format!("Failed to save skill-project.toml: {}", e))?; + } + } + + if lock_path.exists() { + let sidecar = sidecar_path(lock_path); + let _guard = acquire_advisory_lock(&sidecar) + .map_err(|e| format!("Failed to acquire lock on skills.lock: {}", e))?; + + let mut lock = ProjectSkillsLock::load_from_file(lock_path) + .map_err(|e| format!("Failed to load lock file: {}", e))?; + if let Some(entry) = lock.skills.iter_mut().find(|s| s.id == skill_id) { + entry.groups = groups; + lock.save_to_file(lock_path) + .map_err(|e| format!("Failed to save lock file: {}", e))?; + } + + let _ = std::fs::remove_file(&sidecar); + } + + Ok(()) +} + /// T028: Add skill to skill-project.toml [dependencies] section. /// Fails if skill-project.toml is not found in the hierarchy (we never auto-create it). ///