diff --git a/crates/fastskill-cli/src/commands/add/mod.rs b/crates/fastskill-cli/src/commands/add/mod.rs index 7c2bfe8..9bb6fa5 100644 --- a/crates/fastskill-cli/src/commands/add/mod.rs +++ b/crates/fastskill-cli/src/commands/add/mod.rs @@ -534,39 +534,12 @@ pub async fn execute_add(service: &FastSkillService, args: AddArgs, global: bool } else { AddMode::Fresh }; + let groups = args.group.clone().map(|g| vec![g]).unwrap_or_default(); let outcome = service - .add_from_origin(origin, mode) + .add_from_origin(origin, mode, groups) .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 - )) - ); - } - } - // `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). diff --git a/crates/fastskill-cli/src/commands/serve.rs b/crates/fastskill-cli/src/commands/serve.rs index bf1e28d..6dcee04 100644 --- a/crates/fastskill-cli/src/commands/serve.rs +++ b/crates/fastskill-cli/src/commands/serve.rs @@ -99,7 +99,8 @@ impl FromArgValueMap for ServeArgs { } pub async fn execute_serve( - service: std::sync::Arc, + global: bool, + skills_dir: Option, args: ServeArgs, ) -> CliResult<()> { info!( @@ -120,6 +121,29 @@ pub async fn execute_serve( println!(" Write endpoints: disabled (read-only); pass --enable-write to enable"); } + // Build the served service directly (rather than reusing the CLI's cached + // singleton via `FsState::service_with`) so this exact instance can carry + // BOTH the edge-injected embedding provider + repository manager + // (`inject_edge_services`, same as every other command) AND the served + // project's root (ADR-0005 install seam): `add_from_origin`/`preflight`-driven + // writes (via `POST /skills/install`, `/skills/update`) must land in the + // served project's `skill-project.toml`/`skills.lock`, not wherever the + // server process happens to have cwd set. + let cfg = crate::config::create_service_config(global, skills_dir)?; + let mut service = fastskill_core::FastSkillService::new(cfg) + .await + .map_err(CliError::Service)?; + service.initialize().await.map_err(CliError::Service)?; + let mut service = crate::config::inject_edge_services(service)?; + + if let Ok(current_dir) = std::env::current_dir() { + if let Ok(project_config) = fastskill_core::core::load_project_config(¤t_dir) { + service = service.with_project_root(project_config.project_root); + } + } + + let service = std::sync::Arc::new(service); + let server = fastskill_core::http::server::FastSkillServer::from_ref(&service, &args.host, args.port) .enable_write(args.enable_write); diff --git a/crates/fastskill-cli/src/commands/update.rs b/crates/fastskill-cli/src/commands/update.rs index 6bd601d..48d09a4 100644 --- a/crates/fastskill-cli/src/commands/update.rs +++ b/crates/fastskill-cli/src/commands/update.rs @@ -2,7 +2,7 @@ use crate::config::create_service_config; use crate::error::{manifest_required_message, CliError, CliResult}; -use crate::utils::{manifest_utils, messages}; +use crate::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; @@ -417,28 +417,12 @@ async fn execute_update_project(args: UpdateArgs) -> CliResult<()> { println!(" Updating {}...", entry.id); match service.preflight(&entry.origin).await { Ok(UpdatePreflight::Updatable) => { + // Pass the existing groups so update preserves group membership. match service - .add_from_origin(entry.origin.clone(), AddMode::Update) + .add_from_origin(entry.origin.clone(), AddMode::Update, entry.groups.clone()) .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 - )) - ); - } + Ok(_outcome) => { updated_count += 1; println!(" {}", messages::ok(&format!("Updated {}", entry.id))); } diff --git a/crates/fastskill-cli/src/main.rs b/crates/fastskill-cli/src/main.rs index b6ef391..0350cf9 100644 --- a/crates/fastskill-cli/src/main.rs +++ b/crates/fastskill-cli/src/main.rs @@ -528,7 +528,6 @@ fn build_app(builder: AppBuilder, state: Arc) -> anyhow::Result) -> anyhow::Result 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). /// diff --git a/crates/fastskill-core/src/core/install.rs b/crates/fastskill-core/src/core/install.rs index 61a60d9..161a435 100644 --- a/crates/fastskill-core/src/core/install.rs +++ b/crates/fastskill-core/src/core/install.rs @@ -61,14 +61,17 @@ pub struct Fetched { 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)`. + /// origin and overwrites. `groups` are recorded on the Manifest + Lock entry; + /// on `Update`, an empty `groups` preserves whatever groups the skill already + /// had. Equivalent to `commit(fetch(origin), origin, mode, groups)`. pub async fn add_from_origin( &self, origin: Origin, mode: AddMode, + groups: Vec, ) -> Result { let fetched = self.fetch(&origin).await?; - self.commit(fetched, origin, mode).await + self.commit(fetched, origin, mode, groups).await } /// Fetch a skill described by `origin` into a temp dir, capturing the resolved @@ -310,6 +313,7 @@ impl FastSkillService { fetched: Fetched, origin: Origin, mode: AddMode, + groups: Vec, ) -> Result { let Fetched { temp_dir, @@ -353,7 +357,7 @@ impl FastSkillService { .force_register_skill(skill_def.clone()) .await?; - self.upsert_manifest_and_lock(&skill_def)?; + self.upsert_manifest_and_lock(&skill_def, &groups)?; let reindexed = match self.reindex(None, None).await { Ok(outcome) => outcome.reindexed, @@ -378,7 +382,11 @@ impl FastSkillService { /// `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> { + fn upsert_manifest_and_lock( + &self, + skill_def: &SkillDefinition, + groups: &[String], + ) -> Result<(), ServiceError> { // 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 @@ -420,6 +428,21 @@ impl FastSkillService { dependencies: HashMap::new(), }); } + // Effective groups: explicit `groups` win; an empty list preserves whatever + // the skill already had (so `update` never silently drops group membership). + let effective_groups: Option> = if !groups.is_empty() { + Some(groups.to_vec()) + } else { + project + .dependencies + .as_ref() + .and_then(|d| d.dependencies.get(&skill_def.id.to_string())) + .and_then(|spec| match spec { + DependencySpec::Inline { groups, .. } => groups.clone(), + DependencySpec::Version(_) => None, + }) + }; + 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. @@ -437,7 +460,7 @@ impl FastSkillService { skill_def.id.to_string(), DependencySpec::Inline { origin: origin_for_manifest, - groups: None, + groups: effective_groups.clone(), }, ); } @@ -454,6 +477,15 @@ impl FastSkillService { ProjectSkillsLock::new_empty() }; lock.update_skill(skill_def); + // Mirror the manifest's groups onto the lock entry (update_skill does not + // carry them from the manifest). + if let Some(entry) = lock + .skills + .iter_mut() + .find(|s| s.id == skill_def.id.as_str()) + { + entry.groups = effective_groups.clone().unwrap_or_default(); + } lock.save_to_file(&lock_path) .map_err(|e| ServiceError::Config(format!("Failed to save skills.lock: {e}")))?; @@ -811,7 +843,7 @@ mod tests { editable: false, }; let outcome = service - .add_from_origin(origin, AddMode::Fresh) + .add_from_origin(origin, AddMode::Fresh, vec![]) .await .expect("add should succeed"); @@ -842,6 +874,59 @@ mod tests { .is_some()); } + #[tokio::test] + #[allow(clippy::await_holding_lock)] + async fn test_add_from_origin_records_and_preserves_groups() { + 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, + }; + + // Fresh add with an explicit group records it on manifest + lock. + service + .add_from_origin(origin.clone(), AddMode::Fresh, vec!["dev".to_string()]) + .await + .expect("fresh add should succeed"); + + let groups_in_manifest = || { + let project = + SkillProjectToml::load_from_file(&tmp.path().join("skill-project.toml")).unwrap(); + match project + .dependencies + .unwrap() + .dependencies + .remove("test-skill") + { + Some(DependencySpec::Inline { groups, .. }) => groups, + _ => None, + } + }; + let lock_groups = || { + let lock = ProjectSkillsLock::load_from_file(&tmp.path().join("skills.lock")).unwrap(); + lock.skills[0].groups.clone() + }; + assert_eq!(groups_in_manifest(), Some(vec!["dev".to_string()])); + assert_eq!(lock_groups(), vec!["dev".to_string()]); + + // Update with an empty groups list must PRESERVE the existing group. + service + .add_from_origin(origin, AddMode::Update, vec![]) + .await + .expect("update should succeed"); + assert_eq!( + groups_in_manifest(), + Some(vec!["dev".to_string()]), + "update with empty groups must preserve existing groups" + ); + assert_eq!(lock_groups(), vec!["dev".to_string()]); + } + #[tokio::test] #[allow(clippy::await_holding_lock)] async fn test_add_from_origin_fresh_conflict() { @@ -857,11 +942,13 @@ mod tests { editable: false, }; service - .add_from_origin(origin.clone(), AddMode::Fresh) + .add_from_origin(origin.clone(), AddMode::Fresh, vec![]) .await .expect("first add should succeed"); - let result = service.add_from_origin(origin, AddMode::Fresh).await; + let result = service + .add_from_origin(origin, AddMode::Fresh, vec![]) + .await; assert!(matches!(result, Err(ServiceError::AlreadyIndexed(_)))); } @@ -880,7 +967,7 @@ mod tests { editable: false, }; service - .add_from_origin(origin.clone(), AddMode::Fresh) + .add_from_origin(origin.clone(), AddMode::Fresh, vec![]) .await .expect("first add should succeed"); @@ -892,7 +979,7 @@ mod tests { .unwrap(); let outcome = service - .add_from_origin(origin, AddMode::Update) + .add_from_origin(origin, AddMode::Update, vec![]) .await .expect("update should succeed"); assert_eq!(outcome.resolved.version, "2.0.0"); @@ -923,7 +1010,7 @@ mod tests { editable: true, }; let outcome = service - .add_from_origin(origin, AddMode::Fresh) + .add_from_origin(origin, AddMode::Fresh, vec![]) .await .expect("editable add should succeed"); @@ -941,7 +1028,9 @@ mod tests { path: tmp.path().join("does-not-exist"), editable: false, }; - let result = service.add_from_origin(origin, AddMode::Fresh).await; + let result = service + .add_from_origin(origin, AddMode::Fresh, vec![]) + .await; assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); } @@ -985,7 +1074,7 @@ mod tests { url: format!("{}/pkg.zip", server.uri()), }; let outcome = service - .add_from_origin(origin, AddMode::Fresh) + .add_from_origin(origin, AddMode::Fresh, vec![]) .await .expect("zip-url add should succeed"); assert_eq!(outcome.id, "test-skill"); @@ -1005,7 +1094,9 @@ mod tests { skill: "scope/skill".to_string(), version: None, }; - let result = service.add_from_origin(origin, AddMode::Fresh).await; + let result = service + .add_from_origin(origin, AddMode::Fresh, vec![]) + .await; assert!(matches!(result, Err(ServiceError::Config(_)))); } @@ -1022,7 +1113,9 @@ mod tests { r#ref: GitRef::Commit("deadbeef".to_string()), subdir: None, }; - let result = service.add_from_origin(origin, AddMode::Fresh).await; + let result = service + .add_from_origin(origin, AddMode::Fresh, vec![]) + .await; assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); } diff --git a/crates/fastskill-core/src/http/handlers/reindex.rs b/crates/fastskill-core/src/http/handlers/reindex.rs index 2bba1c3..d482356 100644 --- a/crates/fastskill-core/src/http/handlers/reindex.rs +++ b/crates/fastskill-core/src/http/handlers/reindex.rs @@ -1,49 +1,55 @@ //! Reindex endpoint handlers //! //! PARTIAL-6 / ADR-0003: these endpoints mutate the embedding index and are -//! therefore write-gated (only mounted with `--enable-write`). The real reindex -//! implementation (`execute_reindex`) lives in the `fastskill-cli` crate and -//! depends on CLI-only pieces (OpenAI key loading, progress reporting), so it is -//! NOT cleanly reachable from `fastskill-core`. Rather than returning a fake -//! `200` with mock counts, these handlers honestly return `501 Not Implemented` -//! and direct callers to `fastskill reindex` on the CLI. Wiring a core-level -//! reindex service is tracked separately (spec 003 / browser skill management). +//! therefore write-gated (only mounted with `--enable-write`). They call the +//! core reindex seam (`FastSkillService::reindex`, ADR-0002/0005) directly with +//! no observer (HTTP has no progress-streaming channel). Per ADR-0002, when no +//! embedding provider is configured the seam skips silently — that is surfaced +//! here as `200` with `reindexed: false` and a `reason`, not an error. +//! +//! The core seam has no single-skill mode (it always walks the whole skills +//! directory); rather than growing the core seam for an HTTP-only convenience, +//! `POST /reindex/{id}` reindexes the *whole* index, same as `POST /reindex`. +//! This is a deliberate, documented simplification, not an oversight. +use crate::http::errors::HttpResult; use crate::http::handlers::AppState; use crate::http::models::*; use axum::{ extract::{Path, State}, - http::StatusCode, - response::{IntoResponse, Response}, Json, }; -fn not_implemented() -> Response { - ( - StatusCode::NOT_IMPLEMENTED, - Json(ApiResponse::::error(ErrorResponse { - code: "NOT_IMPLEMENTED".to_string(), - message: "reindex via HTTP is not implemented; run `fastskill reindex` from the CLI" - .to_string(), - details: None, - })), - ) - .into_response() +fn outcome_response( + outcome: crate::core::reindex::ReindexOutcome, +) -> axum::Json> { + Json(ApiResponse::success(ReindexOutcomeResponse { + reindexed: outcome.reindexed, + count: outcome.count, + reason: outcome.reason, + })) } -/// POST /api/reindex - Reindex all skills +/// POST /api/v1/reindex - Reindex all skills (skips silently, 200, when no +/// embedding provider is configured; ADR-0002). pub async fn reindex_all( - State(_state): State, + State(state): State, Json(_request): Json, -) -> Response { - not_implemented() +) -> HttpResult>> { + let outcome = state.service.reindex(None, None).await?; + Ok(outcome_response(outcome)) } -/// POST /api/reindex/{id} - Reindex specific skill +/// POST /api/v1/reindex/{id} - Reindex a single skill. +/// +/// The core reindex seam has no single-skill mode (see module docs); `id` is +/// accepted for URL/API-contract compatibility but the whole index is +/// reindexed, same as `POST /reindex`. pub async fn reindex_skill( - State(_state): State, + State(state): State, Path(_skill_id): Path, Json(_request): Json, -) -> Response { - not_implemented() +) -> HttpResult>> { + let outcome = state.service.reindex(None, None).await?; + Ok(outcome_response(outcome)) } diff --git a/crates/fastskill-core/src/http/handlers/skills.rs b/crates/fastskill-core/src/http/handlers/skills.rs index 07fc00a..3a37cf4 100644 --- a/crates/fastskill-core/src/http/handlers/skills.rs +++ b/crates/fastskill-core/src/http/handlers/skills.rs @@ -1,19 +1,17 @@ //! Skills CRUD endpoint handlers +use crate::core::install::{AddMode, UpdatePreflight}; +use crate::core::manifest::SkillProjectToml; +use crate::core::service::ServiceError; use crate::http::errors::{HttpError, HttpResult}; use crate::http::handlers::AppState; use crate::http::models::*; use axum::{ extract::{Path, State}, + http::StatusCode, Json, }; -#[derive(serde::Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct UpgradeRequest { - skill_id: Option, -} - fn skill_metadata_json(skill: &crate::core::skill_manager::SkillDefinition) -> serde_json::Value { let origin = serde_json::to_value(&skill.origin).unwrap_or(serde_json::Value::Null); serde_json::json!({ @@ -159,59 +157,131 @@ pub async fn delete_skill( })))) } -/// POST /api/skills/upgrade - Upgrade one or all skills from manifest (shells out to fastskill update) -pub async fn upgrade_skills( +/// POST /api/v1/skills/install - Fresh-install a skill from an [`Origin`] +/// (core install seam, ADR-0005 / spec 003 §2). `AddMode::Fresh` fails with a +/// 409 if the resolved id is already installed; other seam errors map to +/// 400/500 via the blanket `ServiceError` → `HttpError` conversion. +pub async fn install_skill( State(state): State, - Json(payload): Json>, -) -> HttpResult>> { - let project_path = state.project_file_path.clone(); - let filter_id = payload - .and_then(|p| p.skill_id) - .filter(|s| !s.is_empty() && s != "all"); - - // SEC-2: validate a requested skill id against known skills before spawning - // a subprocess, so an arbitrary/attacker-controlled id can't drive an update. - if let Some(ref id) = filter_id { - let known = state.service.skill_manager().list_skills().await?; - let is_known = known.iter().any(|s| s.id.to_string() == *id); - if !is_known { - return Err(HttpError::BadRequest(format!("Unknown skill: {}", id))); + Json(request): Json, +) -> HttpResult<(StatusCode, axum::Json>)> { + match state + .service + .add_from_origin(request.origin, AddMode::Fresh, request.groups) + .await + { + Ok(outcome) => { + let response = InstallSkillResponse { + id: outcome.id, + resolved_version: outcome.resolved.version, + reindexed: outcome.reindexed, + }; + Ok((StatusCode::CREATED, Json(ApiResponse::success(response)))) } + // ADR-0005 §Q6 / spec 003 §2: a Fresh conflict on an already-installed + // id is a 409, not a generic 400 (which the blanket ServiceError→HttpError + // mapping would otherwise give it). + Err(ServiceError::AlreadyIndexed(id)) => Err(HttpError::Conflict(format!( + "Skill '{}' is already installed", + id + ))), + Err(e) => Err(e.into()), } +} - let output = tokio::task::spawn_blocking(move || { - let exe = std::env::current_exe().map_err(|e| { - HttpError::InternalServerError(format!("Failed to get executable path: {}", e)) - })?; - let mut cmd = std::process::Command::new(exe); - cmd.arg("update"); - // SEC-2 (defense in depth): `--` ends option parsing so a validated id - // beginning with `-` can never be read as a flag by `fastskill update`. - if let Some(ref id) = filter_id { - cmd.arg("--"); - cmd.arg(id); +/// POST /api/v1/skills/update (and its back-compat alias `/skills/upgrade`) - +/// update one or all skills recorded in the project's `skill-project.toml` by +/// routing each dependency's recorded `Origin` through the core install seam +/// (ADR-0005), mirroring `fastskill-cli`'s `update` command: `preflight(&origin)` +/// decides Updatable/UpToDate/Immutable, and only `Updatable` entries are +/// re-fetched via `add_from_origin(origin, AddMode::Update, groups)`. +/// `check: true` reports the preflight verdict without applying anything. +/// Always 200 with a per-skill result list — a per-skill failure does not fail +/// the whole request. +pub async fn update_skills( + State(state): State, + Json(payload): Json>, +) -> HttpResult>>> { + let payload = payload.unwrap_or_default(); + let project_path = &state.project_file_path; + + if !project_path.exists() { + return Err(HttpError::NotFound( + "skill-project.toml not found".to_string(), + )); + } + + let project = SkillProjectToml::load_from_file(project_path).map_err(|e| { + HttpError::InternalServerError(format!("Failed to load skill-project.toml: {}", e)) + })?; + + let mut entries = project + .to_skill_entries() + .map_err(HttpError::InternalServerError)?; + entries.sort_by(|a, b| a.id.cmp(&b.id)); + + let filter_id = payload + .skill_id + .as_deref() + .filter(|s| !s.is_empty() && *s != "all"); + if let Some(id) = filter_id { + entries.retain(|e| e.id == id); + if entries.is_empty() { + return Err(HttpError::NotFound(format!("Unknown skill: {}", id))); } - if let Some(parent) = project_path.parent() { - cmd.current_dir(parent); + } + + let mut results = Vec::with_capacity(entries.len()); + for entry in entries { + match state.service.preflight(&entry.origin).await { + Ok(UpdatePreflight::Updatable) if payload.check => { + results.push(SkillUpdateResult { + id: entry.id, + outcome: "would_update".to_string(), + reason: None, + resolved_version: None, + }); + } + Ok(UpdatePreflight::Updatable) => { + match state + .service + .add_from_origin(entry.origin.clone(), AddMode::Update, entry.groups.clone()) + .await + { + Ok(outcome) => results.push(SkillUpdateResult { + id: entry.id, + outcome: "updated".to_string(), + reason: None, + resolved_version: Some(outcome.resolved.version), + }), + Err(e) => results.push(SkillUpdateResult { + id: entry.id, + outcome: "error".to_string(), + reason: Some(e.to_string()), + resolved_version: None, + }), + } + } + Ok(UpdatePreflight::UpToDate) => results.push(SkillUpdateResult { + id: entry.id, + outcome: "up_to_date".to_string(), + reason: None, + resolved_version: None, + }), + Ok(UpdatePreflight::Immutable { reason }) => results.push(SkillUpdateResult { + id: entry.id, + outcome: "immutable".to_string(), + reason: Some(reason), + resolved_version: None, + }), + Err(e) => results.push(SkillUpdateResult { + id: entry.id, + outcome: "error".to_string(), + reason: Some(e.to_string()), + resolved_version: None, + }), } - let output = cmd - .output() - .map_err(|e| HttpError::InternalServerError(format!("Failed to run update: {}", e)))?; - Ok::<_, HttpError>(output) - }) - .await - .map_err(|e| HttpError::InternalServerError(format!("Upgrade task failed: {}", e)))??; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(HttpError::InternalServerError(format!( - "Upgrade failed: {}", - stderr.trim() - ))); } - Ok(axum::Json(ApiResponse::success(serde_json::json!({ - "message": "Upgrade completed", - "upgraded": true - })))) + Ok(axum::Json(ApiResponse::success(results))) } diff --git a/crates/fastskill-core/src/http/handlers/status.rs b/crates/fastskill-core/src/http/handlers/status.rs index a3b1038..1d12453 100644 --- a/crates/fastskill-core/src/http/handlers/status.rs +++ b/crates/fastskill-core/src/http/handlers/status.rs @@ -275,6 +275,8 @@ pub async fn status( storage_path: config.skill_storage_path.to_string_lossy().to_string(), hot_reload_enabled: config.hot_reload.enabled, uptime_seconds: state.uptime_seconds(), + writable: state.enable_write, + embedding_provider: state.service.embedding_service().is_some(), }; Ok(axum::Json(ApiResponse::success(response))) diff --git a/crates/fastskill-core/src/http/models.rs b/crates/fastskill-core/src/http/models.rs index 1dc042b..6643456 100644 --- a/crates/fastskill-core/src/http/models.rs +++ b/crates/fastskill-core/src/http/models.rs @@ -144,6 +144,17 @@ pub struct ReindexResponse { pub duration_ms: u64, } +/// Outcome of a call into the core reindex seam (ADR-0002/0005). `reindexed: +/// false` + a `reason` means the reindex was skipped (e.g. no embedding +/// provider configured) — a success, not a failure. +#[derive(Debug, Serialize, Clone)] +#[serde(rename_all = "camelCase")] +pub struct ReindexOutcomeResponse { + pub reindexed: bool, + pub count: usize, + pub reason: Option, +} + /// Status response #[derive(Debug, Serialize, Clone)] #[serde(rename_all = "camelCase")] @@ -154,6 +165,11 @@ pub struct StatusResponse { pub storage_path: String, pub hot_reload_enabled: bool, pub uptime_seconds: u64, + /// Whether write (mutating) endpoints are enabled on this server instance. + pub writable: bool, + /// Whether an embedding provider is injected (reindex/semantic search + /// available rather than skipping silently / falling back to keyword search). + pub embedding_provider: bool, } /// Source response for registry @@ -232,3 +248,45 @@ pub struct UpdateSkillRequest { pub editable: Option, pub version: Option, } + +/// POST /api/v1/skills/install request body: a fresh install from an [`Origin`] +/// (core install seam, ADR-0005). `Origin` deserializes directly (internally +/// tagged by `type`). +#[derive(Debug, Deserialize, Clone)] +#[serde(rename_all = "camelCase")] +pub struct InstallSkillRequest { + pub origin: crate::core::origin::Origin, + #[serde(default)] + pub groups: Vec, +} + +/// POST /api/v1/skills/install response (201) / success shape. +#[derive(Debug, Serialize, Clone)] +#[serde(rename_all = "camelCase")] +pub struct InstallSkillResponse { + pub id: String, + pub resolved_version: String, + pub reindexed: bool, +} + +/// POST /api/v1/skills/update request body. `skill_id` omitted (or `"all"`) +/// updates every skill recorded in the project; `check` reports the preflight +/// verdict for each without applying anything. +#[derive(Debug, Deserialize, Clone, Default)] +#[serde(rename_all = "camelCase")] +pub struct UpdateSkillsRequest { + pub skill_id: Option, + #[serde(default)] + pub check: bool, +} + +/// Per-skill outcome of a `POST /api/v1/skills/update` call. +#[derive(Debug, Serialize, Clone)] +#[serde(rename_all = "camelCase")] +pub struct SkillUpdateResult { + pub id: String, + /// One of: `"updated"`, `"would_update"`, `"up_to_date"`, `"immutable"`, `"error"`. + pub outcome: String, + pub reason: Option, + pub resolved_version: Option, +} diff --git a/crates/fastskill-core/src/http/server.rs b/crates/fastskill-core/src/http/server.rs index 8791814..e40a311 100644 --- a/crates/fastskill-core/src/http/server.rs +++ b/crates/fastskill-core/src/http/server.rs @@ -309,13 +309,16 @@ impl FastSkillServer { /// /// These paths are ALWAYS registered but wrapped in the write-gate middleware /// so they return 403 (not 404) when `--enable-write` is off. Includes: - /// delete/upgrade skills, reindex, registry refresh, and manifest mutators. - /// (`POST /skills` create + `PUT /skills/{id}` field-edit removed per - /// PARTIAL-1 / spec 003.) + /// install/update/delete skills, reindex, registry refresh, and manifest + /// mutators. (`POST /skills` create + `PUT /skills/{id}` field-edit removed + /// per PARTIAL-1 / spec 003.) `/skills/upgrade` is kept mounted alongside + /// `/skills/update` as a back-compat alias (spec 003 §2) — same handler. fn create_write_routes_v1() -> Router { Router::new() .route("/skills/{id}", delete(skills::delete_skill)) - .route("/skills/upgrade", post(skills::upgrade_skills)) + .route("/skills/install", post(skills::install_skill)) + .route("/skills/update", post(skills::update_skills)) + .route("/skills/upgrade", post(skills::update_skills)) .route("/reindex", post(reindex::reindex_all)) .route("/reindex/{id}", post(reindex::reindex_skill)) .route("/registry/refresh", post(registry::refresh_sources)) diff --git a/crates/fastskill-core/tests/http_handler_route_tests.rs b/crates/fastskill-core/tests/http_handler_route_tests.rs index 6c938c5..b0a0e69 100644 --- a/crates/fastskill-core/tests/http_handler_route_tests.rs +++ b/crates/fastskill-core/tests/http_handler_route_tests.rs @@ -99,13 +99,71 @@ async fn fixture_with_skills(enable_write: bool) -> Fixture { } } +/// A fixture for the install/update seam (spec 003 Phase 2): a temp project +/// with a valid `skill-project.toml` (so `resolve_project_file` succeeds), and +/// the *service's* `project_root` injected — mirroring what `serve`'s edge +/// wiring does — so `add_from_origin`'s manifest/lock writes land in the temp +/// project rather than falling back to the test process's cwd. +async fn fixture_for_install(enable_write: bool) -> Fixture { + let storage = TempDir::new().unwrap(); + let store = skills_root(&storage); + + let project = TempDir::new().unwrap(); + let project_file_path = project.path().join("skill-project.toml"); + fs::write( + &project_file_path, + format!( + "[tool.fastskill]\nskills_directory = \"{}\"\n\n[dependencies]\n", + store.display() + ), + ) + .unwrap(); + + let config = ServiceConfig { + skill_storage_path: store.clone(), + ..Default::default() + }; + let mut svc = FastSkillService::new(config).await.unwrap(); + svc.initialize().await.unwrap(); + let svc = svc.with_project_root(project.path().to_path_buf()); + let service = Arc::new(svc); + + let mut state = AppState::new(service).unwrap(); + state.project_file_path = project_file_path.clone(); + state.project_root = project.path().to_path_buf(); + state.skills_directory = store; + state.enable_write = enable_write; + + Fixture { + _storage: storage, + _project: project, + state, + project_file_path, + } +} + +/// Write a standalone skill directory (NOT under the service's storage root) +/// suitable for use as an `Origin::Local` install source. +fn write_source_skill(root: &std::path::Path, name: &str) -> PathBuf { + let dir = root.join(name); + fs::create_dir_all(&dir).unwrap(); + fs::write( + dir.join("SKILL.md"), + format!("---\nname: {name}\nversion: \"1.0.0\"\ndescription: A test skill\n---\nBody\n"), + ) + .unwrap(); + dir +} + /// Full router mirroring the production route table (paths without the /api/v1 prefix). fn router(state: AppState) -> Router { Router::new() .route("/skills", get(skills::list_skills)) .route("/skills/{id}", get(skills::get_skill)) .route("/skills/{id}", delete(skills::delete_skill)) - .route("/skills/upgrade", post(skills::upgrade_skills)) + .route("/skills/install", post(skills::install_skill)) + .route("/skills/update", post(skills::update_skills)) + .route("/skills/upgrade", post(skills::update_skills)) .route("/project", get(manifest::get_project)) .route("/manifest/skills", get(manifest::list_manifest_skills)) .route("/manifest/skills", post(manifest::add_skill_to_manifest)) @@ -253,51 +311,122 @@ async fn delete_skill_success_with_project_and_lock() { } #[tokio::test] -async fn upgrade_rejects_unknown_skill() { - let f = fixture_with_skills(true).await; +async fn install_fresh_returns_201() { + let f = fixture_for_install(true).await; + let src = write_source_skill(f._project.path(), "new-skill"); + let (status, body) = post_json( + f.state, + "/skills/install", + serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}), + ) + .await; + assert_eq!(status, StatusCode::CREATED, "body: {body}"); + assert!(body.contains("new-skill"), "body: {body}"); + assert!(body.contains("resolvedVersion"), "body: {body}"); +} + +#[tokio::test] +async fn install_fresh_conflict_is_409() { + let f = fixture_for_install(true).await; + let src = write_source_skill(f._project.path(), "dup-skill"); + let payload = serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}); + + let (status1, body1) = post_json(f.state.clone(), "/skills/install", payload.clone()).await; + assert_eq!(status1, StatusCode::CREATED, "body: {body1}"); + + let (status2, body2) = post_json(f.state, "/skills/install", payload).await; + assert_eq!(status2, StatusCode::CONFLICT, "body: {body2}"); + assert!(body2.contains("dup-skill"), "body: {body2}"); +} + +#[tokio::test] +async fn install_invalid_operation_is_400() { + // A nonexistent local path is an InvalidOperation, mapped to 400. + let f = fixture_for_install(true).await; + let missing = f._project.path().join("does-not-exist"); let (status, _b) = post_json( f.state, - "/skills/upgrade", - serde_json::json!({"skillId": "ghost"}), + "/skills/install", + serde_json::json!({"origin": {"type": "local", "path": missing.to_string_lossy()}}), ) .await; assert_eq!(status, StatusCode::BAD_REQUEST); } #[tokio::test] -async fn upgrade_all_runs_subprocess_branch() { - // skillId "all" -> filter_id None -> no id validation, spawns the (test) binary. - // We only assert it is NOT a 400 validation rejection; the subprocess outcome - // (200 success or 500 failure) depends on the environment. +async fn update_missing_manifest_is_404() { + // fixture_with_skills never writes a skill-project.toml. let f = fixture_with_skills(true).await; + let (status, _b) = post_json(f.state, "/skills/update", serde_json::json!({})).await; + assert_eq!(status, StatusCode::NOT_FOUND); +} + +#[tokio::test] +async fn update_unknown_skill_id_is_404() { + let f = fixture_for_install(true).await; let (status, _b) = post_json( f.state, - "/skills/upgrade", - serde_json::json!({"skillId": "all"}), + "/skills/update", + serde_json::json!({"skillId": "ghost"}), ) .await; - assert_ne!(status, StatusCode::BAD_REQUEST); + assert_eq!(status, StatusCode::NOT_FOUND); } #[tokio::test] -async fn upgrade_known_skill_passes_validation() { - // A known id passes the SEC-2 known-skill check and reaches the subprocess. - let f = fixture_with_skills(true).await; - let (status, _b) = post_json( +async fn update_empty_deps_returns_empty_list() { + let f = fixture_for_install(true).await; + let (status, body) = post_json(f.state, "/skills/update", serde_json::json!({})).await; + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("\"data\":[]"), "body: {body}"); +} + +#[tokio::test] +async fn update_check_mode_reports_would_update_without_applying() { + let f = fixture_for_install(true).await; + let src = write_source_skill(f._project.path(), "chk-skill"); + let (install_status, install_body) = post_json( + f.state.clone(), + "/skills/install", + serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}), + ) + .await; + assert_eq!(install_status, StatusCode::CREATED, "body: {install_body}"); + + let (status, body) = post_json( f.state, - "/skills/upgrade", - serde_json::json!({"skillId": "alpha-skill"}), + "/skills/update", + serde_json::json!({"check": true}), ) .await; - assert_ne!(status, StatusCode::BAD_REQUEST); + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("would_update"), "body: {body}"); } #[tokio::test] -async fn upgrade_empty_body_is_none_filter() { - // JSON null body -> Option None -> filter_id None. - let f = fixture_with_skills(true).await; - let (status, _b) = post_json(f.state, "/skills/upgrade", serde_json::Value::Null).await; - assert_ne!(status, StatusCode::BAD_REQUEST); +async fn update_applies_updatable_origin() { + let f = fixture_for_install(true).await; + let src = write_source_skill(f._project.path(), "upd-skill"); + let (install_status, install_body) = post_json( + f.state.clone(), + "/skills/install", + serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}), + ) + .await; + assert_eq!(install_status, StatusCode::CREATED, "body: {install_body}"); + + let (status, body) = post_json(f.state, "/skills/update", serde_json::json!({})).await; + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("\"outcome\":\"updated\""), "body: {body}"); +} + +#[tokio::test] +async fn update_upgrade_alias_route_behaves_identically() { + // Back-compat: /skills/upgrade is the same handler as /skills/update. + let f = fixture_for_install(true).await; + let (status, body) = post_json(f.state, "/skills/upgrade", serde_json::json!({})).await; + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("\"data\":[]"), "body: {body}"); } // --------------------------------------------------------------------------- @@ -313,6 +442,25 @@ async fn status_endpoint_ok() { assert!(body.contains("skillsCount")); } +#[tokio::test] +async fn status_reports_capability_flags() { + // enable_write:true + no embedding provider configured (fixture_with_skills + // never sets `embedding` on the ServiceConfig). + let f = fixture_with_skills(true).await; + let (status, body) = do_get(f.state, "/status").await; + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("\"writable\":true"), "body: {body}"); + assert!(body.contains("\"embeddingProvider\":false"), "body: {body}"); +} + +#[tokio::test] +async fn status_writable_false_when_write_disabled() { + let f = fixture_with_skills(false).await; + let (status, body) = do_get(f.state, "/status").await; + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("\"writable\":false"), "body: {body}"); +} + #[tokio::test] async fn dashboard_lists_skills() { let f = fixture_with_skills(false).await; @@ -363,18 +511,22 @@ async fn dashboard_escapes_xss_and_truncates_over_ten() { // --------------------------------------------------------------------------- #[tokio::test] -async fn reindex_all_is_501() { +async fn reindex_all_skips_without_embedding_provider() { + // fixture_with_skills configures no embedding provider -> reindex skips + // silently (ADR-0002): 200, reindexed:false, with a reason. let f = fixture_with_skills(true).await; let (status, body) = post_json(f.state, "/reindex", serde_json::json!({})).await; - assert_eq!(status, StatusCode::NOT_IMPLEMENTED); - assert!(body.contains("not implemented")); + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("\"reindexed\":false"), "body: {body}"); + assert!(body.contains("\"reason\":"), "body: {body}"); } #[tokio::test] -async fn reindex_skill_is_501() { +async fn reindex_skill_skips_without_embedding_provider() { let f = fixture_with_skills(true).await; - let (status, _b) = post_json(f.state, "/reindex/alpha-skill", serde_json::json!({})).await; - assert_eq!(status, StatusCode::NOT_IMPLEMENTED); + let (status, body) = post_json(f.state, "/reindex/alpha-skill", serde_json::json!({})).await; + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("\"reindexed\":false"), "body: {body}"); } // --------------------------------------------------------------------------- diff --git a/crates/fastskill-core/tests/write_gate_tests.rs b/crates/fastskill-core/tests/write_gate_tests.rs index b3f50e9..655fa00 100644 --- a/crates/fastskill-core/tests/write_gate_tests.rs +++ b/crates/fastskill-core/tests/write_gate_tests.rs @@ -76,6 +76,23 @@ async fn write_route_returns_403_when_write_disabled() { "403 body should point at --enable-write, got: {body}" ); + // Also cover the other new write routes (spec 003 Phase 2): install/update. + let install_resp = client + .post(format!("http://127.0.0.1:{port}/api/v1/skills/install")) + .json(&serde_json::json!({"origin": {"type": "local", "path": "/tmp/does-not-matter"}})) + .send() + .await + .expect("POST /api/v1/skills/install"); + assert_eq!(install_resp.status(), reqwest::StatusCode::FORBIDDEN); + + let update_resp = client + .post(format!("http://127.0.0.1:{port}/api/v1/skills/update")) + .json(&serde_json::json!({})) + .send() + .await + .expect("POST /api/v1/skills/update"); + assert_eq!(update_resp.status(), reqwest::StatusCode::FORBIDDEN); + handle.abort(); } @@ -93,8 +110,9 @@ async fn write_route_not_forbidden_when_write_enabled() { assert!(wait_for_port(port, 10), "server failed to start"); let client = reqwest::Client::new(); - // reindex is write-gated; with writes enabled it must NOT be 403. - // (PARTIAL-6: the HTTP reindex path is honestly 501, not a fake 200.) + // reindex is write-gated; with writes enabled it must NOT be 403. The + // service here has no embedding provider configured, so the core reindex + // seam skips silently (ADR-0002): 200 with reindexed:false, not an error. let resp = client .post(format!("http://127.0.0.1:{port}/api/v1/reindex")) .json(&serde_json::json!({})) @@ -102,7 +120,9 @@ async fn write_route_not_forbidden_when_write_enabled() { .await .expect("POST /api/v1/reindex"); assert_ne!(resp.status(), reqwest::StatusCode::FORBIDDEN); - assert_eq!(resp.status(), reqwest::StatusCode::NOT_IMPLEMENTED); + assert_eq!(resp.status(), reqwest::StatusCode::OK); + let body = resp.text().await.unwrap(); + assert!(body.contains("\"reindexed\":false"), "body: {body}"); handle.abort(); } @@ -136,10 +156,27 @@ async fn read_route_available_when_write_disabled() { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn upgrade_rejects_unknown_skill_id() { - // SEC-2: an unknown/attacker-controlled skillId must be rejected with 400 - // BEFORE any subprocess is spawned. The skill store is empty here, so any - // id (including a leading-dash one) is unknown. +#[allow(clippy::await_holding_lock)] +async fn update_rejects_unknown_skill_id() { + // /skills/update (and its /skills/upgrade alias) route each dependency's + // recorded Origin through the core install seam; an unknown/attacker- + // controlled skillId not present in skill-project.toml's [dependencies] + // must be rejected with 404 rather than silently updating everything. + let _lock = fastskill_core::test_utils::DIR_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); + let project_dir = TempDir::new().unwrap(); + let original_dir = std::env::current_dir().ok(); + let _guard = fastskill_core::test_utils::DirGuard(original_dir); + std::env::set_current_dir(project_dir.path()).unwrap(); + std::fs::write( + project_dir.path().join("skill-project.toml"), + "[tool.fastskill]\nskills_directory = \".claude/skills\"\n\n\ + [dependencies]\nknown-skill = \"1.0.0\"\n", + ) + .unwrap(); + std::fs::create_dir_all(project_dir.path().join(".claude/skills")).unwrap(); + let dir = TempDir::new().unwrap(); let Some(port) = free_port() else { return; @@ -160,8 +197,8 @@ async fn upgrade_rejects_unknown_skill_id() { .expect("POST /api/v1/skills/upgrade"); assert_eq!( resp.status(), - reqwest::StatusCode::BAD_REQUEST, - "unknown skillId must be rejected with 400" + reqwest::StatusCode::NOT_FOUND, + "unknown skillId must be rejected with 404" ); handle.abort(); diff --git a/webdocs/cli-reference/serve-command.mdx b/webdocs/cli-reference/serve-command.mdx index cc7fade..c739d70 100644 --- a/webdocs/cli-reference/serve-command.mdx +++ b/webdocs/cli-reference/serve-command.mdx @@ -106,16 +106,17 @@ Write endpoints (marked **write**) require `--enable-write`; without it they ret | Endpoint | Method | Access | Description | |----------|--------|--------|-------------| -| `/api/v1/status` | GET | read | Service status and uptime | +| `/api/v1/status` | GET | read | Service status and uptime, plus capability flags: `writable` (server started with `--enable-write`) and `embeddingProvider` (an embedding provider is configured) | | `/api/v1/project` | GET | read | Project view from `skill-project.toml` | | `/api/v1/skills` | GET | read | List installed skills | | `/api/v1/skills/{id}` | GET | read | Get a skill | | `/api/v1/skills/{id}` | DELETE | **write** | Remove a skill | -| `/api/v1/skills/upgrade` | POST | **write** | Update skills from their source | +| `/api/v1/skills/install` | POST | **write** | Install a skill from an origin (`{ "origin": {...}, "groups"?: [...] }`); `201` on success, `409` if the id is already installed | +| `/api/v1/skills/update` | POST | **write** | Update one (`{ "skillId": "..." }`) or all skills recorded in the project from their recorded origin; `{ "check": true }` reports what would change without applying it. `/api/v1/skills/upgrade` is kept mounted as a back-compat alias for this same endpoint. | | `/api/v1/search` | POST | read | Search skills | | `/api/v1/resolve` | POST | read | Resolve context for a prompt | -| `/api/v1/reindex` | POST | **write** | Reindex (returns 501 — run `fastskill reindex` from the CLI) | -| `/api/v1/reindex/{id}` | POST | **write** | Reindex a skill (returns 501) | +| `/api/v1/reindex` | POST | **write** | Reindex all skills. Returns `200` with `{ reindexed, count, reason }`; when no embedding provider is configured, reindex skips silently (`reindexed: false` + a `reason`), which is still `200`, not an error. | +| `/api/v1/reindex/{id}` | POST | **write** | Reindexes the whole index (the core reindex seam has no single-skill mode); same response shape as `/api/v1/reindex`. | | `/api/v1/registry/sources` | GET | read | List registry sources | | `/api/v1/registry/refresh` | POST | **write** | Refresh registry sources | | `/api/v1/manifest/skills` | GET | read | List manifest skills |