diff --git a/crates/fastskill-cli/src/commands/add/origin.rs b/crates/fastskill-cli/src/commands/add/origin.rs index abb99989..6dc1606b 100644 --- a/crates/fastskill-cli/src/commands/add/origin.rs +++ b/crates/fastskill-cli/src/commands/add/origin.rs @@ -534,9 +534,8 @@ fn build_explicit_origin(source: &SkillSource, args: &AddArgs) -> CliResult CliResult> { + let Some((project, _)) = load_current_project()? else { + return Ok(Vec::new()); + }; + let repositories = project + .tool + .and_then(|t| t.fastskill) + .and_then(|f| f.repositories) + .unwrap_or_default(); + Ok(repositories + .into_iter() + .map(convert_repository_definition) + .collect()) +} + +/// Repositories that resolve dependency origins: the project's own plus those +/// of every Manifest it composes through [tool.fastskill.manifests], so a +/// composed repository origin finds the catalog its own Manifest names. +pub fn load_resolution_repositories() -> CliResult> { + let Some((project, project_path)) = load_current_project()? else { + return Ok(Vec::new()); + }; + let project_dir = project_path + .parent() + .unwrap_or_else(|| std::path::Path::new(".")); + Ok(project + .composed_repositories(project_dir) + .map_err(CliError::Config)? + .into_iter() + .map(convert_repository_definition) + .collect()) +} + +/// The skill-project.toml found from the current directory, if any. +fn load_current_project() -> CliResult> { let current_dir = env::current_dir() .map_err(|e| CliError::Config(format!("Failed to get current directory: {}", e)))?; - // Try to find skill-project.toml let project_file = project::resolve_project_file(¤t_dir); if !project_file.found { - return Ok(Vec::new()); // No skill-project.toml found + return Ok(None); } let project_path = project_file.path; @@ -32,20 +68,7 @@ pub fn load_repositories_from_project() -> CliResult> e )) })?; - - // Extract repositories from [tool.fastskill] - let repositories = project - .tool - .and_then(|t| t.fastskill) - .and_then(|f| f.repositories) - .unwrap_or_default(); - - // Convert manifest::RepositoryDefinition to repository::RepositoryDefinition - let converted_repos = repositories - .into_iter() - .map(convert_repository_definition) - .collect(); - Ok(converted_repos) + Ok(Some((project, project_path))) } /// Convert manifest::RepositoryDefinition to repository::RepositoryDefinition @@ -237,8 +260,9 @@ 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. +/// `[tool.fastskill.repositories]` and those of the Manifests it composes). +/// 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` @@ -255,7 +279,7 @@ pub fn inject_edge_services(mut service: FastSkillService) -> CliResult std::process::Output { + Command::new(env!("CARGO_BIN_EXE_fastskill")) + .current_dir(root) + .args(["project", "install", "--no-reindex"]) + .output() + .unwrap() +} + +#[test] +fn a_composed_repository_origin_resolves_through_the_repositories_its_manifest_declares() { + let root = TempDir::new().unwrap(); + shared_manifest_with_its_own_catalog(root.path()); + fs::write( + root.path().join("skill-project.toml"), + "[dependencies]\n[tool.fastskill]\nskills_directory = \"skills\"\n\ + [tool.fastskill.manifests]\nteam = \"shared\"\n", + ) + .unwrap(); + + let output = install(root.path()); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let installed = fs::read_to_string(root.path().join("skills/review/SKILL.md")).unwrap(); + assert!(installed.contains("shared catalog"), "{installed}"); +} + +#[test] +fn a_repository_name_defined_differently_by_composed_manifests_is_refused() { + let root = TempDir::new().unwrap(); + shared_manifest_with_its_own_catalog(root.path()); + let other = root.path().join("catalog/review"); + fs::create_dir_all(&other).unwrap(); + fs::write( + other.join("SKILL.md"), + "---\nname: review\nversion: 1.0.0\ndescription: consumer catalog\n---\n# Review\n", + ) + .unwrap(); + fs::write( + root.path().join("skill-project.toml"), + "[dependencies]\n[tool.fastskill]\nskills_directory = \"skills\"\n\ + [tool.fastskill.manifests]\nteam = \"shared\"\n\ + [[tool.fastskill.repositories]]\n\ + name = \"team\"\n\ + type = \"local\"\n\ + priority = 1\n\ + path = \"catalog\"\n", + ) + .unwrap(); + + let output = install(root.path()); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(!output.status.success(), "{stderr}"); + assert!(stderr.contains("Repository 'team'"), "{stderr}"); + assert!(!root.path().join("skills/review").exists()); + assert!(!root.path().join("skills.lock").exists()); +} + +#[test] +fn the_same_repository_declared_by_two_composed_manifests_is_one_repository() { + let root = TempDir::new().unwrap(); + shared_manifest_with_its_own_catalog(root.path()); + fs::write( + root.path().join("skill-project.toml"), + "[dependencies]\n[tool.fastskill]\nskills_directory = \"skills\"\n\ + [tool.fastskill.manifests]\nteam = \"shared\"\n\ + [[tool.fastskill.repositories]]\n\ + name = \"team\"\n\ + type = \"local\"\n\ + priority = 1\n\ + path = \"shared/catalog\"\n", + ) + .unwrap(); + + let output = install(root.path()); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(root.path().join("skills/review/SKILL.md").is_file()); +} + +#[test] +fn adding_a_repository_never_copies_composed_ones_into_the_project() { + let root = TempDir::new().unwrap(); + shared_manifest_with_its_own_catalog(root.path()); + fs::create_dir_all(root.path().join("mine")).unwrap(); + fs::write( + root.path().join("skill-project.toml"), + "[dependencies]\n[tool.fastskill]\nskills_directory = \"skills\"\n\ + [tool.fastskill.manifests]\nteam = \"shared\"\n", + ) + .unwrap(); + + let output = Command::new(env!("CARGO_BIN_EXE_fastskill")) + .current_dir(root.path()) + .args(["repo", "add", "mine", "mine", "--repo-type", "local"]) + .output() + .unwrap(); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let project = fs::read_to_string(root.path().join("skill-project.toml")).unwrap(); + assert!(project.contains("name = \"mine\""), "{project}"); + assert!(!project.contains("name = \"team\""), "{project}"); +} diff --git a/crates/fastskill-core/src/core/manifest/composition.rs b/crates/fastskill-core/src/core/manifest/composition.rs index dbb1eb3e..20a71d8e 100644 --- a/crates/fastskill-core/src/core/manifest/composition.rs +++ b/crates/fastskill-core/src/core/manifest/composition.rs @@ -1,9 +1,15 @@ -use super::{DependencySpec, SkillEntry, SkillProjectToml}; +use super::{ + DependencySpec, RepositoryConnection, RepositoryDefinition, SkillEntry, SkillProjectToml, +}; use crate::core::origin::Origin; use crate::core::version::VersionConstraint; -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::{Path, PathBuf}; +/// Called once per Manifest in a composition: the Manifest, the directory its +/// relative paths are anchored to, and the file it was read from. +type Visit<'a> = dyn FnMut(&SkillProjectToml, &Path, &Path) -> Result<(), String> + 'a; + impl SkillProjectToml { /// Read dependencies of an installed or fetched skill without following host paths. /// Manifest composition is a project-authoring feature, not package metadata. @@ -28,38 +34,102 @@ impl SkillProjectToml { /// Convert local and composed Manifest dependencies to install roots. /// Relative origins stay anchored to the Manifest that declares them. pub fn to_skill_entries(&self, manifest_dir: &Path) -> Result, String> { - let root_path = manifest_dir.join("skill-project.toml"); - let root_path = root_path.canonicalize().unwrap_or(root_path); - let mut stack = vec![root_path.clone()]; - let mut visited = HashSet::from([root_path]); - let mut entries = BTreeMap::new(); - self.collect_skill_entries(manifest_dir, &mut stack, &mut visited, &mut entries)?; + let mut entries: BTreeMap = BTreeMap::new(); + self.visit_composed(manifest_dir, &mut |manifest, dir, _| { + for entry in manifest.direct_skill_entries(dir)? { + if let Some(existing) = entries.get(&entry.id) { + let mut existing_groups = existing.groups.clone(); + let mut incoming_groups = entry.groups.clone(); + existing_groups.sort(); + incoming_groups.sort(); + if existing.origin != entry.origin || existing_groups != incoming_groups { + return Err(format!( + "Conflicting dependency '{}' is declared by composed Manifests with different origins or groups", + entry.id + )); + } + } else { + entries.insert(entry.id.clone(), entry); + } + } + Ok(()) + })?; Ok(entries.into_values().collect()) } - fn collect_skill_entries( + /// Repositories declared by this Manifest and every Manifest it composes, + /// so a composed repository origin resolves through the catalog its own + /// Manifest names. A composed Manifest's relative local path is anchored to + /// that Manifest; this Manifest's own entries are returned as declared. + /// One name defined two different ways is an error, never a silent pick. + pub fn composed_repositories( + &self, + manifest_dir: &Path, + ) -> Result, String> { + let root_path = manifest_dir.join("skill-project.toml"); + let mut repositories = Vec::new(); + let mut declared: HashMap = HashMap::new(); + self.visit_composed(manifest_dir, &mut |manifest, dir, path| { + let declared_here = manifest + .tool + .as_ref() + .and_then(|tool| tool.fastskill.as_ref()) + .and_then(|fastskill| fastskill.repositories.as_ref()); + for repository in declared_here.into_iter().flatten() { + let anchored = anchored_to(repository, dir); + let identity = toml::Value::try_from(&anchored).map_err(|error| { + format!( + "Repository '{}' cannot be compared: {error}", + repository.name + ) + })?; + match declared.get(&repository.name) { + Some((existing, _)) if *existing == identity => {} + Some((_, first)) => { + return Err(format!( + "Repository '{}' is defined differently by {} and {}; \ + give one of them another name", + repository.name, + first.display(), + path.display() + )); + } + None => { + declared.insert(repository.name.clone(), (identity, path.to_path_buf())); + repositories.push(if path == root_path { + repository.clone() + } else { + anchored + }); + } + } + } + Ok(()) + })?; + Ok(repositories) + } + + /// Visit this Manifest, then every Manifest it composes, depth first and + /// each once. A reference cycle is an error. + fn visit_composed(&self, manifest_dir: &Path, visit: &mut Visit<'_>) -> Result<(), String> { + let root_path = manifest_dir.join("skill-project.toml"); + let canonical_root = root_path + .canonicalize() + .unwrap_or_else(|_| root_path.clone()); + let mut stack = vec![canonical_root.clone()]; + let mut visited = HashSet::from([canonical_root]); + self.walk_composed(manifest_dir, &root_path, &mut stack, &mut visited, visit) + } + + fn walk_composed( &self, manifest_dir: &Path, + manifest_path: &Path, stack: &mut Vec, visited: &mut HashSet, - entries: &mut BTreeMap, + visit: &mut Visit<'_>, ) -> Result<(), String> { - for entry in self.direct_skill_entries(manifest_dir)? { - if let Some(existing) = entries.get(&entry.id) { - let mut existing_groups = existing.groups.clone(); - let mut incoming_groups = entry.groups.clone(); - existing_groups.sort(); - incoming_groups.sort(); - if existing.origin != entry.origin || existing_groups != incoming_groups { - return Err(format!( - "Conflicting dependency '{}' is declared by composed Manifests with different origins or groups", - entry.id - )); - } - } else { - entries.insert(entry.id.clone(), entry); - } - } + visit(self, manifest_dir, manifest_path)?; let manifests = self .tool @@ -112,7 +182,7 @@ impl SkillProjectToml { ) })?; stack.push(canonical); - referenced.collect_skill_entries(&referenced_dir, stack, visited, entries)?; + referenced.walk_composed(&referenced_dir, &path, stack, visited, visit)?; stack.pop(); } Ok(()) @@ -150,3 +220,16 @@ impl SkillProjectToml { Ok(entries) } } + +/// The repository with a relative local path resolved against the directory +/// of the Manifest that declares it, canonical when the directory exists so +/// two spellings of one catalog compare equal. +fn anchored_to(repository: &RepositoryDefinition, manifest_dir: &Path) -> RepositoryDefinition { + let mut anchored = repository.clone(); + if let RepositoryConnection::Local { path } = &mut anchored.connection { + let joined = manifest_dir.join(&*path); + let resolved = joined.canonicalize().unwrap_or(joined); + *path = resolved.to_string_lossy().into_owned(); + } + anchored +} diff --git a/crates/fastskill-core/src/http/handlers/manifest.rs b/crates/fastskill-core/src/http/handlers/manifest.rs index 7c1e5f06..886cb0e8 100644 --- a/crates/fastskill-core/src/http/handlers/manifest.rs +++ b/crates/fastskill-core/src/http/handlers/manifest.rs @@ -65,22 +65,21 @@ fn ensure_document_unchanged( } } -/// Get repositories from skill-project.toml using the canonical From impl. +/// Repositories of the project and every Manifest it composes, converted with +/// the canonical From impl. pub(crate) fn get_repositories( project: &SkillProjectToml, -) -> Vec { - project - .tool - .as_ref() - .and_then(|t| t.fastskill.as_ref()) - .and_then(|f| f.repositories.as_ref()) - .map(|repos| { - repos - .iter() - .map(crate::core::repository::RepositoryDefinition::from) - .collect() - }) - .unwrap_or_default() + project_path: &std::path::Path, +) -> HttpResult> { + let project_dir = project_path + .parent() + .unwrap_or_else(|| std::path::Path::new(".")); + Ok(project + .composed_repositories(project_dir) + .map_err(HttpError::InternalServerError)? + .iter() + .map(crate::core::repository::RepositoryDefinition::from) + .collect()) } fn dep_source_type(spec: &DependencySpec) -> &'static str { @@ -292,7 +291,7 @@ pub async fn add_skill_to_manifest( }); // Get repositories and create manager - let repositories = get_repositories(&project); + let repositories = get_repositories(&project, project_path)?; let repo_manager = RepositoryManager::from_definitions(repositories); // Get sources manager for marketplace-based repositories. spec 008: opts diff --git a/crates/fastskill-core/src/http/handlers/registry.rs b/crates/fastskill-core/src/http/handlers/registry.rs index 03940bbb..354173ae 100644 --- a/crates/fastskill-core/src/http/handlers/registry.rs +++ b/crates/fastskill-core/src/http/handlers/registry.rs @@ -11,27 +11,27 @@ use axum::{ }; use std::collections::HashSet; -fn get_repository_manager(project_file_path: &std::path::Path) -> RepositoryManager { - if project_file_path.exists() { - if let Ok(project) = - crate::core::manifest::SkillProjectToml::load_from_file(project_file_path) - { - if let Some(tool) = project.tool { - if let Some(fastskill_config) = tool.fastskill { - if let Some(repos) = fastskill_config.repositories { - let definitions: Vec<_> = repos - .iter() - .map(crate::core::repository::RepositoryDefinition::from) - .collect(); - - return RepositoryManager::from_definitions(definitions); - } - } - } - } - } - - RepositoryManager::from_definitions(Vec::new()) +/// Repositories of the served project and every Manifest it composes. An +/// unreadable project still serves no repositories; a composition that names +/// one repository two ways is an error rather than a silent pick. +fn get_repository_manager(project_file_path: &std::path::Path) -> HttpResult { + let Some(project) = project_file_path + .exists() + .then(|| crate::core::manifest::SkillProjectToml::load_from_file(project_file_path).ok()) + .flatten() + else { + return Ok(RepositoryManager::from_definitions(Vec::new())); + }; + let project_dir = project_file_path + .parent() + .unwrap_or_else(|| std::path::Path::new(".")); + let definitions = project + .composed_repositories(project_dir) + .map_err(HttpError::InternalServerError)? + .iter() + .map(crate::core::repository::RepositoryDefinition::from) + .collect(); + Ok(RepositoryManager::from_definitions(definitions)) } /// Builds a `SourcesManager` backed by a unique per-call temp directory. @@ -140,12 +140,25 @@ async fn get_sources_manager_from_repos( Ok((sources_manager, temp_dir)) } +/// Marketplace sources for the served project's repositories, kept alive by +/// the returned temp dir. +async fn project_sources_manager( + state: &AppState, +) -> HttpResult<(SourcesManager, tempfile::TempDir)> { + let repo_manager = get_repository_manager(&state.project_file_path)?; + get_sources_manager_from_repos(&repo_manager) + .await + .map_err(|e| { + HttpError::InternalServerError(format!("Failed to create sources manager: {}", e)) + }) +} + /// GET /api/v1/registry/sources - List all configured sources/repositories pub async fn list_sources( State(state): State, ) -> HttpResult>>> { // Get repository manager (supports all formats) - let repo_manager = get_repository_manager(&state.project_file_path); + let repo_manager = get_repository_manager(&state.project_file_path)?; let repos = repo_manager.list_repositories(); @@ -190,12 +203,7 @@ pub async fn list_sources( pub async fn list_all_skills( State(state): State, ) -> HttpResult>> { - let repo_manager = get_repository_manager(&state.project_file_path); - let (sources_manager, _sources_tmp) = get_sources_manager_from_repos(&repo_manager) - .await - .map_err(|e| { - HttpError::InternalServerError(format!("Failed to create sources manager: {}", e)) - })?; + let (sources_manager, _sources_tmp) = project_sources_manager(&state).await?; let skill_manager = state.service.skill_manager(); // Get all installed skills @@ -277,12 +285,7 @@ pub async fn list_source_skills( Path(source_name): Path, State(state): State, ) -> HttpResult>> { - let repo_manager = get_repository_manager(&state.project_file_path); - let (sources_manager, _sources_tmp) = get_sources_manager_from_repos(&repo_manager) - .await - .map_err(|e| { - HttpError::InternalServerError(format!("Failed to create sources manager: {}", e)) - })?; + let (sources_manager, _sources_tmp) = project_sources_manager(&state).await?; let skill_manager = state.service.skill_manager(); // Get source definition @@ -343,12 +346,7 @@ pub async fn get_marketplace( Path(source_name): Path, State(state): State, ) -> HttpResult>> { - let repo_manager = get_repository_manager(&state.project_file_path); - let (sources_manager, _sources_tmp) = get_sources_manager_from_repos(&repo_manager) - .await - .map_err(|e| { - HttpError::InternalServerError(format!("Failed to create sources manager: {}", e)) - })?; + let (sources_manager, _sources_tmp) = project_sources_manager(&state).await?; // Get source definition let source_def = sources_manager @@ -377,12 +375,7 @@ pub async fn get_marketplace( pub async fn refresh_sources( State(state): State, ) -> HttpResult>> { - let repo_manager = get_repository_manager(&state.project_file_path); - let (sources_manager, _sources_tmp) = get_sources_manager_from_repos(&repo_manager) - .await - .map_err(|e| { - HttpError::InternalServerError(format!("Failed to create sources manager: {}", e)) - })?; + let (sources_manager, _sources_tmp) = project_sources_manager(&state).await?; // Clear the cache sources_manager.clear_cache().await; @@ -419,7 +412,7 @@ pub async fn list_skill_versions( })) }; - let repo_manager = get_repository_manager(&state.project_file_path); + let repo_manager = get_repository_manager(&state.project_file_path)?; let (sources_manager, _sources_tmp) = match get_sources_manager_from_repos(&repo_manager).await { Ok(pair) => pair, @@ -569,13 +562,10 @@ pub async fn serve_index_file( // Read the index file (use canonical path) match tokio::fs::read_to_string(&canonical_index_path).await { - Ok(content) => Ok(axum::response::Response::builder() - .status(axum::http::StatusCode::OK) - .header("Content-Type", "application/json") - .body(axum::body::Body::from(content)) - .map_err(|e| { - HttpError::InternalServerError(format!("Failed to build response: {}", e)) - })?), + Ok(content) => Ok(axum::response::IntoResponse::into_response(( + [(axum::http::header::CONTENT_TYPE, "application/json")], + content, + ))), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Err(HttpError::NotFound(format!( "Index file not found for skill: {}", skill_id diff --git a/crates/fastskill-core/tests/http_handler_route_tests.rs b/crates/fastskill-core/tests/http_handler_route_tests.rs index 8307dd0c..5a52c60a 100644 --- a/crates/fastskill-core/tests/http_handler_route_tests.rs +++ b/crates/fastskill-core/tests/http_handler_route_tests.rs @@ -1843,11 +1843,11 @@ async fn registry_skill_versions_populated_and_sorted_descending() { // ---- registry index (list_index_skills) ---- -async fn state_with_registry(registry: &TempDir) -> AppState { +async fn state_with_registry(registry: &std::path::Path) -> AppState { let storage = TempDir::new().unwrap(); let store = skills_root(&storage); write_skill(&store, "alpha-skill", "Alpha Skill", "First"); - let service = make_service(store, Some(registry.path().to_path_buf())).await; + let service = make_service(store, Some(registry.to_path_buf())).await; // Skills are already indexed in memory; the storage temp dir is no longer needed. drop(storage); AppState::new(service).unwrap() @@ -1888,7 +1888,7 @@ async fn index_skills_no_registry_configured_is_404() { async fn index_skills_lists_seeded() { let registry = TempDir::new().unwrap(); seed_registry(registry.path(), "acme/widget", "1.0.0"); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, body) = do_get(state, "/registry/index/skills").await; assert_eq!(status, StatusCode::OK, "body: {body}"); assert!(body.contains("acme/widget")); @@ -1897,7 +1897,7 @@ async fn index_skills_lists_seeded() { #[tokio::test] async fn index_skills_empty_scope_is_400() { let registry = TempDir::new().unwrap(); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, _b) = do_get(state, "/registry/index/skills?scope=").await; assert_eq!(status, StatusCode::BAD_REQUEST); } @@ -1905,7 +1905,7 @@ async fn index_skills_empty_scope_is_400() { #[tokio::test] async fn index_skills_scope_with_separator_is_400() { let registry = TempDir::new().unwrap(); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, _b) = do_get(state, "/registry/index/skills?scope=a%2Fb").await; assert_eq!(status, StatusCode::BAD_REQUEST); } @@ -1913,7 +1913,7 @@ async fn index_skills_scope_with_separator_is_400() { #[tokio::test] async fn index_skills_scope_with_dotdot_is_400() { let registry = TempDir::new().unwrap(); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, _b) = do_get(state, "/registry/index/skills?scope=..").await; assert_eq!(status, StatusCode::BAD_REQUEST); } @@ -1921,7 +1921,7 @@ async fn index_skills_scope_with_dotdot_is_400() { #[tokio::test] async fn index_skills_scope_with_bad_char_is_400() { let registry = TempDir::new().unwrap(); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, _b) = do_get(state, "/registry/index/skills?scope=bad%21name").await; assert_eq!(status, StatusCode::BAD_REQUEST); } @@ -1930,7 +1930,7 @@ async fn index_skills_scope_with_bad_char_is_400() { async fn index_skills_valid_scope_and_flags_ok() { let registry = TempDir::new().unwrap(); seed_registry(registry.path(), "acme/widget", "1.0.0"); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, _b) = do_get( state, "/registry/index/skills?scope=acme&all_versions=true&include_pre_release=1", @@ -1959,7 +1959,7 @@ async fn serve_index_file_success() { "{\"name\":\"testorg/serve-skill\"}", ) .unwrap(); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, body) = do_get(state, "/index/testorg/serve-skill").await; assert_eq!(status, StatusCode::OK, "body: {body}"); assert!(body.contains("serve-skill")); @@ -1969,7 +1969,56 @@ async fn serve_index_file_success() { async fn serve_index_file_missing_is_404() { let registry = TempDir::new().unwrap(); fs::create_dir_all(registry.path().join("testorg")).unwrap(); - let state = state_with_registry(®istry).await; + let state = state_with_registry(registry.path()).await; let (status, _b) = do_get(state, "/index/testorg/ghost").await; assert_eq!(status, StatusCode::NOT_FOUND); } + +#[tokio::test] +async fn serve_index_file_missing_registry_root_is_500() { + let registry = TempDir::new().unwrap(); + let state = state_with_registry(®istry.path().join("absent")).await; + let (status, body) = do_get(state, "/index/testorg/widget").await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "body: {body}"); + assert!(body.contains("Failed to canonicalize registry path")); +} + +// Walking through a regular file is ENOTDIR on Unix; Windows reports NotFound. +#[cfg(unix)] +#[tokio::test] +async fn serve_index_file_through_a_file_is_500() { + let registry = TempDir::new().unwrap(); + fs::write(registry.path().join("testorg"), "not a directory").unwrap(); + let state = state_with_registry(registry.path()).await; + let (status, body) = do_get(state, "/index/testorg/widget").await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "body: {body}"); + assert!(body.contains("Failed to canonicalize index path")); +} + +#[tokio::test] +async fn serve_index_file_directory_is_500() { + let registry = TempDir::new().unwrap(); + fs::create_dir_all(registry.path().join("testorg/widget")).unwrap(); + let state = state_with_registry(registry.path()).await; + let (status, body) = do_get(state, "/index/testorg/widget").await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "body: {body}"); + assert!(body.contains("Failed to read index file")); +} + +#[cfg(unix)] +#[tokio::test] +async fn serve_index_file_symlink_out_of_the_index_is_400() { + let registry = TempDir::new().unwrap(); + let outside = TempDir::new().unwrap(); + fs::write(outside.path().join("secret"), "{}").unwrap(); + fs::create_dir_all(registry.path().join("testorg")).unwrap(); + std::os::unix::fs::symlink( + outside.path().join("secret"), + registry.path().join("testorg/widget"), + ) + .unwrap(); + let state = state_with_registry(registry.path()).await; + let (status, body) = do_get(state, "/index/testorg/widget").await; + assert_eq!(status, StatusCode::BAD_REQUEST, "body: {body}"); + assert!(body.contains("Invalid skill ID")); +} diff --git a/crates/fastskill-core/tests/http_registry_sources_tests.rs b/crates/fastskill-core/tests/http_registry_sources_tests.rs new file mode 100644 index 00000000..b3edf233 --- /dev/null +++ b/crates/fastskill-core/tests/http_registry_sources_tests.rs @@ -0,0 +1,303 @@ +//! `/registry/*` browse routes over a project that actually declares +//! repositories — a zip-url marketplace (wiremock), unreadable Git and zip-url +//! marketplaces, a local catalog and an HTTP registry — plus the repositories a +//! composed Manifest brings in. `http_handler_route_tests.rs` covers the same +//! routes with no repositories configured. + +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use axum::{ + body::Body, + http::{Request, StatusCode}, + routing::{get, post}, + Router, +}; +use fastskill_core::http::handlers::{registry, AppState}; +use fastskill_core::{FastSkillService, ServiceConfig}; +use std::fs; +use std::path::Path; +use std::sync::Arc; +use tempfile::TempDir; +use tower::ServiceExt; +use wiremock::{ + matchers::{method, path}, + Mock, MockServer, ResponseTemplate, +}; + +struct Project { + _dir: TempDir, + state: AppState, +} + +/// A served project whose `skill-project.toml` is `manifest`, with one +/// installed skill (`review`) so marketplace entries can report `installed`. +async fn project(manifest: &str, extra: impl FnOnce(&Path)) -> Project { + let dir = TempDir::new().unwrap(); + let store = dir.path().join("store"); + fs::create_dir_all(store.join("review")).unwrap(); + fs::write( + store.join("review/SKILL.md"), + "---\nname: review\ndescription: installed\nversion: 1.0.0\n---\n# Review\n", + ) + .unwrap(); + let project_file = dir.path().join("skill-project.toml"); + fs::write(&project_file, manifest).unwrap(); + extra(dir.path()); + + let mut service = FastSkillService::new(ServiceConfig { + skill_cache_root: Some(dir.path().join("cache")), + skill_storage_path: store.clone(), + ..Default::default() + }) + .await + .unwrap(); + service.initialize().await.unwrap(); + let state = AppState::new(Arc::new(service)) + .unwrap() + .with_project_config(dir.path().to_path_buf(), project_file, store); + Project { _dir: dir, state } +} + +async fn marketplace_server() -> MockServer { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/.claude-plugin/marketplace.json")) + .respond_with(ResponseTemplate::new(404)) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/marketplace.json")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "name": "team-marketplace", + "plugins": [{ + "name": "review", + "description": "marketplace review", + "source": "./plugins/review", + "skills": ["./skills/review"] + }] + }))) + .mount(&server) + .await; + server +} + +/// One repository of every kind the browse routes distinguish, plus a +/// zip-url repository whose `auth` block makes its marketplace unreadable. +fn every_kind_of_repository(market: &MockServer) -> String { + format!( + "[dependencies]\n\ + [[tool.fastskill.repositories]]\n\ + name = \"market\"\ntype = \"zip-url\"\nzip_url = {market:?}\npriority = 0\n\ + [[tool.fastskill.repositories]]\n\ + name = \"locked\"\ntype = \"zip-url\"\nzip_url = {market:?}\npriority = 4\n\ + auth = {{ type = \"pat\", env_var = \"FASTSKILL_TEST_UNSET_TOKEN\" }}\n\ + [[tool.fastskill.repositories]]\n\ + name = \"broken\"\ntype = \"git-marketplace\"\nurl = \"file:///nonexistent/fastskill-broken.git\"\npriority = 1\n\ + auth = {{ type = \"pat\", env_var = \"FASTSKILL_TEST_UNSET_TOKEN\" }}\n\ + [[tool.fastskill.repositories]]\n\ + name = \"shelf\"\ntype = \"local\"\npath = \"shelf\"\npriority = 2\n\ + [[tool.fastskill.repositories]]\n\ + name = \"index\"\ntype = \"http-registry\"\nindex_url = \"http://127.0.0.1:9/index\"\npriority = 3\n", + market = market.uri() + ) +} + +async fn send(state: AppState, method: &str, uri: &str) -> (StatusCode, serde_json::Value) { + let app = Router::new() + .route("/registry/sources", get(registry::list_sources)) + .route("/registry/skills", get(registry::list_all_skills)) + .route( + "/registry/skills/{id}/versions", + get(registry::list_skill_versions), + ) + .route( + "/registry/sources/{name}/skills", + get(registry::list_source_skills), + ) + .route( + "/registry/sources/{name}/marketplace", + get(registry::get_marketplace), + ) + .route("/registry/refresh", post(registry::refresh_sources)) + .with_state(state); + let request = Request::builder() + .method(method) + .uri(uri) + .body(Body::empty()) + .unwrap(); + let response = app.oneshot(request).await.unwrap(); + let status = response.status(); + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + let body = serde_json::from_slice(&bytes) + .unwrap_or_else(|_| serde_json::Value::String(String::from_utf8_lossy(&bytes).into())); + (status, body) +} + +#[tokio::test] +async fn sources_describe_every_kind_of_repository() { + let market = marketplace_server().await; + let p = project(&every_kind_of_repository(&market), |_| {}).await; + + let (status, body) = send(p.state, "GET", "/registry/sources").await; + + assert_eq!(status, StatusCode::OK, "{body}"); + let sources = body["data"].as_array().unwrap(); + let kind = |name: &str| { + sources + .iter() + .find(|s| s["name"] == name) + .unwrap_or_else(|| panic!("{name} missing from {body}")) + .clone() + }; + assert_eq!(kind("market")["sourceType"], "zip-url"); + assert_eq!(kind("market")["supportsMarketplace"], true); + assert_eq!(kind("broken")["sourceType"], "git-marketplace"); + assert_eq!(kind("shelf")["sourceType"], "local"); + assert_eq!(kind("shelf")["path"], "shelf"); + assert_eq!(kind("index")["sourceType"], "http-registry"); + assert_eq!(kind("index")["supportsMarketplace"], false); +} + +#[tokio::test] +async fn all_skills_lists_readable_marketplaces_and_skips_the_rest() { + let market = marketplace_server().await; + let p = project(&every_kind_of_repository(&market), |_| {}).await; + + let (status, body) = send(p.state, "GET", "/registry/skills").await; + + assert_eq!(status, StatusCode::OK, "{body}"); + let data = &body["data"]; + let sources = data["sources"].as_array().unwrap(); + assert_eq!(sources.len(), 1, "{body}"); + assert_eq!(sources[0]["sourceName"], "market"); + assert_eq!(sources[0]["skills"][0]["installed"], true, "{body}"); + assert_eq!(data["totalSkills"], sources[0]["count"]); + assert_eq!( + data["totalSources"], 4, + "the HTTP registry is not a source: {body}" + ); +} + +#[tokio::test] +async fn refresh_reloads_the_same_listing() { + let market = marketplace_server().await; + let p = project(&every_kind_of_repository(&market), |_| {}).await; + + let (status, body) = send(p.state, "POST", "/registry/refresh").await; + + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["data"]["sources"][0]["sourceName"], "market"); +} + +#[tokio::test] +async fn one_marketplace_is_listed_with_install_status() { + let market = marketplace_server().await; + let p = project(&every_kind_of_repository(&market), |_| {}).await; + + let (status, body) = send(p.state.clone(), "GET", "/registry/sources/market/skills").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["data"]["sourceName"], "market"); + assert_eq!(body["data"]["skills"][0]["installed"], true, "{body}"); + assert_eq!( + body["data"]["count"], + body["data"]["skills"].as_array().unwrap().len() + ); + + let (status, body) = send(p.state, "GET", "/registry/sources/market/marketplace").await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert!(body["data"].is_object(), "{body}"); +} + +#[tokio::test] +async fn a_source_without_a_marketplace_is_a_bad_request() { + let market = marketplace_server().await; + let p = project(&every_kind_of_repository(&market), |_| {}).await; + + for uri in [ + "/registry/sources/shelf/skills", + "/registry/sources/shelf/marketplace", + ] { + let (status, body) = send(p.state.clone(), "GET", uri).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{uri}: {body}"); + assert!(body.to_string().contains("does not support marketplace")); + } +} + +#[tokio::test] +async fn an_unreadable_marketplace_is_a_server_error() { + let market = marketplace_server().await; + let p = project(&every_kind_of_repository(&market), |_| {}).await; + + for uri in [ + "/registry/sources/broken/skills", + "/registry/sources/broken/marketplace", + ] { + let (status, body) = send(p.state.clone(), "GET", uri).await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "{uri}: {body}"); + assert!(body.to_string().contains("Failed to load marketplace")); + } +} + +#[tokio::test] +async fn versions_come_from_the_readable_marketplaces() { + let market = marketplace_server().await; + let p = project(&every_kind_of_repository(&market), |_| {}).await; + + let (status, body) = send(p.state, "GET", "/registry/skills/review/versions").await; + + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["data"]["id"], "review"); + assert!(body["data"]["versions"].is_array(), "{body}"); +} + +#[tokio::test] +async fn composed_repositories_are_browsable() { + let market = marketplace_server().await; + let shared = format!( + "[dependencies]\n[[tool.fastskill.repositories]]\n\ + name = \"market\"\ntype = \"zip-url\"\nzip_url = {:?}\npriority = 0\n", + market.uri() + ); + let p = project( + "[dependencies]\n[tool.fastskill.manifests]\nteam = \"shared\"\n", + |root| { + fs::create_dir_all(root.join("shared")).unwrap(); + fs::write(root.join("shared/skill-project.toml"), &shared).unwrap(); + }, + ) + .await; + + let (status, body) = send(p.state, "GET", "/registry/sources/market/skills").await; + + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["data"]["sourceName"], "market"); +} + +#[tokio::test] +async fn one_repository_name_defined_two_ways_is_refused() { + let p = project( + "[dependencies]\n[tool.fastskill.manifests]\nteam = \"shared\"\n\ + [[tool.fastskill.repositories]]\nname = \"team\"\ntype = \"local\"\npath = \"mine\"\npriority = 0\n", + |root| { + fs::create_dir_all(root.join("shared")).unwrap(); + fs::write( + root.join("shared/skill-project.toml"), + "[dependencies]\n[[tool.fastskill.repositories]]\n\ + name = \"team\"\ntype = \"local\"\npath = \"theirs\"\npriority = 0\n", + ) + .unwrap(); + }, + ) + .await; + + let (status, body) = send(p.state, "GET", "/registry/sources").await; + + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "{body}"); + assert!( + body.to_string() + .contains("Repository 'team' is defined differently"), + "{body}" + ); +} diff --git a/webdocs/skill-management/manifest-system.mdx b/webdocs/skill-management/manifest-system.mdx index 41c2a257..2914862f 100644 --- a/webdocs/skill-management/manifest-system.mdx +++ b/webdocs/skill-management/manifest-system.mdx @@ -44,6 +44,16 @@ shared-filesystem origins normally. identical declarations are deduplicated. Missing files, cycles, or different origins/groups for the same skill ID are errors and stop installation before managed state changes. +A composed Manifest's `[[tool.fastskill.repositories]]` join the consuming project's for +resolution, so a shared `repository` origin finds the catalog its own Manifest names. A local +repository `path` is relative to the Manifest that declares it. When two Manifests define the same +repository name differently, installation stops and asks you to rename one; identical definitions +are merged. `fastskill repo` commands list and edit only the project's own repositories. + +Only dependencies and repositories are taken from a composed Manifest. Its `skills_directory`, +`[tool.fastskill.eval]` and `[tool.fastskill.server]` are ignored on purpose: those settings belong +to the consuming project. + Manifest composition is supported in author-controlled project manifests. Installed or fetched skill packages must declare dependencies directly; their manifest references are rejected before any referenced file is read. This prevents packages from importing host project files.