diff --git a/Cargo.lock b/Cargo.lock index 0527e00a7..5c6a491a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -567,6 +567,7 @@ dependencies = [ "regex", "serde", "serde_json", + "tempfile", "thiserror 2.0.18", "tokio", "tracing", diff --git a/crates/aionui-ai-agent/src/capability/skill_manager/mod.rs b/crates/aionui-ai-agent/src/capability/skill_manager/mod.rs index 1329ba9c0..596383234 100644 --- a/crates/aionui-ai-agent/src/capability/skill_manager/mod.rs +++ b/crates/aionui-ai-agent/src/capability/skill_manager/mod.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::{Arc, LazyLock}; +use aionui_db::ISkillRepository; use regex::Regex; use tokio::sync::RwLock; use tracing::{debug, warn}; @@ -52,6 +53,9 @@ pub struct AcpSkillManager { /// Consumed by `discover_skills` / `get_skill` (Task 4 / 5 of the refactor). #[allow(dead_code)] paths: Arc, + /// User skill state source. When absent, discovery falls back to legacy + /// path-based listing for unit tests that do not stand up a database. + skill_repo: Option>, } impl AcpSkillManager { @@ -60,6 +64,16 @@ impl AcpSkillManager { cache: RwLock::new(HashMap::new()), discovered: RwLock::new(false), paths, + skill_repo: None, + }) + } + + pub fn new_with_repo(paths: Arc, skill_repo: Arc) -> Arc { + Arc::new(Self { + cache: RwLock::new(HashMap::new()), + discovered: RwLock::new(false), + paths, + skill_repo: Some(skill_repo), }) } @@ -68,8 +82,9 @@ impl AcpSkillManager { /// Filtering rules: /// - Auto-inject builtin skills (under `auto-inject/` in the corpus) are /// always included unless listed in `exclude_builtin_skills`. - /// - Opt-in builtin skills (siblings of `auto-inject/`) and custom/extension - /// skills are included only if `enabled_skills` contains their name. + /// - Opt-in builtin skills (siblings of `auto-inject/`) and custom/cron/ + /// extension skills are included only if `enabled_skills` contains their + /// name. /// /// Populates the cache; subsequent `get_skill(name)` calls read body lazily. pub async fn discover_skills( @@ -77,7 +92,7 @@ impl AcpSkillManager { enabled_skills: Option<&[String]>, exclude_builtin_skills: Option<&[String]>, ) -> Vec { - let items = match aionui_extension::list_available_skills(&self.paths).await { + let items = match self.list_available_skills().await { Ok(v) => v, Err(e) => { warn!(error = %e, "Failed to list skills via extension service"); @@ -102,7 +117,9 @@ impl AcpSkillManager { enabled_skills.is_some_and(|en| en.iter().any(|n| n == &item.name)) } } - aionui_extension::SkillSource::Custom | aionui_extension::SkillSource::Extension => { + aionui_extension::SkillSource::Custom + | aionui_extension::SkillSource::Cron + | aionui_extension::SkillSource::Extension => { enabled_skills.is_some_and(|en| en.iter().any(|n| n == &item.name)) } }; @@ -150,7 +167,7 @@ impl AcpSkillManager { *discovered = true; return Vec::new(); } - let items = match aionui_extension::list_available_skills(&self.paths).await { + let items = match self.list_available_skills().await { Ok(v) => v, Err(e) => { warn!(error = %e, "discover_by_names: list_available_skills failed"); @@ -188,6 +205,16 @@ impl AcpSkillManager { .collect() } + async fn list_available_skills( + &self, + ) -> Result, aionui_extension::ExtensionError> { + if let Some(repo) = &self.skill_repo { + aionui_extension::list_available_skills_with_repo(&self.paths, repo.as_ref()).await + } else { + aionui_extension::list_available_skills(&self.paths).await + } + } + /// Return the current skill index without re-scanning. pub async fn get_skills_index(&self) -> Vec { let cache = self.cache.read().await; @@ -205,7 +232,7 @@ impl AcpSkillManager { /// Returns `None` if the skill is unknown. On first access the body is /// read via the appropriate channel based on `source`: /// - `Builtin` → `aionui_extension::read_builtin_skill(&paths, relative)` - /// - `Custom` / `Extension` → direct `tokio::fs::read_to_string(location/SKILL.md)` + /// - `Custom` / `Cron` / `Extension` → direct `tokio::fs::read_to_string(location/SKILL.md)` pub async fn get_skill(&self, name: &str) -> Option { // Fast path: check if body is already cached { @@ -239,7 +266,9 @@ impl AcpSkillManager { String::new() } } - aionui_extension::SkillSource::Custom | aionui_extension::SkillSource::Extension => { + aionui_extension::SkillSource::Custom + | aionui_extension::SkillSource::Cron + | aionui_extension::SkillSource::Extension => { // `location` for scanned user skills is the directory; append SKILL.md. let skill_file = if def.location.is_dir() { def.location.join("SKILL.md") diff --git a/crates/aionui-api-types/src/lib.rs b/crates/aionui-api-types/src/lib.rs index 10f03f8c0..1bc821629 100644 --- a/crates/aionui-api-types/src/lib.rs +++ b/crates/aionui-api-types/src/lib.rs @@ -140,12 +140,13 @@ pub use shell::{ SpeechToTextProvider, SpeechToTextResult, SttStreamClientMessage, SttStreamServerMessage, ToolType, }; pub use skill::{ - AddExternalPathRequest, BuiltinAutoSkillResponse, DeleteSkillRequest, ExportSkillRequest, - ExternalSkillSourceResponse, ImportSkillRequest, ImportSkillResponse, MaterializeSkillsRequest, + AddExternalPathRequest, DeleteSkillRequest, ExportSkillRequest, ExternalSkillSourceResponse, + ImportSkillFailureResponse, ImportSkillRequest, ImportSkillResponse, MaterializeSkillsRequest, MaterializeSkillsResponse, MaterializedSkillRef, NamedPathResponse, ReadAssistantRuleRequest, ReadBuiltinResourceRequest, ReadSkillInfoRequest, ReadSkillInfoResponse, RemoveExternalPathRequest, - ScanForSkillsRequest, ScanForSkillsResponse, ScannedSkillResponse, SkillListItemResponse, SkillPathsResponse, - SkillSourceResponse, WriteAssistantRuleRequest, + ScanForSkillsRequest, ScanForSkillsResponse, ScannedSkillResponse, SkillImportLimitsResponse, + SkillImportRecordResponse, SkillListItemResponse, SkillPathsResponse, SkillSourceResponse, + WriteAssistantRuleRequest, }; pub use system::{ ClientPreferencesResponse, SystemSettingsResponse, UpdateClientPreferencesRequest, UpdateSettingsRequest, diff --git a/crates/aionui-api-types/src/skill.rs b/crates/aionui-api-types/src/skill.rs index 2981cb054..e94638647 100644 --- a/crates/aionui-api-types/src/skill.rs +++ b/crates/aionui-api-types/src/skill.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize}; // A. Skill list & info // --------------------------------------------------------------------------- -/// Origin of a listed skill — `builtin`, `custom`, or `extension`. +/// Origin of a listed skill — `builtin`, `custom`, `cron`, or `extension`. /// /// Matches the renderer contract in /// `src/common/adapter/ipcBridge.ts::listAvailableSkills`. @@ -13,6 +13,7 @@ use serde::{Deserialize, Serialize}; pub enum SkillSourceResponse { Builtin, Custom, + Cron, Extension, } @@ -35,17 +36,6 @@ pub struct SkillListItemResponse { pub source: SkillSourceResponse, } -/// An auto-injected built-in skill (`GET /api/skills/builtin-auto`). -/// -/// `location` is the relative path the frontend passes back into -/// `POST /api/skills/builtin-skill` (e.g. `"auto-inject/cron/SKILL.md"`). -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -pub struct BuiltinAutoSkillResponse { - pub name: String, - pub description: String, - pub location: String, -} - /// Request body for `POST /api/skills/info`. #[derive(Debug, Clone, Deserialize)] pub struct ReadSkillInfoRequest { @@ -75,6 +65,54 @@ pub struct ImportSkillResponse { pub skill_name: String, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub skill_names: Vec, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub failed: Vec, +} + +/// Per-skill failure summary for batch skill import operations. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct ImportSkillFailureResponse { + pub source_name: String, + pub code: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub error_path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub actual_bytes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub limit_bytes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub line: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub column: Option, +} + +/// One row in the skill import history. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct SkillImportRecordResponse { + pub id: String, + pub operation_id: String, + pub source_label: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub source_path: Option, + pub source_name: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub skill_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub skill_name: Option, + pub status: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub error_code: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub error_path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub actual_bytes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub limit_bytes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub line: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub column: Option, + pub created_at: i64, } /// Request body for `POST /api/skills/export-symlink`. @@ -143,6 +181,13 @@ pub struct SkillPathsResponse { pub builtin_skills_dir: String, } +/// Response for `GET /api/skills/import-limits`. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct SkillImportLimitsResponse { + pub max_file_bytes: u64, + pub max_total_bytes: u64, +} + // --------------------------------------------------------------------------- // D. Assistant rules & skills // --------------------------------------------------------------------------- @@ -366,6 +411,10 @@ mod tests { serde_json::to_value(SkillSourceResponse::Custom).unwrap(), serde_json::json!("custom") ); + assert_eq!( + serde_json::to_value(SkillSourceResponse::Cron).unwrap(), + serde_json::json!("cron") + ); assert_eq!( serde_json::to_value(SkillSourceResponse::Extension).unwrap(), serde_json::json!("extension") @@ -408,10 +457,29 @@ mod tests { let resp = ImportSkillResponse { skill_name: "imported-skill".into(), skill_names: vec!["imported-skill".into(), "second-skill".into()], + failed: vec![ImportSkillFailureResponse { + source_name: "bad-skill".into(), + code: "SKILL_IMPORT_FILE_TOO_LARGE".into(), + error_path: Some("assets/movie.mp4".into()), + actual_bytes: Some(20), + limit_bytes: Some(10), + line: None, + column: None, + }], }; let json = serde_json::to_value(&resp).unwrap(); assert_eq!(json["skill_name"], "imported-skill"); assert_eq!(json["skill_names"], json!(["imported-skill", "second-skill"])); + assert_eq!( + json["failed"], + json!([{ + "source_name": "bad-skill", + "code": "SKILL_IMPORT_FILE_TOO_LARGE", + "error_path": "assets/movie.mp4", + "actual_bytes": 20, + "limit_bytes": 10 + }]) + ); assert!(json.get("skillName").is_none()); } diff --git a/crates/aionui-app/assets/builtin-skills/aionui-config/SKILL.md b/crates/aionui-app/assets/builtin-skills/aionui-config/SKILL.md index 049676f84..5ab36ea71 100644 --- a/crates/aionui-app/assets/builtin-skills/aionui-config/SKILL.md +++ b/crates/aionui-app/assets/builtin-skills/aionui-config/SKILL.md @@ -217,8 +217,8 @@ python3 scripts/aionui_api.py get /api/skills/paths ### Import a skill into the registry `POST /api/skills/import` copies a skill folder into the user skills dir and -registers it. `import-symlink` links it instead (good for skills you keep editing -in an external repo). +registers it. It also accepts a parent folder containing multiple skills or a +zip package. ```bash python3 scripts/aionui_api.py post /api/skills/import '{"skill_path":"/abs/path/to/skill-folder"}' diff --git a/crates/aionui-app/src/router/routes.rs b/crates/aionui-app/src/router/routes.rs index f1b1b6b5f..7779e2ced 100644 --- a/crates/aionui-app/src/router/routes.rs +++ b/crates/aionui-app/src/router/routes.rs @@ -23,6 +23,7 @@ use aionui_auth::{ use aionui_channel::channel_routes; #[cfg(feature = "weixin")] use aionui_channel::weixin_login_route; +use aionui_common::ApiErrorLogContext; use aionui_conversation::{conversation_ops_routes, conversation_routes}; use aionui_cron::cron_routes; use aionui_extension::{extension_routes, hub_routes, skill_routes}; @@ -312,6 +313,10 @@ async fn normalize_boundary_error_response(request: Request, next: Next) -> Resp let original_headers = response.headers().clone(); let mut normalized = (status, Json(ErrorResponse::new(error, code))).into_response(); + normalized.extensions_mut().insert(ApiErrorLogContext { + code, + message: error.to_owned(), + }); for (name, value) in original_headers.iter() { if *name != header::CONTENT_TYPE && *name != header::CONTENT_LENGTH { normalized.headers_mut().insert(name, value.clone()); diff --git a/crates/aionui-app/src/router/state.rs b/crates/aionui-app/src/router/state.rs index 84af628de..42a852173 100644 --- a/crates/aionui-app/src/router/state.rs +++ b/crates/aionui-app/src/router/state.rs @@ -510,6 +510,7 @@ pub async fn build_channel_state( ); let skill_resolver = Arc::new(aionui_conversation::skill_resolver::ExtensionSkillResolver::new( services.skill_paths.clone(), + services.skill_repo.clone(), )); let agent_metadata_repo: Arc = Arc::new( aionui_db::SqliteAgentMetadataRepository::new(services.database.pool().clone()), @@ -639,6 +640,7 @@ pub fn build_cron_state(services: &AppServices) -> CronRouterState { let acp_session_repo: Arc = Arc::new(SqliteAcpSessionRepository::new(pool)); let skill_resolver = Arc::new(aionui_conversation::skill_resolver::ExtensionSkillResolver::new( services.skill_paths.clone(), + services.skill_repo.clone(), )); let conv_service = ConversationService::new( services.work_dir.clone(), @@ -766,13 +768,6 @@ pub async fn build_extension_states( let index_manager = HubIndexManager::new(hub_dir, registry.clone()); let installer = HubInstaller::new(index_manager.clone(), registry.clone()); - let app_resource_dir = std::env::current_exe() - .ok() - .and_then(|p| p.canonicalize().ok()) - .and_then(|p| p.parent().map(|pp| pp.to_path_buf())) - .unwrap_or_else(|| std::path::PathBuf::from(".")); - let skill_paths = aionui_extension::resolve_skill_paths(&app_resource_dir, &skill_data_dir); - let ext_paths_mgr = Arc::new(ExternalPathsManager::new(&skill_data_dir).await); let ext_state = ExtensionRouterState { @@ -785,7 +780,8 @@ pub async fn build_extension_states( }; let skill_state = SkillRouterState { - skill_paths, + skill_paths: services.skill_paths.as_ref().clone(), + skill_repo: services.skill_repo.clone(), external_paths_manager: ext_paths_mgr, assistant_dispatcher: None, }; diff --git a/crates/aionui-app/src/router/trace.rs b/crates/aionui-app/src/router/trace.rs index 9950d2cfd..cbd4e7ca8 100644 --- a/crates/aionui-app/src/router/trace.rs +++ b/crates/aionui-app/src/router/trace.rs @@ -1,41 +1,101 @@ //! HTTP request access-log layer. +use std::time::Instant; + +use aionui_common::{ApiErrorLogContext, generate_short_id}; use axum::Router; -use tower_http::trace::TraceLayer; +use axum::extract::Request; +use axum::middleware::{self, Next}; +use axum::response::Response; + +const REQUEST_ID_HEADER: &str = "x-request-id"; +const MAX_QUERY_KEYS: usize = 16; pub(super) fn with_access_log(router: Router) -> Router { - router.layer( - TraceLayer::new_for_http() - .make_span_with(|req: &axum::http::Request<_>| { - tracing::info_span!( - "http", - method = %req.method(), - path = %req.uri().path(), - ) - }) - .on_response( - |res: &axum::http::Response<_>, latency: std::time::Duration, _span: &tracing::Span| { - let status = res.status().as_u16(); - let latency_ms = latency.as_millis() as u64; - if status >= 500 { - tracing::error!(status, latency_ms, "response"); - } else if status >= 400 { - tracing::warn!(status, latency_ms, "response"); - } else { - tracing::info!(status, latency_ms, "response"); - } - }, - ) - .on_failure( - |error: tower_http::classify::ServerErrorsFailureClass, - latency: std::time::Duration, - _span: &tracing::Span| { - tracing::error!( - %error, - latency_ms = latency.as_millis() as u64, - "request failed" - ); - }, - ), - ) + router.layer(middleware::from_fn(access_log)) +} + +async fn access_log(request: Request, next: Next) -> Response { + let started = Instant::now(); + let method = request.method().clone(); + let path = request.uri().path().to_owned(); + let query_keys = query_keys(request.uri().query()); + let request_id = request + .headers() + .get(REQUEST_ID_HEADER) + .and_then(|value| value.to_str().ok()) + .filter(|value| !value.trim().is_empty()) + .map(str::to_owned) + .unwrap_or_else(generate_short_id); + + let response = next.run(request).await; + let status = response.status().as_u16(); + let latency_ms = started.elapsed().as_millis() as u64; + let error_context = response.extensions().get::(); + let error_code = error_context.map(|context| context.code).unwrap_or(""); + let error_message = error_context.map(|context| context.message.as_str()).unwrap_or(""); + + if status >= 500 { + tracing::error!( + request_id = %request_id, + method = %method, + path = %path, + query_keys = %query_keys, + status, + latency_ms, + error_code, + error_message, + "http response" + ); + } else if status >= 400 { + tracing::warn!( + request_id = %request_id, + method = %method, + path = %path, + query_keys = %query_keys, + status, + latency_ms, + error_code, + error_message, + "http response" + ); + } else { + tracing::info!( + request_id = %request_id, + method = %method, + path = %path, + query_keys = %query_keys, + status, + latency_ms, + "http response" + ); + } + + response +} + +fn query_keys(query: Option<&str>) -> String { + query + .into_iter() + .flat_map(|query| query.split('&')) + .filter_map(|pair| { + let key = pair.split_once('=').map_or(pair, |(key, _)| key).trim(); + (!key.is_empty()).then_some(key) + }) + .take(MAX_QUERY_KEYS) + .collect::>() + .join(",") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn query_keys_omit_values() { + assert_eq!( + query_keys(Some("path=/Users/alice/project&token=secret&flag&empty=")), + "path,token,flag,empty" + ); + } } diff --git a/crates/aionui-app/src/services.rs b/crates/aionui-app/src/services.rs index 53d7f7962..766ae3508 100644 --- a/crates/aionui-app/src/services.rs +++ b/crates/aionui-app/src/services.rs @@ -13,9 +13,10 @@ use aionui_common::OnConversationDelete; use aionui_conversation::{ConversationService, runtime_state::ConversationRuntimeStateService}; use aionui_db::{ Database, IAcpSessionRepository, IAgentMetadataRepository, IConversationRepository, IMcpServerRepository, - IUserRepository, SqliteAcpSessionRepository, SqliteAgentMetadataRepository, SqliteAssistantDefinitionRepository, - SqliteAssistantOverlayRepository, SqliteAssistantPreferenceRepository, SqliteConversationRepository, - SqliteMcpServerRepository, SqliteProviderRepository, SqliteUserRepository, + ISkillRepository, IUserRepository, SqliteAcpSessionRepository, SqliteAgentMetadataRepository, + SqliteAssistantDefinitionRepository, SqliteAssistantOverlayRepository, SqliteAssistantPreferenceRepository, + SqliteConversationRepository, SqliteMcpServerRepository, SqliteProviderRepository, SqliteSkillRepository, + SqliteUserRepository, }; use aionui_realtime::{BroadcastEventBus, WebSocketManager}; use aionui_team::GuideMcpServer; @@ -51,6 +52,8 @@ pub struct AppServices { /// Resolved skill paths. Shared with the `ConversationService` for /// snapshot resolution at create time. pub skill_paths: Arc, + /// User skill metadata and import history repository. + pub skill_repo: Arc, /// Guide MCP server config (port, token, binary_path). /// `None` when the server failed to start (graceful degradation). pub guide_mcp_config: Option, @@ -69,6 +72,7 @@ impl AppServices { work_dir: self.work_dir.clone(), event_bus: self.event_bus.clone(), skill_paths: self.skill_paths.clone(), + skill_repo: self.skill_repo.clone(), worker_task_manager: self.worker_task_manager.clone(), conversation_runtime_state: self.conversation_runtime_state.clone(), conversation_repo: self.conversation_repo.clone(), @@ -139,6 +143,7 @@ impl AppServices { let conversation_repo: Arc = Arc::new(SqliteConversationRepository::new(database.pool().clone())); + let skill_repo: Arc = Arc::new(SqliteSkillRepository::new(database.pool().clone())); // Skill paths need app resource dir (for builtin rules) + data dir // (for user skills + materialized views). AcpSkillManager uses these @@ -149,6 +154,9 @@ impl AppServices { .and_then(|p| p.parent().map(|pp| pp.to_path_buf())) .unwrap_or_else(|| std::path::PathBuf::from(".")); let skill_paths = Arc::new(aionui_extension::resolve_skill_paths(&app_resource_dir, &data_dir)); + aionui_extension::sync_skill_catalog_into_repo(skill_paths.as_ref(), skill_repo.as_ref()) + .await + .map_err(|e| anyhow::anyhow!("Failed to synchronize skill catalog: {e}"))?; // Absolute path to this process's binary. Reused as the `command` for // the stdio MCP bridge spawned by ACP CLIs when a team session is @@ -180,7 +188,7 @@ impl AppServices { }; let factory = build_agent_factory(AgentFactoryDeps { - skill_manager: AcpSkillManager::new(skill_paths.clone()), + skill_manager: AcpSkillManager::new_with_repo(skill_paths.clone(), skill_repo.clone()), provider_repo, encryption_key, agent_registry: agent_registry.clone(), @@ -204,6 +212,7 @@ impl AppServices { work_dir: work_dir.clone(), event_bus: event_bus.clone(), skill_paths: skill_paths.clone(), + skill_repo: skill_repo.clone(), worker_task_manager: worker_task_manager.clone(), conversation_runtime_state: conversation_runtime_state.clone(), conversation_repo: conversation_repo.clone(), @@ -231,6 +240,7 @@ impl AppServices { local, app_version, skill_paths, + skill_repo, guide_mcp_config: guide_mcp_config.clone(), _guide_server: guide_server, }) @@ -242,6 +252,7 @@ struct ConversationServiceDeps<'a> { work_dir: PathBuf, event_bus: Arc, skill_paths: Arc, + skill_repo: Arc, worker_task_manager: Arc, conversation_runtime_state: Arc, conversation_repo: Arc, @@ -251,6 +262,7 @@ struct ConversationServiceDeps<'a> { fn build_conversation_service(deps: ConversationServiceDeps<'_>) -> ConversationService { let skill_resolver = Arc::new(aionui_conversation::skill_resolver::ExtensionSkillResolver::new( deps.skill_paths, + deps.skill_repo, )); let service = ConversationService::new( deps.work_dir, diff --git a/crates/aionui-app/tests/assistants_e2e.rs b/crates/aionui-app/tests/assistants_e2e.rs index 2a74c02ec..365b98838 100644 --- a/crates/aionui-app/tests/assistants_e2e.rs +++ b/crates/aionui-app/tests/assistants_e2e.rs @@ -299,6 +299,7 @@ async fn fixture() -> Fixture { }; states.skill = SkillRouterState { skill_paths, + skill_repo: std::sync::Arc::new(aionui_db::SqliteSkillRepository::new(services.database.pool().clone())), external_paths_manager: ext_paths_mgr, assistant_dispatcher: None, // wired below once service is constructed }; diff --git a/crates/aionui-app/tests/common/mod.rs b/crates/aionui-app/tests/common/mod.rs index 42686181c..57c044969 100644 --- a/crates/aionui-app/tests/common/mod.rs +++ b/crates/aionui-app/tests/common/mod.rs @@ -20,16 +20,30 @@ pub async fn build_app() -> (axum::Router, AppServices) { (router, services) } -/// Build an app whose skill router reads from the given temp directories. +/// Build an app whose skill router uses the given temp directories. /// /// Use for HTTP integration tests that need deterministic on-disk layouts -/// (E1 `/api/skills`, E2 `/api/skills/builtin-auto`, E3/E4 built-in reads, -/// E5 `/api/skills/info`). Returns the router, services, and the -/// `SkillPaths` so the test can seed fixtures at known locations. +/// (E1 `/api/skills`, E3/E4 built-in reads, E5 `/api/skills/info`). +/// Returns the router, services, and the `SkillPaths` so the test can seed +/// fixtures at known locations. `/api/skills` reads the database only; tests +/// that seed skill directories should call `sync_skill_catalog_for_test`. #[allow(dead_code)] pub async fn build_app_with_skill_paths(root: &std::path::Path) -> (axum::Router, AppServices, SkillPaths) { let db = aionui_db::init_database_memory().await.unwrap(); - let services = AppServices::from_config(db, &AppConfig::default()).await.unwrap(); + let config = AppConfig { + data_dir: root.to_path_buf(), + work_dir: root.to_path_buf(), + ..Default::default() + }; + let services = AppServices::from_config(db, &config).await.unwrap(); + sqlx::query("DELETE FROM skill_import_records") + .execute(services.database.pool()) + .await + .unwrap(); + sqlx::query("DELETE FROM skills") + .execute(services.database.pool()) + .await + .unwrap(); let (mut states, _) = build_module_states(&services).await.expect("build module states"); let builtin_dir = root.join("builtin-skills"); @@ -55,6 +69,7 @@ pub async fn build_app_with_skill_paths(root: &std::path::Path) -> (axum::Router let ext_paths_mgr = std::sync::Arc::new(ExternalPathsManager::with_file(root.join("paths.json")).await); states.skill = SkillRouterState { skill_paths: paths.clone(), + skill_repo: std::sync::Arc::new(aionui_db::SqliteSkillRepository::new(services.database.pool().clone())), external_paths_manager: ext_paths_mgr, assistant_dispatcher: states.skill.assistant_dispatcher.clone(), }; @@ -63,6 +78,13 @@ pub async fn build_app_with_skill_paths(root: &std::path::Path) -> (axum::Router (router, services, paths) } +pub async fn sync_skill_catalog_for_test(services: &AppServices, paths: &SkillPaths) { + let repo = aionui_db::SqliteSkillRepository::new(services.database.pool().clone()); + aionui_extension::sync_skill_catalog_into_repo(paths, &repo) + .await + .expect("sync skill catalog"); +} + pub async fn build_app_with_noop_opener() -> (axum::Router, AppServices) { let db = aionui_db::init_database_memory().await.unwrap(); let services = AppServices::from_config(db, &AppConfig::default()).await.unwrap(); diff --git a/crates/aionui-app/tests/extension_e2e.rs b/crates/aionui-app/tests/extension_e2e.rs index 2d1fdbedd..af0b473b8 100644 --- a/crates/aionui-app/tests/extension_e2e.rs +++ b/crates/aionui-app/tests/extension_e2e.rs @@ -10,7 +10,10 @@ use aionui_common::{decrypt_string, now_ms}; use aionui_db::{IChannelRepository, SqliteChannelRepository}; use aionui_extension::{ExtensionSource, ScanPath}; -use common::{body_json, build_app, build_app_with_skill_paths, get_with_token, json_with_token, setup_and_login}; +use common::{ + body_json, build_app, build_app_with_skill_paths, get_with_token, json_with_token, setup_and_login, + sync_skill_catalog_for_test, +}; fn write_legacy_extension_fixture(tmp: &TempDir) -> std::path::PathBuf { let ext_root = tmp.path().join("extensions"); @@ -345,6 +348,7 @@ async fn eq12_get_i18n_for_locale() { let (token, csrf) = setup_and_login(&mut app, &services, "user1", "pass1").await; let resp = app + .clone() .oneshot(json_with_token( "POST", "/api/extensions/i18n", @@ -372,6 +376,7 @@ async fn eq13_permissions_not_found() { let (token, csrf) = setup_and_login(&mut app, &services, "user1", "pass1").await; let resp = app + .clone() .oneshot(json_with_token( "POST", "/api/extensions/permissions", @@ -1203,6 +1208,120 @@ async fn si3_read_skill_info_returns_not_found_for_missing_path() { assert_eq!(json["success"], false); } +#[tokio::test] +async fn skill_import_returns_specific_code_for_oversized_file() { + let tmp = TempDir::new().unwrap(); + let (mut app, services, _paths) = build_app_with_skill_paths(tmp.path()).await; + let (token, csrf) = setup_and_login(&mut app, &services, "user1", "pass1").await; + + let skill_dir = tmp.path().join("huge-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: huge-skill\ndescription: Huge skill\n---\nBody", + ) + .unwrap(); + std::fs::write(skill_dir.join("movie.bin"), vec![0u8; 10 * 1024 * 1024 + 1]).unwrap(); + + let resp = app + .oneshot(json_with_token( + "POST", + "/api/skills/import", + json!({ "skill_path": skill_dir.to_str().unwrap() }), + &token, + &csrf, + )) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + let json = body_json(resp).await; + assert_eq!(json["success"], true); + assert!(json["data"].get("skill_names").is_none()); + assert_eq!( + json["data"]["failed"], + json!([{ + "source_name": "huge-skill", + "code": "SKILL_IMPORT_FILE_TOO_LARGE", + "error_path": "movie.bin", + "actual_bytes": 10 * 1024 * 1024 + 1, + "limit_bytes": 10 * 1024 * 1024 + }]) + ); +} + +#[tokio::test] +async fn skill_batch_import_reports_partial_failures_without_rolling_back_successes() { + let tmp = TempDir::new().unwrap(); + let (mut app, services, paths) = build_app_with_skill_paths(tmp.path()).await; + let (token, csrf) = setup_and_login(&mut app, &services, "user1", "pass1").await; + + let parent_dir = tmp.path().join("parent-pack"); + let alpha_dir = parent_dir.join("alpha-skill"); + let beta_dir = parent_dir.join("beta-skill"); + std::fs::create_dir_all(&alpha_dir).unwrap(); + std::fs::create_dir_all(&beta_dir).unwrap(); + std::fs::write( + alpha_dir.join("SKILL.md"), + "---\nname: sample-alpha\ndescription: Alpha skill\n---\nBody", + ) + .unwrap(); + std::fs::write( + beta_dir.join("SKILL.md"), + "---\nname: sample-beta\ndescription: Beta skill\n---\nBody", + ) + .unwrap(); + std::fs::write(beta_dir.join("movie.bin"), vec![0u8; 10 * 1024 * 1024 + 1]).unwrap(); + + let resp = app + .clone() + .oneshot(json_with_token( + "POST", + "/api/skills/import", + json!({ "skill_path": parent_dir.to_str().unwrap() }), + &token, + &csrf, + )) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + let json = body_json(resp).await; + assert_eq!(json["success"], true); + assert_eq!(json["data"]["skill_names"], json!(["sample-alpha"])); + assert_eq!( + json["data"]["failed"], + json!([{ + "source_name": "beta-skill", + "code": "SKILL_IMPORT_FILE_TOO_LARGE", + "error_path": "movie.bin", + "actual_bytes": 10 * 1024 * 1024 + 1, + "limit_bytes": 10 * 1024 * 1024 + }]) + ); + assert!(paths.user_skills_dir.join("sample-alpha").join("SKILL.md").exists()); + assert!(!paths.user_skills_dir.join("sample-beta").exists()); + + let resp = app + .oneshot(get_with_token("/api/skills/import-history", &token)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let json = body_json(resp).await; + let records = json["data"].as_array().unwrap(); + assert!( + records.iter().any(|record| { + record["source_name"] == "beta-skill" + && record["status"] == "failed" + && record["error_code"] == "SKILL_IMPORT_FILE_TOO_LARGE" + && record["error_path"] == "movie.bin" + && record["actual_bytes"] == 10 * 1024 * 1024 + 1 + && record["limit_bytes"] == 10 * 1024 * 1024 + }), + "history records = {records:?}" + ); +} + // --------------------------------------------------------------------------- // SL — Skill listing (E1 / `GET /api/skills`) // --------------------------------------------------------------------------- @@ -1223,6 +1342,7 @@ async fn sl1_list_skills_tags_builtin_and_custom_with_source_field() { let builtin_dir = paths.builtin_skills_dir.clone(); write_skill(&builtin_dir, "review", "Built-in review skill"); write_skill(&paths.user_skills_dir, "my-skill", "A user-imported skill"); + sync_skill_catalog_for_test(&services, &paths).await; let resp = app.oneshot(get_with_token("/api/skills", &token)).await.unwrap(); assert_eq!(resp.status(), StatusCode::OK); @@ -1259,6 +1379,7 @@ async fn sl2_list_skills_user_custom_overrides_builtin() { let builtin_dir = paths.builtin_skills_dir.clone(); write_skill(&builtin_dir, "review", "Built-in review"); write_skill(&paths.user_skills_dir, "review", "Custom review override"); + sync_skill_catalog_for_test(&services, &paths).await; let resp = app.oneshot(get_with_token("/api/skills", &token)).await.unwrap(); assert_eq!(resp.status(), StatusCode::OK); @@ -1285,11 +1406,11 @@ async fn sl3_list_skills_returns_empty_array_when_no_skills() { } // --------------------------------------------------------------------------- -// BA — Built-in auto skills (E2 / `GET /api/skills/builtin-auto`) +// BA — Auto-inject builtins through unified `GET /api/skills` // --------------------------------------------------------------------------- #[tokio::test] -async fn ba1_auto_skills_lists_underscore_builtin_entries() { +async fn ba1_unified_skill_list_includes_auto_inject_builtin_entries() { let tmp = TempDir::new().unwrap(); let (mut app, services, paths) = build_app_with_skill_paths(tmp.path()).await; let (token, _csrf) = setup_and_login(&mut app, &services, "user1", "pass1").await; @@ -1300,40 +1421,40 @@ async fn ba1_auto_skills_lists_underscore_builtin_entries() { write_skill(&auto_dir, "skill-creator", "Scaffold a new skill"); // A top-level builtin that must NOT appear in the auto list. write_skill(&builtin_dir, "review", "Top-level"); + sync_skill_catalog_for_test(&services, &paths).await; - let resp = app - .oneshot(get_with_token("/api/skills/builtin-auto", &token)) - .await - .unwrap(); + let resp = app.oneshot(get_with_token("/api/skills", &token)).await.unwrap(); assert_eq!(resp.status(), StatusCode::OK); let json = body_json(resp).await; assert_eq!(json["success"], true); let arr = json["data"].as_array().unwrap(); - assert_eq!(arr.len(), 2); - let names: std::collections::HashSet<_> = arr.iter().map(|v| v["name"].as_str().unwrap()).collect(); - assert!(names.contains("cron")); - assert!(names.contains("skill-creator")); - assert!(!names.contains("review")); - // Must be `{ name, description, location }` — no path / is_custom leak. - for item in arr { - assert!(item.get("path").is_none()); - assert!(item.get("is_custom").is_none()); - assert!(item.get("is_custom").is_none()); - assert!(item["description"].is_string()); - } + let by_name: std::collections::HashMap<_, _> = arr + .iter() + .map(|v| (v["name"].as_str().unwrap().to_owned(), v.clone())) + .collect(); + + let cron = &by_name["cron"]; + assert_eq!(cron["source"], "builtin"); + assert_eq!(cron["relative_location"], "auto-inject/cron/SKILL.md"); + assert_eq!(cron["description"], "Schedule recurring tasks"); + + let skill_creator = &by_name["skill-creator"]; + assert_eq!(skill_creator["source"], "builtin"); + assert_eq!(skill_creator["relative_location"], "auto-inject/skill-creator/SKILL.md"); + + let review = &by_name["review"]; + assert_eq!(review["source"], "builtin"); + assert_eq!(review["relative_location"], "review/SKILL.md"); } #[tokio::test] -async fn ba2_auto_skills_returns_empty_array_when_subdir_missing() { +async fn ba2_unified_skill_list_excludes_missing_auto_inject_dir() { let tmp = TempDir::new().unwrap(); let (mut app, services, _paths) = build_app_with_skill_paths(tmp.path()).await; let (token, _csrf) = setup_and_login(&mut app, &services, "user1", "pass1").await; - let resp = app - .oneshot(get_with_token("/api/skills/builtin-auto", &token)) - .await - .unwrap(); + let resp = app.oneshot(get_with_token("/api/skills", &token)).await.unwrap(); assert_eq!(resp.status(), StatusCode::OK); let json = body_json(resp).await; @@ -1342,15 +1463,14 @@ async fn ba2_auto_skills_returns_empty_array_when_subdir_missing() { } #[tokio::test] -async fn ba3_auto_skills_unauthenticated_rejected() { - let (app, _) = build_app().await; +async fn ba3_builtin_auto_skill_list_get_is_not_registered() { + let (mut app, services) = build_app().await; + let (token, _csrf) = setup_and_login(&mut app, &services, "user1", "pass1").await; let resp = app - .oneshot(common::get_request("/api/skills/builtin-auto")) + .oneshot(get_with_token("/api/skills/builtin-auto", &token)) .await .unwrap(); - assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); - let json = body_json(resp).await; - assert_eq!(json["code"], "UNAUTHORIZED"); + assert_eq!(resp.status(), StatusCode::METHOD_NOT_ALLOWED); } // --------------------------------------------------------------------------- diff --git a/crates/aionui-app/tests/skills_builtin_e2e.rs b/crates/aionui-app/tests/skills_builtin_e2e.rs index e82dd6dac..e9243ec29 100644 --- a/crates/aionui-app/tests/skills_builtin_e2e.rs +++ b/crates/aionui-app/tests/skills_builtin_e2e.rs @@ -1,6 +1,6 @@ //! HTTP integration tests for the built-in skills migration surface: -//! `/api/skills/builtin-auto`, `/api/skills/builtin-skill`, `/api/skills`, -//! and the symlink-contract `/api/skills/materialize-for-agent` (POST). +//! `/api/skills/builtin-skill`, `/api/skills`, and the symlink-contract +//! `/api/skills/materialize-for-agent` (POST). //! //! Covers the spec's §9.2 scenarios end-to-end through //! `aionui_app::create_router_with_states` against an in-memory DB. @@ -73,11 +73,16 @@ async fn fixture_embedded() -> Fixture { assistant_skills_dir: data_dir.join("assistant-skills"), }; let ext_paths_mgr = Arc::new(ExternalPathsManager::with_file(data_dir.join("paths.json")).await); + let skill_repo = Arc::new(aionui_db::SqliteSkillRepository::new(services.database.pool().clone())); states.skill = SkillRouterState { skill_paths, + skill_repo: skill_repo.clone(), external_paths_manager: ext_paths_mgr, assistant_dispatcher: states.skill.assistant_dispatcher.clone(), }; + aionui_extension::sync_skill_catalog_into_repo(&states.skill.skill_paths, skill_repo.as_ref()) + .await + .expect("sync embedded builtin skill catalog"); let mut app = create_router_with_states(&services, states); let (token, csrf) = setup_and_login(&mut app, &services, "builtin-e2e", "StrongP@ss1").await; @@ -91,28 +96,18 @@ async fn fixture_embedded() -> Fixture { } } -fn write_user_skill(dir: &std::path::Path, name: &str, desc: &str) { - let skill_dir = dir.join("skills").join(name); - std::fs::create_dir_all(&skill_dir).unwrap(); - std::fs::write( - skill_dir.join("SKILL.md"), - format!("---\nname: {name}\ndescription: {desc}\n---\nBody for {name}."), - ) - .unwrap(); -} - // =========================================================================== -// GET /api/skills/builtin-auto — embedded corpus +// GET /api/skills — embedded corpus // =========================================================================== #[tokio::test] -async fn builtin_auto_lists_entries_from_embedded_corpus() { +async fn unified_skill_list_includes_auto_inject_entries_from_embedded_corpus() { let fx = fixture_embedded().await; let resp = fx .app .clone() - .oneshot(get_with_token("/api/skills/builtin-auto", &fx.token)) + .oneshot(get_with_token("/api/skills", &fx.token)) .await .unwrap(); assert_eq!(resp.status(), StatusCode::OK); @@ -120,16 +115,29 @@ async fn builtin_auto_lists_entries_from_embedded_corpus() { let json = body_json(resp).await; assert_eq!(json["success"], true); let arr = json["data"].as_array().unwrap(); - assert!(arr.len() >= 3, "expected ≥3 auto-inject entries, got {}", arr.len()); - let names: Vec<&str> = arr.iter().filter_map(|item| item["name"].as_str()).collect(); + let auto_items: Vec<&Value> = arr + .iter() + .filter(|item| { + item["source"] == "builtin" + && item["relative_location"] + .as_str() + .is_some_and(|location| location.starts_with("auto-inject/")) + }) + .collect(); + assert!( + auto_items.len() >= 3, + "expected ≥3 auto-inject entries, got {}", + auto_items.len() + ); + let names: Vec<&str> = auto_items.iter().filter_map(|item| item["name"].as_str()).collect(); assert!( !names.contains(&"aionui-skills"), "aionui-skills should not be shipped as an auto-inject builtin skill: {names:?}", ); - for item in arr { + for item in auto_items { assert!(item["name"].is_string()); assert!(item["description"].is_string()); - let loc = item["location"].as_str().unwrap(); + let loc = item["relative_location"].as_str().unwrap(); assert!(loc.starts_with("auto-inject/"), "location={loc}"); assert!(loc.ends_with("/SKILL.md")); } @@ -241,7 +249,26 @@ async fn list_skills_builtin_entries_carry_relative_location() { let fx = fixture_embedded().await; // Seed one user skill so the merge is non-trivial. - write_user_skill(&fx.data_dir, "my-custom", "Custom skill for test"); + let source_dir = fx.data_dir.join("import-source").join("my-custom"); + std::fs::create_dir_all(&source_dir).unwrap(); + std::fs::write( + source_dir.join("SKILL.md"), + "---\nname: my-custom\ndescription: Custom skill for test\n---\nBody", + ) + .unwrap(); + let resp = fx + .app + .clone() + .oneshot(json_with_token( + "POST", + "/api/skills/import", + json!({ "skill_path": source_dir.to_str().unwrap() }), + &fx.token, + &fx.csrf, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); let resp = fx .app @@ -276,9 +303,16 @@ async fn list_skills_builtin_entries_carry_relative_location() { "custom" => { saw_custom = true; assert!(item.get("relative_location").is_none()); - assert!(item.get("relative_location").is_none()); assert_eq!(item["name"], "my-custom"); } + "cron" => { + assert!(item.get("relative_location").is_none()); + let loc = item["location"].as_str().unwrap(); + assert!( + loc.ends_with("/SKILL.md"), + "cron skill location should point at SKILL.md: {loc}" + ); + } other => panic!("unexpected source: {other}"), } } diff --git a/crates/aionui-common/src/error.rs b/crates/aionui-common/src/error.rs index a1eef364c..11c5840d0 100644 --- a/crates/aionui-common/src/error.rs +++ b/crates/aionui-common/src/error.rs @@ -92,6 +92,13 @@ struct ErrorBody { details: Option, } +/// Safe error metadata attached to API error responses for HTTP access logs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ApiErrorLogContext { + pub code: &'static str, + pub message: String, +} + impl ApiError { pub fn coded( status: StatusCode, @@ -254,13 +261,18 @@ pub fn validate_workspace_path_availability(workspace: &str) -> Result Response { let status = self.status_code(); + let code = self.error_code(); + let message = self.public_message(); + let details = self.error_details(); let body = ErrorBody { success: false, - error: self.public_message(), - code: self.error_code().to_owned(), - details: self.error_details(), + error: message.clone(), + code: code.to_owned(), + details, }; - (status, axum::Json(body)).into_response() + let mut response = (status, axum::Json(body)).into_response(); + response.extensions_mut().insert(ApiErrorLogContext { code, message }); + response } } @@ -425,6 +437,18 @@ mod tests { assert_eq!(json["code"], "NOT_FOUND"); } + #[tokio::test] + async fn api_error_response_carries_log_context_extension() { + let resp = ApiError::BadRequest("invalid skill".into()).into_response(); + let context = resp + .extensions() + .get::() + .expect("ApiError responses should expose log context to the access log layer"); + + assert_eq!(context.code, "BAD_REQUEST"); + assert_eq!(context.message, "invalid skill"); + } + #[tokio::test] async fn public_message_does_not_expose_internal_details() { let cases = [ diff --git a/crates/aionui-common/src/lib.rs b/crates/aionui-common/src/lib.rs index f00a60291..03e10a4a9 100644 --- a/crates/aionui-common/src/lib.rs +++ b/crates/aionui-common/src/lib.rs @@ -21,7 +21,9 @@ pub use enums::{ RemoteAgentProtocol, RemoteAgentStatus, }; #[allow(clippy::disallowed_types)] -pub use error::{ApiError, ErrorChain, WorkspacePathValidationError, validate_workspace_path_availability}; +pub use error::{ + ApiError, ApiErrorLogContext, ErrorChain, WorkspacePathValidationError, validate_workspace_path_availability, +}; pub use hooks::OnConversationDelete; pub use id::{fnv1a_hex8, generate_id, generate_id_with_length, generate_prefixed_id, generate_short_id}; pub use pagination::PaginatedResult; diff --git a/crates/aionui-conversation/Cargo.toml b/crates/aionui-conversation/Cargo.toml index 113dc5272..ee7bf4794 100644 --- a/crates/aionui-conversation/Cargo.toml +++ b/crates/aionui-conversation/Cargo.toml @@ -25,6 +25,7 @@ thiserror.workspace = true [dev-dependencies] tokio = { workspace = true, features = ["test-util"] } +tempfile.workspace = true # Enable the `AgentInstance::Mock` variant so tests can build fake agents # through the trait-object escape hatch without spawning real CLI processes. aionui-ai-agent = { workspace = true, features = ["test-support"] } diff --git a/crates/aionui-conversation/src/skill_resolver.rs b/crates/aionui-conversation/src/skill_resolver.rs index 4819b5672..8b47f4e4e 100644 --- a/crates/aionui-conversation/src/skill_resolver.rs +++ b/crates/aionui-conversation/src/skill_resolver.rs @@ -1,10 +1,11 @@ //! Abstraction over "what are the auto-inject skill names right now?" so //! `ConversationService` can compute the initial snapshot without forcing -//! every test setup to stand up a real `SkillPaths`. +//! every test setup to stand up a real `SkillPaths` and skill repository. use std::path::Path; use std::sync::Arc; +use aionui_db::ISkillRepository; pub use aionui_extension::ResolvedAgentSkill; use async_trait::async_trait; use tracing::warn; @@ -42,11 +43,12 @@ pub trait SkillResolver: Send + Sync { /// Production adapter backed by `aionui_extension::skill_service`. pub struct ExtensionSkillResolver { paths: Arc, + skill_repo: Arc, } impl ExtensionSkillResolver { - pub fn new(paths: Arc) -> Self { - Self { paths } + pub fn new(paths: Arc, skill_repo: Arc) -> Self { + Self { paths, skill_repo } } } @@ -90,16 +92,26 @@ fn extract_skill_body(content: &str) -> String { #[async_trait] impl SkillResolver for ExtensionSkillResolver { async fn auto_inject_names(&self) -> Vec { - match aionui_extension::list_builtin_auto_skills(&self.paths).await { + match aionui_extension::list_available_skills_with_repo(&self.paths, self.skill_repo.as_ref()).await { Ok(items) => { - let mut names: Vec = items.into_iter().map(|i| i.name).collect(); + let mut names: Vec = items + .into_iter() + .filter(|item| { + item.source == aionui_extension::SkillSource::Builtin + && item + .relative_location + .as_deref() + .is_some_and(|location| location.starts_with("auto-inject/")) + }) + .map(|item| item.name) + .collect(); names.sort(); names } Err(e) => { tracing::warn!( error = %e, - "auto_inject_names: list_builtin_auto_skills failed, falling back to empty" + "auto_inject_names: skill catalog lookup failed, falling back to empty" ); Vec::new() } @@ -112,7 +124,14 @@ impl SkillResolver for ExtensionSkillResolver { } // Conversation_id is validated upstream; we don't use a real one here // because this resolver is purely a path-resolution helper. - match aionui_extension::materialize_skills_for_agent(&self.paths, "workspace-link", names).await { + match aionui_extension::materialize_skills_for_agent_with_repo( + &self.paths, + self.skill_repo.as_ref(), + "workspace-link", + names, + ) + .await + { Ok(list) => list, Err(e) => { tracing::warn!( @@ -171,10 +190,52 @@ impl SkillResolver for FixedSkillResolver { #[cfg(test)] mod tests { use super::*; + use aionui_db::SqliteSkillRepository; + + fn write_skill(dir: &Path, name: &str, description: &str) { + let skill_dir = dir.join(name); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {description}\n---\nBody"), + ) + .unwrap(); + } #[test] fn extract_skill_body_removes_frontmatter() { let content = "---\nname: cron\ndescription: Cron\n---\nCron body"; assert_eq!(extract_skill_body(content), "Cron body"); } + + #[tokio::test] + async fn extension_resolver_reads_auto_inject_names_from_skill_catalog() { + let tmp = tempfile::TempDir::new().unwrap(); + let paths = Arc::new(aionui_extension::SkillPaths { + data_dir: tmp.path().to_path_buf(), + user_skills_dir: tmp.path().join("skills"), + cron_skills_dir: tmp.path().join("cron").join("skills"), + builtin_skills_dir: tmp.path().join("builtin-skills"), + builtin_rules_dir: tmp.path().join("builtin-rules"), + assistant_rules_dir: tmp.path().join("assistant-rules"), + assistant_skills_dir: tmp.path().join("assistant-skills"), + }); + write_skill(&paths.builtin_skills_dir, "review", "Top-level builtin"); + write_skill( + &paths.builtin_skills_dir.join("auto-inject"), + "auto-cron", + "Auto-injected builtin", + ); + write_skill(&paths.cron_skills_dir, "scheduled-task", "Cron source skill"); + + let db = aionui_db::init_database_memory().await.unwrap(); + let repo: Arc = Arc::new(SqliteSkillRepository::new(db.pool().clone())); + aionui_extension::sync_skill_catalog_into_repo(paths.as_ref(), repo.as_ref()) + .await + .unwrap(); + + let resolver = ExtensionSkillResolver::new(paths, repo); + + assert_eq!(resolver.auto_inject_names().await, vec!["auto-cron".to_string()]); + } } diff --git a/crates/aionui-db/migrations/014_skill_management.sql b/crates/aionui-db/migrations/014_skill_management.sql new file mode 100644 index 000000000..d717f643d --- /dev/null +++ b/crates/aionui-db/migrations/014_skill_management.sql @@ -0,0 +1,53 @@ +-- Migration 014: skill management metadata and import history +-- +-- `skills` is the source of truth for skill listing and user-managed skill state. +-- Skill files still live on disk under the data directory; the database owns +-- listing, soft deletion, and path lookup semantics. + +CREATE TABLE IF NOT EXISTS skills ( + id TEXT PRIMARY KEY NOT NULL, + name TEXT NOT NULL UNIQUE, + description TEXT, + path TEXT NOT NULL, + source TEXT NOT NULL DEFAULT 'user' + CHECK (source IN ('user', 'builtin', 'extension', 'cron')), + enabled INTEGER NOT NULL DEFAULT 1, + deleted_at INTEGER, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL +); + +CREATE INDEX IF NOT EXISTS idx_skills_deleted_at ON skills(deleted_at); +CREATE INDEX IF NOT EXISTS idx_skills_source ON skills(source); +CREATE INDEX IF NOT EXISTS idx_skills_updated_at ON skills(updated_at DESC); + +CREATE TABLE IF NOT EXISTS skill_import_records ( + id TEXT PRIMARY KEY NOT NULL, + operation_id TEXT NOT NULL, + + source_label TEXT NOT NULL, + source_path TEXT, + source_name TEXT NOT NULL, + + skill_id TEXT, + skill_name TEXT, + + status TEXT NOT NULL + CHECK (status IN ('imported', 'failed', 'overwritten')), + error_code TEXT, + + error_path TEXT, + actual_bytes INTEGER, + limit_bytes INTEGER, + line INTEGER, + column INTEGER, + + created_at INTEGER NOT NULL, + + FOREIGN KEY (skill_id) REFERENCES skills(id) +); + +CREATE INDEX IF NOT EXISTS idx_skill_import_records_operation_id ON skill_import_records(operation_id); +CREATE INDEX IF NOT EXISTS idx_skill_import_records_created_at ON skill_import_records(created_at DESC); +CREATE INDEX IF NOT EXISTS idx_skill_import_records_status ON skill_import_records(status); +CREATE INDEX IF NOT EXISTS idx_skill_import_records_skill_id ON skill_import_records(skill_id); diff --git a/crates/aionui-db/src/lib.rs b/crates/aionui-db/src/lib.rs index 423b676ba..5cf0983aa 100644 --- a/crates/aionui-db/src/lib.rs +++ b/crates/aionui-db/src/lib.rs @@ -19,8 +19,8 @@ pub use error::DbError; pub use models::{ AgentMetadataRow, AssistantDefinitionRow, AssistantOverlayRow, AssistantOverrideRow, AssistantPreferenceRow, AssistantRow, ConversationArtifactRow, ConversationAssistantSnapshotRow, CreateAssistantParams, - UpdateAgentAvailabilitySnapshotParams, UpdateAgentHandshakeParams, UpdateAssistantParams, - UpsertAgentMetadataParams, UpsertAssistantDefinitionParams, UpsertAssistantOverlayParams, + SkillImportRecordRow, SkillRow, UpdateAgentAvailabilitySnapshotParams, UpdateAgentHandshakeParams, + UpdateAssistantParams, UpsertAgentMetadataParams, UpsertAssistantDefinitionParams, UpsertAssistantOverlayParams, UpsertAssistantPreferenceParams, UpsertConversationAssistantSnapshotParams, UpsertOverrideParams, }; pub use repository::channel::UpdatePluginStatusParams; @@ -33,18 +33,20 @@ pub use repository::mcp_server::{CreateMcpServerParams, UpdateMcpServerParams}; pub use repository::oauth_token::UpsertOAuthTokenParams; pub use repository::provider::{CreateProviderParams, UpdateProviderParams}; pub use repository::remote_agent::{CreateRemoteAgentParams, UpdateRemoteAgentParams}; +pub use repository::skill::{CreateSkillImportRecordParams, UpsertSkillParams}; pub use repository::team::{UpdateTaskParams, UpdateTeamParams}; pub use repository::{ CreateAcpSessionParams, IAcpSessionRepository, IAgentMetadataRepository, IAssistantDefinitionRepository, IAssistantOverlayRepository, IAssistantOverrideRepository, IAssistantPreferenceRepository, IAssistantRepository, IChannelRepository, IClientPreferenceRepository, IConversationRepository, ICronRepository, IMcpServerRepository, - IOAuthTokenRepository, IProviderRepository, IRemoteAgentRepository, ISettingsRepository, ITeamRepository, - IUserRepository, PersistedSessionState, SaveRuntimeStateParams, SqliteAcpSessionRepository, + IOAuthTokenRepository, IProviderRepository, IRemoteAgentRepository, ISettingsRepository, ISkillRepository, + ITeamRepository, IUserRepository, PersistedSessionState, SaveRuntimeStateParams, SqliteAcpSessionRepository, SqliteAgentMetadataRepository, SqliteAssistantDefinitionRepository, SqliteAssistantOverlayRepository, SqliteAssistantOverrideRepository, SqliteAssistantPreferenceRepository, SqliteAssistantRepository, SqliteChannelRepository, SqliteClientPreferenceRepository, SqliteConversationRepository, SqliteCronRepository, SqliteMcpServerRepository, SqliteOAuthTokenRepository, SqliteProviderRepository, SqliteRemoteAgentRepository, - SqliteSettingsRepository, SqliteTeamRepository, SqliteUserRepository, rebuild_legacy_assistant_mirror, + SqliteSettingsRepository, SqliteSkillRepository, SqliteTeamRepository, SqliteUserRepository, + rebuild_legacy_assistant_mirror, }; // Re-export sqlx pool type for downstream crates diff --git a/crates/aionui-db/src/models/mod.rs b/crates/aionui-db/src/models/mod.rs index a2f0b9756..076836557 100644 --- a/crates/aionui-db/src/models/mod.rs +++ b/crates/aionui-db/src/models/mod.rs @@ -11,6 +11,7 @@ mod message; mod oauth_token; mod provider; mod remote_agent; +mod skill; mod system_settings; mod team; mod user; @@ -34,6 +35,7 @@ pub use message::MessageRow; pub use oauth_token::OAuthTokenRow; pub use provider::Provider; pub use remote_agent::RemoteAgentRow; +pub use skill::{SkillImportRecordRow, SkillRow}; pub use system_settings::SystemSettings; pub use team::{MailboxMessageRow, TeamRow, TeamTaskRow}; pub use user::User; diff --git a/crates/aionui-db/src/models/skill.rs b/crates/aionui-db/src/models/skill.rs new file mode 100644 index 000000000..70e37295a --- /dev/null +++ b/crates/aionui-db/src/models/skill.rs @@ -0,0 +1,36 @@ +use aionui_common::TimestampMs; +use serde::{Deserialize, Serialize}; + +/// Row mapping for the `skills` table. +#[derive(Debug, Clone, Serialize, Deserialize, sqlx::FromRow)] +pub struct SkillRow { + pub id: String, + pub name: String, + pub description: Option, + pub path: String, + pub source: String, + pub enabled: bool, + pub deleted_at: Option, + pub created_at: TimestampMs, + pub updated_at: TimestampMs, +} + +/// Row mapping for the `skill_import_records` table. +#[derive(Debug, Clone, Serialize, Deserialize, sqlx::FromRow)] +pub struct SkillImportRecordRow { + pub id: String, + pub operation_id: String, + pub source_label: String, + pub source_path: Option, + pub source_name: String, + pub skill_id: Option, + pub skill_name: Option, + pub status: String, + pub error_code: Option, + pub error_path: Option, + pub actual_bytes: Option, + pub limit_bytes: Option, + pub line: Option, + pub column: Option, + pub created_at: TimestampMs, +} diff --git a/crates/aionui-db/src/repository/mod.rs b/crates/aionui-db/src/repository/mod.rs index bb5e5d83e..138bf5d97 100644 --- a/crates/aionui-db/src/repository/mod.rs +++ b/crates/aionui-db/src/repository/mod.rs @@ -10,6 +10,7 @@ pub mod oauth_token; pub mod provider; pub mod remote_agent; mod settings; +pub mod skill; mod sqlite_acp_session; mod sqlite_agent_metadata; mod sqlite_assistant; @@ -22,6 +23,7 @@ mod sqlite_oauth_token; mod sqlite_provider; mod sqlite_remote_agent; mod sqlite_settings; +mod sqlite_skill; mod sqlite_team; mod sqlite_user; pub mod team; @@ -42,6 +44,7 @@ pub use oauth_token::IOAuthTokenRepository; pub use provider::IProviderRepository; pub use remote_agent::IRemoteAgentRepository; pub use settings::ISettingsRepository; +pub use skill::ISkillRepository; pub use sqlite_acp_session::SqliteAcpSessionRepository; pub use sqlite_agent_metadata::SqliteAgentMetadataRepository; pub use sqlite_assistant::{ @@ -57,6 +60,7 @@ pub use sqlite_oauth_token::SqliteOAuthTokenRepository; pub use sqlite_provider::SqliteProviderRepository; pub use sqlite_remote_agent::SqliteRemoteAgentRepository; pub use sqlite_settings::SqliteSettingsRepository; +pub use sqlite_skill::SqliteSkillRepository; pub use sqlite_team::SqliteTeamRepository; pub use sqlite_user::SqliteUserRepository; pub use team::ITeamRepository; diff --git a/crates/aionui-db/src/repository/skill.rs b/crates/aionui-db/src/repository/skill.rs new file mode 100644 index 000000000..1a2c80931 --- /dev/null +++ b/crates/aionui-db/src/repository/skill.rs @@ -0,0 +1,58 @@ +use crate::error::DbError; +use crate::models::{SkillImportRecordRow, SkillRow}; + +/// Skill metadata and import-history data access abstraction. +#[async_trait::async_trait] +pub trait ISkillRepository: Send + Sync { + /// Returns active skills ordered by most recent update first. + async fn list(&self) -> Result, DbError>; + + /// Finds an active skill by name. + async fn find_by_name(&self, name: &str) -> Result, DbError>; + + /// Finds a skill by name, including soft-deleted rows. + async fn find_by_name_any(&self, name: &str) -> Result, DbError>; + + /// Creates or updates a user skill by name and clears soft-delete state. + async fn upsert(&self, params: UpsertSkillParams<'_>) -> Result; + + /// Soft-deletes an active skill by name. + async fn delete_by_name(&self, name: &str) -> Result; + + /// Appends one import record. + async fn create_import_record( + &self, + params: CreateSkillImportRecordParams<'_>, + ) -> Result; + + /// Lists recent import records ordered by creation time descending. + async fn list_import_records(&self, limit: i64) -> Result, DbError>; +} + +/// Parameters for creating or updating a skill row. +#[derive(Debug, Clone)] +pub struct UpsertSkillParams<'a> { + pub name: &'a str, + pub description: Option<&'a str>, + pub path: &'a str, + pub source: &'a str, + pub enabled: bool, +} + +/// Parameters for appending a skill import history row. +#[derive(Debug, Clone)] +pub struct CreateSkillImportRecordParams<'a> { + pub operation_id: &'a str, + pub source_label: &'a str, + pub source_path: Option<&'a str>, + pub source_name: &'a str, + pub skill_id: Option<&'a str>, + pub skill_name: Option<&'a str>, + pub status: &'a str, + pub error_code: Option<&'a str>, + pub error_path: Option<&'a str>, + pub actual_bytes: Option, + pub limit_bytes: Option, + pub line: Option, + pub column: Option, +} diff --git a/crates/aionui-db/src/repository/sqlite_skill.rs b/crates/aionui-db/src/repository/sqlite_skill.rs new file mode 100644 index 000000000..72281d8b3 --- /dev/null +++ b/crates/aionui-db/src/repository/sqlite_skill.rs @@ -0,0 +1,257 @@ +use sqlx::SqlitePool; + +use crate::error::DbError; +use crate::models::{SkillImportRecordRow, SkillRow}; +use crate::repository::skill::{CreateSkillImportRecordParams, ISkillRepository, UpsertSkillParams}; + +/// SQLite-backed implementation of [`ISkillRepository`]. +#[derive(Clone, Debug)] +pub struct SqliteSkillRepository { + pool: SqlitePool, +} + +impl SqliteSkillRepository { + pub fn new(pool: SqlitePool) -> Self { + Self { pool } + } +} + +#[async_trait::async_trait] +impl ISkillRepository for SqliteSkillRepository { + async fn list(&self) -> Result, DbError> { + let rows = sqlx::query_as::<_, SkillRow>( + "SELECT * FROM skills WHERE deleted_at IS NULL AND enabled = 1 ORDER BY updated_at DESC, name ASC", + ) + .fetch_all(&self.pool) + .await?; + Ok(rows) + } + + async fn find_by_name(&self, name: &str) -> Result, DbError> { + let row = + sqlx::query_as::<_, SkillRow>("SELECT * FROM skills WHERE name = ? AND deleted_at IS NULL AND enabled = 1") + .bind(name) + .fetch_optional(&self.pool) + .await?; + Ok(row) + } + + async fn find_by_name_any(&self, name: &str) -> Result, DbError> { + let row = sqlx::query_as::<_, SkillRow>("SELECT * FROM skills WHERE name = ?") + .bind(name) + .fetch_optional(&self.pool) + .await?; + Ok(row) + } + + async fn upsert(&self, params: UpsertSkillParams<'_>) -> Result { + let now = aionui_common::now_ms(); + let existing = self.find_by_name_any(params.name).await?; + let id = existing + .as_ref() + .map(|row| row.id.clone()) + .unwrap_or_else(|| aionui_common::generate_prefixed_id("skill")); + let created_at = existing.as_ref().map(|row| row.created_at).unwrap_or(now); + + sqlx::query( + "INSERT INTO skills \ + (id, name, description, path, source, enabled, deleted_at, created_at, updated_at) \ + VALUES (?, ?, ?, ?, ?, ?, NULL, ?, ?) \ + ON CONFLICT(name) DO UPDATE SET \ + description = excluded.description, \ + path = excluded.path, \ + source = excluded.source, \ + enabled = excluded.enabled, \ + deleted_at = NULL, \ + updated_at = excluded.updated_at", + ) + .bind(&id) + .bind(params.name) + .bind(params.description) + .bind(params.path) + .bind(params.source) + .bind(params.enabled) + .bind(created_at) + .bind(now) + .execute(&self.pool) + .await?; + + self.find_by_name_any(params.name) + .await? + .ok_or_else(|| DbError::NotFound(format!("skill '{}' was not found after upsert", params.name))) + } + + async fn delete_by_name(&self, name: &str) -> Result { + let now = aionui_common::now_ms(); + let result = sqlx::query( + "UPDATE skills SET enabled = 0, deleted_at = ?, updated_at = ? WHERE name = ? AND deleted_at IS NULL", + ) + .bind(now) + .bind(now) + .bind(name) + .execute(&self.pool) + .await?; + + if result.rows_affected() == 0 { + return Err(DbError::NotFound(format!("skill '{name}'"))); + } + + self.find_by_name_any(name) + .await? + .ok_or_else(|| DbError::NotFound(format!("skill '{name}'"))) + } + + async fn create_import_record( + &self, + params: CreateSkillImportRecordParams<'_>, + ) -> Result { + let id = aionui_common::generate_prefixed_id("skill_import"); + let now = aionui_common::now_ms(); + + sqlx::query( + "INSERT INTO skill_import_records \ + (id, operation_id, source_label, source_path, source_name, skill_id, skill_name, \ + status, error_code, error_path, actual_bytes, limit_bytes, line, column, created_at) \ + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + ) + .bind(&id) + .bind(params.operation_id) + .bind(params.source_label) + .bind(params.source_path) + .bind(params.source_name) + .bind(params.skill_id) + .bind(params.skill_name) + .bind(params.status) + .bind(params.error_code) + .bind(params.error_path) + .bind(params.actual_bytes) + .bind(params.limit_bytes) + .bind(params.line) + .bind(params.column) + .bind(now) + .execute(&self.pool) + .await?; + + let row = sqlx::query_as::<_, SkillImportRecordRow>("SELECT * FROM skill_import_records WHERE id = ?") + .bind(&id) + .fetch_one(&self.pool) + .await?; + Ok(row) + } + + async fn list_import_records(&self, limit: i64) -> Result, DbError> { + let rows = sqlx::query_as::<_, SkillImportRecordRow>( + "SELECT * FROM skill_import_records ORDER BY created_at DESC, id DESC LIMIT ?", + ) + .bind(limit.max(0)) + .fetch_all(&self.pool) + .await?; + Ok(rows) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::init_database_memory; + + async fn setup() -> (SqliteSkillRepository, crate::Database) { + let db = init_database_memory().await.unwrap(); + let repo = SqliteSkillRepository::new(db.pool().clone()); + (repo, db) + } + + #[tokio::test] + async fn upsert_restores_soft_deleted_skill() { + let (repo, _db) = setup().await; + + let created = repo + .upsert(UpsertSkillParams { + name: "sample", + description: Some("Old"), + path: "/tmp/old", + source: "user", + enabled: true, + }) + .await + .unwrap(); + repo.delete_by_name("sample").await.unwrap(); + + let restored = repo + .upsert(UpsertSkillParams { + name: "sample", + description: Some("New"), + path: "/tmp/new", + source: "user", + enabled: true, + }) + .await + .unwrap(); + + assert_eq!(restored.id, created.id); + assert_eq!(restored.description.as_deref(), Some("New")); + assert_eq!(restored.path, "/tmp/new"); + assert_eq!(restored.deleted_at, None); + assert!(repo.find_by_name("sample").await.unwrap().is_some()); + } + + #[tokio::test] + async fn list_filters_soft_deleted_skills() { + let (repo, _db) = setup().await; + + repo.upsert(UpsertSkillParams { + name: "active", + description: None, + path: "/tmp/active", + source: "user", + enabled: true, + }) + .await + .unwrap(); + repo.upsert(UpsertSkillParams { + name: "deleted", + description: None, + path: "/tmp/deleted", + source: "user", + enabled: true, + }) + .await + .unwrap(); + repo.delete_by_name("deleted").await.unwrap(); + + let names: Vec<_> = repo.list().await.unwrap().into_iter().map(|row| row.name).collect(); + assert_eq!(names, vec!["active"]); + assert!(repo.find_by_name_any("deleted").await.unwrap().is_some()); + } + + #[tokio::test] + async fn import_records_keep_structured_error_details() { + let (repo, _db) = setup().await; + + let row = repo + .create_import_record(CreateSkillImportRecordParams { + operation_id: "import_1", + source_label: "parent-pack", + source_path: Some("/tmp/parent-pack"), + source_name: "beta-skill", + skill_id: None, + skill_name: None, + status: "failed", + error_code: Some("SKILL_IMPORT_FILE_TOO_LARGE"), + error_path: Some("assets/movie.mp4"), + actual_bytes: Some(73_400_320), + limit_bytes: Some(10_485_760), + line: None, + column: None, + }) + .await + .unwrap(); + + assert_eq!(row.operation_id, "import_1"); + assert_eq!(row.error_path.as_deref(), Some("assets/movie.mp4")); + assert_eq!(row.actual_bytes, Some(73_400_320)); + assert_eq!(row.limit_bytes, Some(10_485_760)); + let records = repo.list_import_records(10).await.unwrap(); + assert_eq!(records.len(), 1); + } +} diff --git a/crates/aionui-db/tests/skill_management_schema.rs b/crates/aionui-db/tests/skill_management_schema.rs new file mode 100644 index 000000000..9e6e74e9f --- /dev/null +++ b/crates/aionui-db/tests/skill_management_schema.rs @@ -0,0 +1,83 @@ +use aionui_db::init_database_memory; + +#[tokio::test] +async fn migration_creates_skill_management_tables() { + let db = init_database_memory().await.unwrap(); + let pool = db.pool(); + + let skill_columns: Vec<(String,)> = sqlx::query_as("SELECT name FROM pragma_table_info('skills') ORDER BY cid") + .fetch_all(pool) + .await + .unwrap(); + let skill_columns: Vec = skill_columns.into_iter().map(|row| row.0).collect(); + assert_eq!( + skill_columns, + vec![ + "id", + "name", + "description", + "path", + "source", + "enabled", + "deleted_at", + "created_at", + "updated_at", + ] + ); + + let import_columns: Vec<(String,)> = + sqlx::query_as("SELECT name FROM pragma_table_info('skill_import_records') ORDER BY cid") + .fetch_all(pool) + .await + .unwrap(); + let import_columns: Vec = import_columns.into_iter().map(|row| row.0).collect(); + assert_eq!( + import_columns, + vec![ + "id", + "operation_id", + "source_label", + "source_path", + "source_name", + "skill_id", + "skill_name", + "status", + "error_code", + "error_path", + "actual_bytes", + "limit_bytes", + "line", + "column", + "created_at", + ] + ); +} + +#[tokio::test] +async fn migration_allows_known_skill_sources_and_rejects_unknown_sources() { + let db = init_database_memory().await.unwrap(); + let pool = db.pool(); + + for source in ["user", "builtin", "extension", "cron"] { + sqlx::query( + "INSERT INTO skills (id, name, description, path, source, enabled, created_at, updated_at) + VALUES (?, ?, NULL, ?, ?, 1, 1, 1)", + ) + .bind(format!("skill-{source}")) + .bind(format!("skill-{source}")) + .bind(format!("/tmp/{source}")) + .bind(source) + .execute(pool) + .await + .unwrap(); + } + + let rejected = sqlx::query( + "INSERT INTO skills (id, name, description, path, source, enabled, created_at, updated_at) + VALUES ('skill-invalid', 'skill-invalid', NULL, '/tmp/invalid', 'invalid', 1, 1, 1)", + ) + .execute(pool) + .await; + + assert!(rejected.is_err()); +} diff --git a/crates/aionui-extension/src/error.rs b/crates/aionui-extension/src/error.rs index 15dbb2418..39a89e94d 100644 --- a/crates/aionui-extension/src/error.rs +++ b/crates/aionui-extension/src/error.rs @@ -77,6 +77,34 @@ pub enum ExtensionError { #[error("Invalid skill path: {0}")] InvalidSkillPath(String), + #[error("Skill frontmatter is invalid: {0}")] + SkillInvalidFrontmatter(String), + + #[error("No skill directories found: {0}")] + SkillImportNoSkillFound(String), + + #[error("Invalid skill import source: {0}")] + SkillImportInvalidSource(String), + + #[error("Skill import does not allow symlink entries: {0}")] + SkillImportSymlinkEntry(String), + + #[error("Skill import file is too large: {file_bytes} bytes, limit {limit_bytes} bytes")] + SkillImportFileTooLarge { + file_path: Option, + file_bytes: u64, + limit_bytes: u64, + }, + + #[error("Skill import is too large: {total_bytes} bytes, limit {limit_bytes} bytes")] + SkillImportTotalTooLarge { total_bytes: u64, limit_bytes: u64 }, + + #[error("Invalid skill zip archive: {0}")] + SkillImportInvalidZip(String), + + #[error("{0}")] + Db(#[from] aionui_db::DbError), + #[error("Invalid request: {0}")] InvalidRequest(String), diff --git a/crates/aionui-extension/src/lib.rs b/crates/aionui-extension/src/lib.rs index 4cb151ad1..62315503b 100644 --- a/crates/aionui-extension/src/lib.rs +++ b/crates/aionui-extension/src/lib.rs @@ -51,11 +51,13 @@ pub use hub_routes::{HubRouterState, hub_routes}; pub use routes::{ExtensionRouterState, extension_routes}; pub use skill_routes::{SkillRouterState, skill_routes}; pub use skill_service::{ - BUILTIN_SKILLS_ENV_VAR, BuiltinAutoSkillItem, ExternalSkillSource, NamedPath, ResolvedAgentSkill, ScannedSkill, - SkillListItem, SkillPaths, SkillSource, builtin_skills_corpus, delete_skill, detect_and_count_external_skills, - detect_common_skill_paths, export_skill_with_symlink, get_skill_paths, import_skill, import_skill_with_symlink, - link_workspace_skills, list_available_skills, list_builtin_auto_skills, materialize_skills_for_agent, + BUILTIN_SKILLS_ENV_VAR, ExternalSkillSource, NamedPath, ResolvedAgentSkill, ScannedSkill, SkillListItem, + SkillPaths, SkillSource, builtin_skills_corpus, delete_skill, delete_skill_with_repo, + detect_and_count_external_skills, detect_common_skill_paths, export_skill_with_symlink, get_skill_paths, + import_skill, import_skills_with_repo, link_workspace_skills, list_available_skills, + list_available_skills_with_repo, materialize_skills_for_agent, materialize_skills_for_agent_with_repo, read_builtin_rule, read_builtin_skill, read_skill_info, resolve_skill_paths, scan_for_skills, + sync_skill_catalog_into_repo, }; pub use skill_service::{ delete_assistant_rule, delete_assistant_skill, read_assistant_rule, read_assistant_skill, write_assistant_rule, diff --git a/crates/aionui-extension/src/routes.rs b/crates/aionui-extension/src/routes.rs index 3c12db7fd..4d3ef27ad 100644 --- a/crates/aionui-extension/src/routes.rs +++ b/crates/aionui-extension/src/routes.rs @@ -16,6 +16,7 @@ use aionui_api_types::{ GetPermissionsRequest, GetRiskLevelRequest, PermissionDetailResponse, PermissionSummaryResponse, }; use aionui_common::{ApiError, now_ms}; +use serde_json::json; use crate::asset_paths::normalize_relative_asset_path; use crate::error::ExtensionError; @@ -51,6 +52,65 @@ impl From for ApiError { } ExtensionError::SkillNotFound(name) => ApiError::NotFound(format!("Skill not found: {name}")), ExtensionError::InvalidSkillPath(path) => ApiError::BadRequest(format!("Invalid skill path: {path}")), + ExtensionError::SkillInvalidFrontmatter(_) => ApiError::coded( + StatusCode::BAD_REQUEST, + "SKILL_INVALID_FRONTMATTER", + "Skill frontmatter is invalid.", + None, + ), + ExtensionError::SkillImportNoSkillFound(_) => ApiError::coded( + StatusCode::BAD_REQUEST, + "SKILL_IMPORT_NO_SKILL_FOUND", + "No valid skill was found in the selected path.", + None, + ), + ExtensionError::SkillImportInvalidSource(_) => ApiError::coded( + StatusCode::BAD_REQUEST, + "SKILL_IMPORT_INVALID_SOURCE", + "Selected path is not a skill directory, SKILL.md, parent directory, or zip archive.", + None, + ), + ExtensionError::SkillImportSymlinkEntry(_) => ApiError::coded( + StatusCode::BAD_REQUEST, + "SKILL_IMPORT_SYMLINK_ENTRY", + "Skill import cannot contain symlink entries.", + None, + ), + ExtensionError::SkillImportFileTooLarge { + file_path, + file_bytes, + limit_bytes, + } => ApiError::coded( + StatusCode::BAD_REQUEST, + "SKILL_IMPORT_FILE_TOO_LARGE", + "Skill import contains a file that is too large.", + Some(json!({ + "error_path": file_path, + "file_bytes": file_bytes, + "limit_bytes": limit_bytes, + })), + ), + ExtensionError::SkillImportTotalTooLarge { + total_bytes, + limit_bytes, + } => ApiError::coded( + StatusCode::BAD_REQUEST, + "SKILL_IMPORT_TOTAL_TOO_LARGE", + "Skill import is too large.", + Some(json!({ + "total_bytes": total_bytes, + "limit_bytes": limit_bytes, + })), + ), + ExtensionError::SkillImportInvalidZip(_) => ApiError::coded( + StatusCode::BAD_REQUEST, + "SKILL_IMPORT_INVALID_ZIP", + "Selected zip archive is not a valid skill package.", + None, + ), + ExtensionError::Db(aionui_db::DbError::NotFound(msg)) => ApiError::NotFound(msg), + ExtensionError::Db(aionui_db::DbError::Conflict(msg)) => ApiError::Conflict(msg), + ExtensionError::Db(err) => ApiError::Internal(err.to_string()), ExtensionError::InvalidRequest(msg) => ApiError::BadRequest(msg), ExtensionError::Internal(msg) => ApiError::Internal(msg), ExtensionError::Io(e) => ApiError::Internal(e.to_string()), diff --git a/crates/aionui-extension/src/skill_routes.rs b/crates/aionui-extension/src/skill_routes.rs index f16a03601..b792bf664 100644 --- a/crates/aionui-extension/src/skill_routes.rs +++ b/crates/aionui-extension/src/skill_routes.rs @@ -7,17 +7,21 @@ use axum::Router; use axum::extract::rejection::JsonRejection; use axum::extract::{Json, Path as AxumPath, State}; use axum::routing::{delete, get, post}; +use tracing::warn; use aionui_api_types::{ - AddExternalPathRequest, ApiResponse, BuiltinAutoSkillResponse, ExportSkillRequest, ExternalSkillSourceResponse, + AddExternalPathRequest, ApiResponse, ExportSkillRequest, ExternalSkillSourceResponse, ImportSkillFailureResponse, ImportSkillRequest, ImportSkillResponse, MaterializeSkillsRequest, MaterializeSkillsResponse, MaterializedSkillRef, NamedPathResponse, ReadAssistantRuleRequest, ReadBuiltinResourceRequest, ReadSkillInfoRequest, ReadSkillInfoResponse, RemoveExternalPathRequest, ScanForSkillsRequest, ScanForSkillsResponse, - ScannedSkillResponse, SkillListItemResponse, SkillPathsResponse, SkillSourceResponse, WriteAssistantRuleRequest, + ScannedSkillResponse, SkillImportLimitsResponse, SkillImportRecordResponse, SkillListItemResponse, + SkillPathsResponse, SkillSourceResponse, WriteAssistantRuleRequest, }; use aionui_common::ApiError; +use aionui_db::ISkillRepository; use crate::classifier::AssistantRuleDispatcher; +use crate::error::ExtensionError; use crate::external_paths::ExternalPathsManager; use crate::skill_service::{self, SkillPaths, SkillSource}; @@ -25,6 +29,7 @@ fn to_source_response(source: SkillSource) -> SkillSourceResponse { match source { SkillSource::Builtin => SkillSourceResponse::Builtin, SkillSource::Custom => SkillSourceResponse::Custom, + SkillSource::Cron => SkillSourceResponse::Cron, SkillSource::Extension => SkillSourceResponse::Extension, } } @@ -37,6 +42,7 @@ fn to_source_response(source: SkillSource) -> SkillSourceResponse { #[derive(Clone)] pub struct SkillRouterState { pub skill_paths: SkillPaths, + pub skill_repo: Arc, pub external_paths_manager: Arc, /// Optional dispatcher that routes assistant-rule / assistant-skill /// read/write/delete by source (builtin / extension / user). When @@ -56,12 +62,12 @@ pub fn skill_routes(state: SkillRouterState) -> Router { Router::new() // Skill listing & info .route("/api/skills", get(list_skills)) - .route("/api/skills/builtin-auto", get(list_builtin_auto_skills)) + .route("/api/skills/import-history", get(list_import_history)) + .route("/api/skills/import-limits", get(get_import_limits)) .route("/api/skills/info", post(read_skill_info)) .route("/api/skills/paths", get(get_skill_paths)) // Import / export / delete .route("/api/skills/import", post(import_skill)) - .route("/api/skills/import-symlink", post(import_skill_symlink)) .route("/api/skills/export-symlink", post(export_skill_symlink)) .route("/api/skills/{name}", delete(delete_skill)) // Scanning & discovery @@ -102,7 +108,7 @@ pub fn skill_routes(state: SkillRouterState) -> Router { async fn list_skills( State(state): State, ) -> Result>>, ApiError> { - let items = skill_service::list_available_skills(&state.skill_paths).await?; + let items = skill_service::list_available_skills_with_repo(&state.skill_paths, state.skill_repo.as_ref()).await?; let resp: Vec = items .into_iter() .map(|s| SkillListItemResponse { @@ -117,22 +123,6 @@ async fn list_skills( Ok(Json(ApiResponse::ok(resp))) } -/// `GET /api/skills/builtin-auto` — list auto-injected built-in skills. -async fn list_builtin_auto_skills( - State(state): State, -) -> Result>>, ApiError> { - let items = skill_service::list_builtin_auto_skills(&state.skill_paths).await?; - let resp: Vec = items - .into_iter() - .map(|s| BuiltinAutoSkillResponse { - name: s.name, - description: s.description, - location: s.location, - }) - .collect(); - Ok(Json(ApiResponse::ok(resp))) -} - /// `POST /api/skills/info` — read skill info without importing. async fn read_skill_info( body: Result, JsonRejection>, @@ -153,34 +143,70 @@ async fn get_skill_paths( }))) } +/// `GET /api/skills/import-limits` — get server-side skill import limits. +async fn get_import_limits() -> Result>, ApiError> { + let limits = skill_service::skill_import_limits(); + Ok(Json(ApiResponse::ok(SkillImportLimitsResponse { + max_file_bytes: limits.max_file_bytes, + max_total_bytes: limits.max_total_bytes, + }))) +} + // --------------------------------------------------------------------------- // Import / export / delete // --------------------------------------------------------------------------- -/// `POST /api/skills/import` — import a skill by copying. +/// `POST /api/skills/import` — import skill directories or zip packages by copying. async fn import_skill( State(state): State, body: Result, JsonRejection>, ) -> Result>, ApiError> { let Json(req) = body.map_err(ApiError::from)?; - let name = skill_service::import_skill(&state.skill_paths, Path::new(&req.skill_path)).await?; - Ok(Json(ApiResponse::ok(ImportSkillResponse { - skill_name: name.clone(), - skill_names: vec![name], - }))) -} - -/// `POST /api/skills/import-symlink` — import a skill by symlink. -async fn import_skill_symlink( - State(state): State, - body: Result, JsonRejection>, -) -> Result>, ApiError> { - let Json(req) = body.map_err(ApiError::from)?; - let names = skill_service::import_skills_with_symlink(&state.skill_paths, Path::new(&req.skill_path)).await?; + let outcome = match skill_service::import_skills_with_repo( + &state.skill_paths, + state.skill_repo.as_ref(), + Path::new(&req.skill_path), + ) + .await + { + Ok(outcome) => outcome, + Err(err) => { + warn!( + source_path = %req.skill_path, + error = %err, + "skill import failed" + ); + return Err(err.into()); + } + }; + if !outcome.failed.is_empty() { + warn!( + source_path = %req.skill_path, + imported_count = outcome.imported.len(), + failed_count = outcome.failed.len(), + failures = ?outcome.failed, + "skill batch import completed with failures" + ); + } + let names = outcome.imported; let first_name = names.first().cloned().unwrap_or_default(); + let failed = outcome + .failed + .into_iter() + .map(|failure| ImportSkillFailureResponse { + source_name: failure.source_name, + code: failure.code, + error_path: failure.error_path, + actual_bytes: failure.actual_bytes, + limit_bytes: failure.limit_bytes, + line: failure.line, + column: failure.column, + }) + .collect(); Ok(Json(ApiResponse::ok(ImportSkillResponse { skill_name: first_name, skill_names: names, + failed, }))) } @@ -198,10 +224,42 @@ async fn delete_skill( State(state): State, AxumPath(name): AxumPath, ) -> Result>, ApiError> { - skill_service::delete_skill(&state.skill_paths, &name).await?; + skill_service::delete_skill_with_repo(&state.skill_paths, state.skill_repo.as_ref(), &name).await?; Ok(Json(ApiResponse::success())) } +/// `GET /api/skills/import-history` — list recent skill import records. +async fn list_import_history( + State(state): State, +) -> Result>>, ApiError> { + let records = state + .skill_repo + .list_import_records(100) + .await + .map_err(ExtensionError::from)?; + let resp = records + .into_iter() + .map(|row| SkillImportRecordResponse { + id: row.id, + operation_id: row.operation_id, + source_label: row.source_label, + source_path: row.source_path, + source_name: row.source_name, + skill_id: row.skill_id, + skill_name: row.skill_name, + status: row.status, + error_code: row.error_code, + error_path: row.error_path, + actual_bytes: row.actual_bytes, + limit_bytes: row.limit_bytes, + line: row.line, + column: row.column, + created_at: row.created_at, + }) + .collect(); + Ok(Json(ApiResponse::ok(resp))) +} + // --------------------------------------------------------------------------- // Scanning & discovery // --------------------------------------------------------------------------- @@ -301,8 +359,13 @@ async fn materialize_for_agent( if req.conversation_id.trim().is_empty() { return Err(ApiError::BadRequest("conversationId must not be empty".into())); } - let resolved = - skill_service::materialize_skills_for_agent(&state.skill_paths, &req.conversation_id, &req.skills).await?; + let resolved = skill_service::materialize_skills_for_agent_with_repo( + &state.skill_paths, + state.skill_repo.as_ref(), + &req.conversation_id, + &req.skills, + ) + .await?; let skills: Vec = resolved .into_iter() .map(|s| MaterializedSkillRef { @@ -498,6 +561,9 @@ async fn disable_skills_market(State(state): State) -> Result< #[cfg(test)] mod tests { use super::*; + use axum::body::Body; + use axum::http::{Request, StatusCode}; + use tower::ServiceExt; async fn make_state() -> SkillRouterState { let tmp = tempfile::TempDir::new().unwrap(); @@ -511,9 +577,12 @@ mod tests { assistant_skills_dir: tmp.path().join("assistant-skills"), }; let ext_mgr = Arc::new(ExternalPathsManager::with_file(tmp.path().join("paths.json")).await); + let db = aionui_db::init_database_memory().await.unwrap(); + let skill_repo = Arc::new(aionui_db::SqliteSkillRepository::new(db.pool().clone())); std::mem::forget(tmp); SkillRouterState { skill_paths: paths, + skill_repo, external_paths_manager: ext_mgr, assistant_dispatcher: None, } @@ -524,4 +593,21 @@ mod tests { let state = make_state().await; let _router = skill_routes(state); } + + #[tokio::test] + async fn builtin_auto_skill_list_get_is_not_registered() { + let state = make_state().await; + let response = skill_routes(state) + .oneshot( + Request::builder() + .method("GET") + .uri("/api/skills/builtin-auto") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::METHOD_NOT_ALLOWED); + } } diff --git a/crates/aionui-extension/src/skill_service.rs b/crates/aionui-extension/src/skill_service.rs index 2e17c8a14..407e6c350 100644 --- a/crates/aionui-extension/src/skill_service.rs +++ b/crates/aionui-extension/src/skill_service.rs @@ -3,7 +3,9 @@ use std::path::{Component, Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; use include_dir::{Dir, include_dir}; -use tracing::{debug, warn}; +use tracing::{debug, info, warn}; + +use aionui_db::{CreateSkillImportRecordParams, ISkillRepository, SkillRow, UpsertSkillParams}; use crate::constants::{ ASSISTANT_RULES_DIR_NAME, ASSISTANT_SKILLS_DIR_NAME, BUILTIN_AUTO_SKILLS_SUBDIR, BUILTIN_RULES_DIR_NAME, @@ -23,6 +25,22 @@ static BUILTIN_SKILLS: Dir<'static> = include_dir!("$CARGO_MANIFEST_DIR/../aionu /// corpus with an on-disk directory. Consumed by /// [`resolve_skill_paths`] when building [`SkillPaths`]. pub const BUILTIN_SKILLS_ENV_VAR: &str = "AIONUI_BUILTIN_SKILLS_PATH"; +const MAX_SKILL_IMPORT_FILE_BYTES: u64 = 10 * 1024 * 1024; +const MAX_SKILL_IMPORT_TOTAL_BYTES: u64 = 50 * 1024 * 1024; +const IMPORT_STAGING_PREFIX: &str = ".import-staging-"; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct SkillImportLimits { + pub max_file_bytes: u64, + pub max_total_bytes: u64, +} + +pub fn skill_import_limits() -> SkillImportLimits { + SkillImportLimits { + max_file_bytes: MAX_SKILL_IMPORT_FILE_BYTES, + max_total_bytes: MAX_SKILL_IMPORT_TOTAL_BYTES, + } +} /// Expose the embedded builtin skills corpus for startup /// materialization. Consumers outside this crate should not depend on @@ -199,6 +217,7 @@ pub async fn delete_assistant_skill(paths: &SkillPaths, assistant_id: &str) -> R pub enum SkillSource { Builtin, Custom, + Cron, Extension, } @@ -240,20 +259,13 @@ pub async fn list_available_skills(paths: &SkillPaths) -> Result Result Result, ExtensionError> { + list_skills_from_repo(paths, repo).await +} + /// Emit a [`SkillListItem`] for every built-in skill (both auto-inject /// and opt-in). All paths resolve directly against /// `paths.builtin_skills_dir`. @@ -350,11 +370,12 @@ pub struct ScannedSkill { /// An auto-injected built-in skill. /// -/// Returned by `GET /api/skills/builtin-auto`. `location` is the -/// relative path the frontend passes back into -/// `POST /api/skills/builtin-skill`, e.g. `"auto-inject/cron/SKILL.md"`. +/// Auto-injected built-in skill metadata. `location` is the relative path +/// the frontend passes back into `POST /api/skills/builtin-skill`, e.g. +/// `"auto-inject/cron/SKILL.md"`. +#[cfg(test)] #[derive(Debug, Clone, PartialEq)] -pub struct BuiltinAutoSkillItem { +struct AutoInjectSkillDiskItem { pub name: String, pub description: String, pub location: String, @@ -365,14 +386,16 @@ pub struct BuiltinAutoSkillItem { /// Reads from `{paths.builtin_skills_dir}/auto-inject/`. A missing /// `auto-inject/` directory yields an empty list, matching the /// graceful-degradation semantics used elsewhere in this module. -pub async fn list_builtin_auto_skills(paths: &SkillPaths) -> Result, ExtensionError> { +#[cfg(test)] +async fn list_auto_inject_skills_from_disk(paths: &SkillPaths) -> Result, ExtensionError> { let auto_dir = paths.builtin_skills_dir.join(BUILTIN_AUTO_SKILLS_SUBDIR); let mut items = list_auto_skills_from_disk(&auto_dir).await; items.sort_by(|a, b| a.name.cmp(&b.name)); Ok(items) } -async fn list_auto_skills_from_disk(auto_dir: &Path) -> Vec { +#[cfg(test)] +async fn list_auto_skills_from_disk(auto_dir: &Path) -> Vec { let entries = match scan_skill_dirs(auto_dir).await { Ok(entries) => entries, Err(_) => return Vec::new(), @@ -381,7 +404,7 @@ async fn list_auto_skills_from_disk(auto_dir: &Path) -> Vec Result<(String, String), Exte .map_err(|_| ExtensionError::SkillNotFound(skill_path.display().to_string()))?; let (name, description) = parse_frontmatter_fields(&content) - .ok_or_else(|| ExtensionError::InvalidSkillPath(format!("No valid frontmatter in {}", skill_file.display())))?; + .ok_or_else(|| ExtensionError::SkillInvalidFrontmatter(skill_file.display().to_string()))?; // Fallback: use directory name if name is empty let final_name = if name.is_empty() { @@ -428,84 +451,257 @@ pub async fn read_skill_info(skill_path: &Path) -> Result<(String, String), Exte /// /// Returns the skill name. pub async fn import_skill(paths: &SkillPaths, skill_path: &Path) -> Result { - let (name, _) = read_skill_info(skill_path).await?; - validate_filename(&name)?; + let copied = copy_skill_into_user_dir(paths, skill_path).await?; - let target_dir = paths.user_skills_dir.join(&name); - tokio::fs::create_dir_all(&paths.user_skills_dir).await?; + debug!( + skill = %copied.name, + target = %copied.target_dir.display(), + copied_bytes = copied.copied_bytes, + "skill imported (copy)" + ); + Ok(copied.name) +} - copy_dir_recursive(skill_path, &target_dir).await?; +/// Import a skill and persist its management metadata in the database. +pub async fn import_skill_with_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, + skill_path: &Path, +) -> Result { + let copied = copy_skill_into_user_dir(paths, skill_path).await?; + let overwritten = repo.find_by_name(&copied.name).await?.is_some(); + let row = repo + .upsert(UpsertSkillParams { + name: &copied.name, + description: Some(&copied.description), + path: copied.target_dir.to_string_lossy().as_ref(), + source: "user", + enabled: true, + }) + .await?; - debug!(skill = %name, target = %target_dir.display(), "skill imported (copy)"); - Ok(name) + debug!( + skill = %copied.name, + target = %copied.target_dir.display(), + copied_bytes = copied.copied_bytes, + "skill imported (copy)" + ); + Ok(ImportedSkill { + name: copied.name, + skill_id: Some(row.id), + overwritten, + copied_bytes: copied.copied_bytes, + }) } -/// Import a skill by creating a symlink in the user skills directory. -/// -/// Returns the skill name. -pub async fn import_skill_with_symlink(paths: &SkillPaths, skill_path: &Path) -> Result { - let (name, _) = read_skill_info(skill_path).await?; +struct CopiedSkill { + name: String, + description: String, + target_dir: PathBuf, + copied_bytes: u64, +} + +async fn copy_skill_into_user_dir(paths: &SkillPaths, skill_path: &Path) -> Result { + let (name, description) = read_skill_info(skill_path).await?; validate_filename(&name)?; - let target_link = paths.user_skills_dir.join(&name); + let target_dir = paths.user_skills_dir.join(&name); tokio::fs::create_dir_all(&paths.user_skills_dir).await?; - // Remove existing link/dir if present - if target_link.exists() { - if target_link.is_symlink() || target_link.is_file() { - tokio::fs::remove_file(&target_link).await?; - } else { - tokio::fs::remove_dir_all(&target_link).await?; + let staging_dir = import_staging_dir(paths, &name); + replace_existing_path(&staging_dir).await?; + let mut budget = SkillImportBudget::default(); + if let Err(err) = copy_skill_dir_for_import(skill_path, &staging_dir, skill_path, &mut budget).await { + if let Err(cleanup_err) = replace_existing_path(&staging_dir).await { + warn!( + staging = %staging_dir.display(), + error = %cleanup_err, + "failed to clean skill import staging after copy failure" + ); } + return Err(err); } + replace_existing_path(&target_dir).await?; + tokio::fs::rename(&staging_dir, &target_dir).await?; - create_symlink(skill_path, &target_link).await?; + Ok(CopiedSkill { + name, + description, + target_dir, + copied_bytes: budget.total_bytes, + }) +} - debug!(skill = %name, link = %target_link.display(), "skill imported (symlink)"); - Ok(name) +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillImportBatchFailure { + pub source_name: String, + pub code: String, + pub error_path: Option, + pub actual_bytes: Option, + pub limit_bytes: Option, + pub line: Option, + pub column: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ImportedSkill { + pub name: String, + pub skill_id: Option, + pub overwritten: bool, + pub copied_bytes: u64, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillImportOutcome { + pub operation_id: String, + pub imported: Vec, + pub failed: Vec, } /// Import one skill, a parent directory containing skills, or a zip archive. /// -/// Directory inputs preserve the existing symlink behavior. Zip inputs are -/// extracted into an internal temporary directory, then copied into the user -/// skills directory so imported skills do not point at disposable files. -pub async fn import_skills_with_symlink(paths: &SkillPaths, source_path: &Path) -> Result, ExtensionError> { +/// Imports persist stable copies in the user skills directory. Runtime +/// materialization creates agent-specific symlinks later. +pub async fn import_skills(paths: &SkillPaths, source_path: &Path) -> Result { + let operation_id = aionui_common::generate_prefixed_id("skill_import_op"); if is_zip_path(source_path) { - return import_skills_from_zip(paths, source_path).await; + let mut outcome = import_skills_from_zip(paths, source_path).await?; + outcome.operation_id = operation_id; + return Ok(outcome); } let source_path = normalize_import_source_path(source_path)?; if source_path.is_dir() { if source_path.join(SKILL_MANIFEST_FILE).exists() { - return Ok(vec![import_skill_with_symlink(paths, &source_path).await?]); + let name = import_skill(paths, &source_path).await?; + return Ok(SkillImportOutcome { + operation_id, + imported: vec![name], + failed: Vec::new(), + }); } let skills = scan_skill_dirs(&source_path).await?; if skills.is_empty() { - return Err(ExtensionError::InvalidSkillPath(format!( - "No skill directories found in {}", - source_path.display() - ))); + return Err(ExtensionError::SkillImportNoSkillFound( + source_path.display().to_string(), + )); } - let mut imported = Vec::new(); - for skill in skills { - imported.push(import_skill_with_symlink(paths, Path::new(&skill.path)).await?); + return import_skill_dirs_batch( + paths, + skills.into_iter().map(|skill| PathBuf::from(skill.path)).collect(), + operation_id, + ) + .await; + } + + Err(ExtensionError::SkillImportInvalidSource( + source_path.display().to_string(), + )) +} + +/// Import skills and persist both current skill metadata and import history. +pub async fn import_skills_with_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, + source_path: &Path, +) -> Result { + let operation_id = aionui_common::generate_prefixed_id("skill_import_op"); + let source_label = import_source_label(source_path); + let source_path_text = source_path.to_string_lossy().into_owned(); + + if is_zip_path(source_path) { + let temp_root = paths.user_skills_dir.join(".import-tmp"); + tokio::fs::create_dir_all(&temp_root).await?; + + let nonce = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + let extract_dir = temp_root.join(format!("skills-{}-{nonce}", std::process::id())); + tokio::fs::create_dir_all(&extract_dir).await?; + + let archive = source_path.to_path_buf(); + let destination = extract_dir.clone(); + let extraction = tokio::task::spawn_blocking(move || extract_zip_archive(&archive, &destination)) + .await + .map_err(|e| ExtensionError::SkillImportInvalidZip(format!("Zip extraction task failed: {e}")))?; + + if let Err(err) = extraction { + let _ = tokio::fs::remove_dir_all(&extract_dir).await; + let _ = tokio::fs::remove_dir(&temp_root).await; + return Err(err); + } + + let result = async { + let mut skill_dirs = Vec::new(); + collect_skill_dirs_recursive(&extract_dir, &mut skill_dirs).await?; + if skill_dirs.is_empty() { + return Err(ExtensionError::SkillImportNoSkillFound( + source_path.display().to_string(), + )); + } + + import_skill_dirs_batch_with_repo( + paths, + repo, + skill_dirs, + &operation_id, + &source_label, + Some(&source_path_text), + ) + .await + } + .await; + + let _ = tokio::fs::remove_dir_all(&extract_dir).await; + let _ = tokio::fs::remove_dir(&temp_root).await; + return result; + } + + let source_path = normalize_import_source_path(source_path)?; + let source_label = import_source_label(&source_path); + let source_path_text = source_path.to_string_lossy().into_owned(); + + if source_path.is_dir() { + if source_path.join(SKILL_MANIFEST_FILE).exists() { + return import_skill_dirs_batch_with_repo( + paths, + repo, + vec![source_path], + &operation_id, + &source_label, + Some(&source_path_text), + ) + .await; } - imported.sort(); - imported.dedup(); - return Ok(imported); + + let skills = scan_skill_dirs(&source_path).await?; + if skills.is_empty() { + return Err(ExtensionError::SkillImportNoSkillFound( + source_path.display().to_string(), + )); + } + + return import_skill_dirs_batch_with_repo( + paths, + repo, + skills.into_iter().map(|skill| PathBuf::from(skill.path)).collect(), + &operation_id, + &source_label, + Some(&source_path_text), + ) + .await; } - Err(ExtensionError::InvalidSkillPath(format!( - "Expected a skill directory, parent directory, SKILL.md, or zip archive: {}", - source_path.display() - ))) + Err(ExtensionError::SkillImportInvalidSource( + source_path.display().to_string(), + )) } -async fn import_skills_from_zip(paths: &SkillPaths, archive_path: &Path) -> Result, ExtensionError> { +async fn import_skills_from_zip(paths: &SkillPaths, archive_path: &Path) -> Result { let temp_root = paths.user_skills_dir.join(".import-tmp"); tokio::fs::create_dir_all(&temp_root).await?; @@ -520,7 +716,7 @@ async fn import_skills_from_zip(paths: &SkillPaths, archive_path: &Path) -> Resu let destination = extract_dir.clone(); let extraction = tokio::task::spawn_blocking(move || extract_zip_archive(&archive, &destination)) .await - .map_err(|e| ExtensionError::InvalidSkillPath(format!("Zip extraction task failed: {e}")))?; + .map_err(|e| ExtensionError::SkillImportInvalidZip(format!("Zip extraction task failed: {e}")))?; if let Err(err) = extraction { let _ = tokio::fs::remove_dir_all(&extract_dir).await; @@ -532,19 +728,17 @@ async fn import_skills_from_zip(paths: &SkillPaths, archive_path: &Path) -> Resu let mut skill_dirs = Vec::new(); collect_skill_dirs_recursive(&extract_dir, &mut skill_dirs).await?; if skill_dirs.is_empty() { - return Err(ExtensionError::InvalidSkillPath(format!( - "No skill directories found in {}", - archive_path.display() - ))); + return Err(ExtensionError::SkillImportNoSkillFound( + archive_path.display().to_string(), + )); } - let mut imported = Vec::new(); - for skill_dir in skill_dirs { - imported.push(import_skill(paths, &skill_dir).await?); - } - imported.sort(); - imported.dedup(); - Ok(imported) + import_skill_dirs_batch( + paths, + skill_dirs, + aionui_common::generate_prefixed_id("skill_import_op"), + ) + .await } .await; @@ -553,6 +747,177 @@ async fn import_skills_from_zip(paths: &SkillPaths, archive_path: &Path) -> Resu result } +async fn import_skill_dirs_batch( + paths: &SkillPaths, + skill_dirs: Vec, + operation_id: String, +) -> Result { + let mut imported = Vec::new(); + let mut failed = Vec::new(); + + for skill_dir in skill_dirs { + match import_skill(paths, &skill_dir).await { + Ok(name) => imported.push(name), + Err(err) => failed.push(skill_import_failure_from_error(&import_source_name(&skill_dir), &err)), + } + } + + imported.sort(); + imported.dedup(); + failed.sort_by(|left, right| { + left.source_name + .cmp(&right.source_name) + .then(left.code.cmp(&right.code)) + }); + failed.dedup(); + + Ok(SkillImportOutcome { + operation_id, + imported, + failed, + }) +} + +async fn import_skill_dirs_batch_with_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, + skill_dirs: Vec, + operation_id: &str, + source_label: &str, + source_path: Option<&str>, +) -> Result { + let mut imported = Vec::new(); + let mut failed = Vec::new(); + + for skill_dir in skill_dirs { + let source_name = import_source_name(&skill_dir); + match import_skill_with_repo(paths, repo, &skill_dir).await { + Ok(skill) => { + repo.create_import_record(CreateSkillImportRecordParams { + operation_id, + source_label, + source_path, + source_name: &source_name, + skill_id: skill.skill_id.as_deref(), + skill_name: Some(&skill.name), + status: if skill.overwritten { "overwritten" } else { "imported" }, + error_code: None, + error_path: None, + actual_bytes: Some(u64_to_i64(skill.copied_bytes)), + limit_bytes: None, + line: None, + column: None, + }) + .await?; + imported.push(skill.name); + } + Err(err) => { + let failure = skill_import_failure_from_error(&source_name, &err); + repo.create_import_record(CreateSkillImportRecordParams { + operation_id, + source_label, + source_path, + source_name: &source_name, + skill_id: None, + skill_name: None, + status: "failed", + error_code: Some(&failure.code), + error_path: failure.error_path.as_deref(), + actual_bytes: failure.actual_bytes, + limit_bytes: failure.limit_bytes, + line: failure.line, + column: failure.column, + }) + .await?; + failed.push(failure); + } + } + } + + imported.sort(); + imported.dedup(); + failed.sort_by(|left, right| { + left.source_name + .cmp(&right.source_name) + .then(left.code.cmp(&right.code)) + }); + failed.dedup(); + + Ok(SkillImportOutcome { + operation_id: operation_id.to_owned(), + imported, + failed, + }) +} + +fn import_source_name(path: &Path) -> String { + path.file_name() + .and_then(|name| name.to_str()) + .filter(|name| !name.is_empty()) + .map(str::to_owned) + .unwrap_or_else(|| path.display().to_string()) +} + +fn import_source_label(path: &Path) -> String { + path.file_name() + .and_then(|name| name.to_str()) + .filter(|name| !name.is_empty()) + .map(str::to_owned) + .unwrap_or_else(|| path.display().to_string()) +} + +fn skill_import_failure_from_error(source_name: &str, err: &ExtensionError) -> SkillImportBatchFailure { + let mut failure = SkillImportBatchFailure { + source_name: source_name.to_owned(), + code: skill_import_error_code(err).to_owned(), + error_path: None, + actual_bytes: None, + limit_bytes: None, + line: None, + column: None, + }; + + match err { + ExtensionError::SkillImportFileTooLarge { + file_path, + file_bytes, + limit_bytes, + } => { + failure.error_path = file_path.clone(); + failure.actual_bytes = Some(u64_to_i64(*file_bytes)); + failure.limit_bytes = Some(u64_to_i64(*limit_bytes)); + } + ExtensionError::SkillImportTotalTooLarge { + total_bytes, + limit_bytes, + } => { + failure.actual_bytes = Some(u64_to_i64(*total_bytes)); + failure.limit_bytes = Some(u64_to_i64(*limit_bytes)); + } + _ => {} + } + + failure +} + +fn u64_to_i64(value: u64) -> i64 { + i64::try_from(value).unwrap_or(i64::MAX) +} + +fn skill_import_error_code(err: &ExtensionError) -> &'static str { + match err { + ExtensionError::SkillInvalidFrontmatter(_) => "SKILL_INVALID_FRONTMATTER", + ExtensionError::SkillImportNoSkillFound(_) => "SKILL_IMPORT_NO_SKILL_FOUND", + ExtensionError::SkillImportInvalidSource(_) => "SKILL_IMPORT_INVALID_SOURCE", + ExtensionError::SkillImportSymlinkEntry(_) => "SKILL_IMPORT_SYMLINK_ENTRY", + ExtensionError::SkillImportFileTooLarge { .. } => "SKILL_IMPORT_FILE_TOO_LARGE", + ExtensionError::SkillImportTotalTooLarge { .. } => "SKILL_IMPORT_TOTAL_TOO_LARGE", + ExtensionError::SkillImportInvalidZip(_) => "SKILL_IMPORT_INVALID_ZIP", + ExtensionError::PathTraversal(_) => "SKILL_IMPORT_INVALID_NAME", + _ => "SKILL_IMPORT_FAILED", + } +} + fn is_zip_path(path: &Path) -> bool { path.extension() .and_then(|ext| ext.to_str()) @@ -575,12 +940,111 @@ fn normalize_import_source_path(source_path: &Path) -> Result Result<(), ExtensionError> { + let metadata = match tokio::fs::symlink_metadata(path).await { + Ok(metadata) => metadata, + Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e.into()), + }; + + if metadata.file_type().is_symlink() || metadata.is_file() { + tokio::fs::remove_file(path).await?; + } else { + tokio::fs::remove_dir_all(path).await?; + } + + Ok(()) +} + +fn import_staging_dir(paths: &SkillPaths, skill_name: &str) -> PathBuf { + let nonce = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + paths.user_skills_dir.join(format!( + "{IMPORT_STAGING_PREFIX}{skill_name}-{}-{nonce}", + std::process::id() + )) +} + +#[derive(Default)] +struct SkillImportBudget { + total_bytes: u64, +} + +async fn copy_skill_dir_for_import( + src: &Path, + dst: &Path, + root: &Path, + budget: &mut SkillImportBudget, +) -> Result<(), ExtensionError> { + tokio::fs::create_dir_all(dst).await?; + + let mut entries = tokio::fs::read_dir(src).await?; + while let Ok(Some(entry)) = entries.next_entry().await { + let entry_path = entry.path(); + let file_name = entry.file_name(); + let file_name = file_name.to_string_lossy(); + + let metadata = tokio::fs::symlink_metadata(&entry_path).await?; + if metadata.file_type().is_symlink() { + return Err(ExtensionError::SkillImportSymlinkEntry( + entry_path.display().to_string(), + )); + } + + let dest_path = dst.join(file_name.as_ref()); + if metadata.is_dir() { + Box::pin(copy_skill_dir_for_import(&entry_path, &dest_path, root, budget)).await?; + continue; + } + + if !metadata.is_file() { + continue; + } + + let relative_path = entry_path + .strip_prefix(root) + .ok() + .map(|path| path.to_string_lossy().into_owned()); + enforce_skill_import_budget(metadata.len(), relative_path.as_deref(), budget)?; + tokio::fs::copy(&entry_path, &dest_path).await?; + } + + Ok(()) +} + +fn enforce_skill_import_budget( + file_bytes: u64, + file_path: Option<&str>, + budget: &mut SkillImportBudget, +) -> Result<(), ExtensionError> { + if file_bytes > MAX_SKILL_IMPORT_FILE_BYTES { + return Err(ExtensionError::SkillImportFileTooLarge { + file_path: file_path.map(str::to_owned), + file_bytes, + limit_bytes: MAX_SKILL_IMPORT_FILE_BYTES, + }); + } + + let next_total = budget.total_bytes.saturating_add(file_bytes); + if next_total > MAX_SKILL_IMPORT_TOTAL_BYTES { + return Err(ExtensionError::SkillImportTotalTooLarge { + total_bytes: next_total, + limit_bytes: MAX_SKILL_IMPORT_TOTAL_BYTES, + }); + } + + budget.total_bytes = next_total; + Ok(()) +} + /// Export a skill by creating a symlink in the target directory. pub async fn export_skill_with_symlink(skill_path: &Path, target_dir: &Path) -> Result<(), ExtensionError> { let skill_name = skill_path @@ -620,26 +1084,52 @@ pub async fn delete_skill(paths: &SkillPaths, skill_name: &str) -> Result<(), Ex } let user_path = paths.user_skills_dir.join(skill_name); - - if !user_path.exists() { - // Check if it exists as a built-in (disk override → filesystem, - // otherwise embedded corpus). - if builtin_skill_exists(paths, skill_name) { - return Err(ExtensionError::BuiltinSkillDeletion(skill_name.to_string())); + match tokio::fs::symlink_metadata(&user_path).await { + Ok(_) => {} + Err(e) if e.kind() == io::ErrorKind::NotFound => { + // Check if it exists as a built-in (disk override → filesystem, + // otherwise embedded corpus). + if builtin_skill_exists(paths, skill_name) { + return Err(ExtensionError::BuiltinSkillDeletion(skill_name.to_string())); + } + return Err(ExtensionError::SkillNotFound(skill_name.to_string())); } - return Err(ExtensionError::SkillNotFound(skill_name.to_string())); + Err(e) => return Err(e.into()), } - if user_path.is_symlink() || user_path.is_file() { - tokio::fs::remove_file(&user_path).await?; - } else { - tokio::fs::remove_dir_all(&user_path).await?; - } + replace_existing_path(&user_path).await?; debug!(skill = %skill_name, "skill deleted"); Ok(()) } +/// Soft-delete a user skill through the database state source. +pub async fn delete_skill_with_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, + skill_name: &str, +) -> Result<(), ExtensionError> { + validate_filename(skill_name)?; + if builtin_skill_exists(paths, skill_name) { + return Err(ExtensionError::BuiltinSkillDeletion(skill_name.to_string())); + } + + let Some(row) = repo.find_by_name(skill_name).await? else { + return Err(ExtensionError::SkillNotFound(skill_name.to_string())); + }; + if !PathBuf::from(&row.path).is_dir() { + warn!( + skill = %skill_name, + path = %row.path, + "deleting skill whose database path is missing" + ); + } + + repo.delete_by_name(skill_name).await?; + debug!(skill = %skill_name, "skill marked deleted"); + Ok(()) +} + /// Check whether a skill name exists in the built-in corpus — either as /// a top-level opt-in skill or under `auto-inject/`. Consults the /// on-disk tree at `paths.builtin_skills_dir`. @@ -706,7 +1196,39 @@ pub async fn materialize_skills_for_agent( warn!(skill = %name, "skipping skill with invalid name"); continue; } - match resolve_skill_source_path(paths, name) { + match resolve_skill_source_path(paths, name).await? { + Some(source_path) => resolved.push(ResolvedAgentSkill { + name: name.clone(), + source_path, + }), + None => warn!(skill = %name, "skill not found in any source"), + } + } + + resolved.sort_by(|a, b| a.name.cmp(&b.name)); + Ok(resolved) +} + +/// Resolve requested skill names using the database for user skill state. +pub async fn materialize_skills_for_agent_with_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, + conversation_id: &str, + skills: &[String], +) -> Result, ExtensionError> { + validate_filename(conversation_id)?; + sync_disk_user_skills_into_repo(paths, repo).await?; + + let mut resolved = Vec::with_capacity(skills.len()); + for name in skills { + if name.is_empty() { + continue; + } + if name.contains('/') || name.contains('\\') || name.contains("..") { + warn!(skill = %name, "skipping skill with invalid name"); + continue; + } + match resolve_skill_source_path_with_repo(paths, repo, name).await? { Some(source_path) => resolved.push(ResolvedAgentSkill { name: name.clone(), source_path, @@ -814,24 +1336,57 @@ async fn path_is_dir(path: &Path) -> bool { /// Resolve a skill name to its on-disk source directory using the same /// search order as [`materialize_skills_for_agent`]. Returns `None` if /// no matching directory exists in any known source. -fn resolve_skill_source_path(paths: &SkillPaths, name: &str) -> Option { +async fn resolve_skill_source_path(paths: &SkillPaths, name: &str) -> Result, ExtensionError> { let top = paths.builtin_skills_dir.join(name); if top.is_dir() { - return Some(top); + return Ok(Some(top)); } let auto = paths.builtin_skills_dir.join(BUILTIN_AUTO_SKILLS_SUBDIR).join(name); if auto.is_dir() { - return Some(auto); + return Ok(Some(auto)); } let user = paths.user_skills_dir.join(name); if user.is_dir() { - return Some(user); + return Ok(Some(user)); } let cron = paths.cron_skills_dir.join(name); if cron.is_dir() { - return Some(cron); + return Ok(Some(cron)); } - None + Ok(None) +} + +async fn resolve_skill_source_path_with_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, + name: &str, +) -> Result, ExtensionError> { + let top = paths.builtin_skills_dir.join(name); + if top.is_dir() { + return Ok(Some(top)); + } + let auto = paths.builtin_skills_dir.join(BUILTIN_AUTO_SKILLS_SUBDIR).join(name); + if auto.is_dir() { + return Ok(Some(auto)); + } + if let Some(row) = repo.find_by_name_any(name).await? { + let path = PathBuf::from(&row.path); + if path.is_dir() { + return Ok(Some(path)); + } + warn!( + skill = %name, + path = %path.display(), + deleted = row.deleted_at.is_some(), + "skill row points at a missing directory" + ); + return Ok(None); + } + let cron = paths.cron_skills_dir.join(name); + if cron.is_dir() { + return Ok(Some(cron)); + } + Ok(None) } // --------------------------------------------------------------------------- @@ -951,6 +1506,188 @@ pub fn get_skill_paths(paths: &SkillPaths) -> (String, String) { ) } +async fn list_skills_from_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, +) -> Result, ExtensionError> { + let mut items = Vec::new(); + for row in repo.list().await? { + let description = row.description.clone().unwrap_or_default(); + items.push(skill_row_to_list_item(paths, row, description)); + } + Ok(items) +} + +/// Synchronize filesystem-backed skill catalogs into the database. +/// +/// Listing endpoints read the database only. This synchronization is intended +/// for startup/refresh paths and imports so the database remains the single +/// catalog source for API consumers. +pub async fn sync_skill_catalog_into_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, +) -> Result<(), ExtensionError> { + sync_disk_user_skills_into_repo(paths, repo).await?; + sync_builtin_skills_into_repo(paths, repo).await?; + Ok(()) +} + +async fn sync_builtin_skills_into_repo(paths: &SkillPaths, repo: &dyn ISkillRepository) -> Result<(), ExtensionError> { + if let Ok(skills) = scan_skill_dirs(&paths.builtin_skills_dir).await { + for skill in skills { + if skill.name == BUILTIN_AUTO_SKILLS_SUBDIR { + continue; + } + sync_managed_skill_into_repo(repo, &skill, "builtin").await?; + } + } + + let auto_inject_dir = paths.builtin_skills_dir.join(BUILTIN_AUTO_SKILLS_SUBDIR); + if let Ok(skills) = scan_skill_dirs(&auto_inject_dir).await { + for skill in skills { + sync_managed_skill_into_repo(repo, &skill, "builtin").await?; + } + } + + if let Ok(skills) = scan_skill_dirs(&paths.cron_skills_dir).await { + for skill in skills { + sync_managed_skill_into_repo(repo, &skill, "cron").await?; + } + } + + Ok(()) +} + +async fn sync_managed_skill_into_repo( + repo: &dyn ISkillRepository, + skill: &ScannedSkill, + source: &str, +) -> Result<(), ExtensionError> { + if let Some(existing) = repo.find_by_name_any(&skill.name).await? + && existing.source == "user" + && existing.deleted_at.is_none() + && existing.enabled + { + return Ok(()); + } + + repo.upsert(UpsertSkillParams { + name: &skill.name, + description: Some(&skill.description), + path: &skill.path, + source, + enabled: true, + }) + .await?; + Ok(()) +} + +async fn sync_disk_user_skills_into_repo( + paths: &SkillPaths, + repo: &dyn ISkillRepository, +) -> Result<(), ExtensionError> { + let scanned = scan_skill_dirs(&paths.user_skills_dir).await?; + let mut backfilled = 0usize; + + for skill in scanned { + if let Err(err) = validate_filename(&skill.name) { + warn!( + skill = %skill.name, + error = %err, + "skipping existing user skill with invalid name during database backfill" + ); + continue; + } + if repo.find_by_name_any(&skill.name).await?.is_some() { + continue; + } + + repo.upsert(UpsertSkillParams { + name: &skill.name, + description: Some(&skill.description), + path: &skill.path, + source: "user", + enabled: true, + }) + .await?; + backfilled += 1; + } + + if backfilled > 0 { + info!( + count = backfilled, + "backfilled existing user skill directories into skill database" + ); + } + + Ok(()) +} + +fn skill_row_to_list_item(paths: &SkillPaths, row: SkillRow, description: String) -> SkillListItem { + let source = skill_source_from_row(&row.source); + let relative_location = skill_relative_location(paths, &row, source); + let location = match source { + SkillSource::Builtin | SkillSource::Cron => PathBuf::from(&row.path) + .join(SKILL_MANIFEST_FILE) + .to_string_lossy() + .into_owned(), + SkillSource::Custom | SkillSource::Extension => row.path.clone(), + }; + + SkillListItem { + name: row.name, + description, + location, + relative_location, + is_custom: source == SkillSource::Custom, + source, + } +} + +fn skill_source_from_row(source: &str) -> SkillSource { + match source { + "builtin" => SkillSource::Builtin, + "cron" => SkillSource::Cron, + "extension" => SkillSource::Extension, + _ => SkillSource::Custom, + } +} + +fn skill_relative_location(paths: &SkillPaths, row: &SkillRow, source: SkillSource) -> Option { + let skill_dir = Path::new(&row.path); + match source { + SkillSource::Builtin => relative_skill_manifest_path(&paths.builtin_skills_dir, skill_dir), + SkillSource::Cron => None, + SkillSource::Custom | SkillSource::Extension => None, + } +} + +fn relative_skill_manifest_path(base_dir: &Path, skill_dir: &Path) -> Option { + let relative_dir = skill_dir.strip_prefix(base_dir).ok()?; + let mut relative = relative_dir.to_string_lossy().replace('\\', "/"); + if relative.is_empty() { + return None; + } + relative.push('/'); + relative.push_str(SKILL_MANIFEST_FILE); + Some(relative) +} + +async fn list_user_skills_from_disk(paths: &SkillPaths) -> Result, ExtensionError> { + let scanned = scan_skill_dirs(&paths.user_skills_dir).await?; + Ok(scanned + .into_iter() + .map(|skill| SkillListItem { + name: skill.name, + description: skill.description, + location: skill.path, + relative_location: None, + is_custom: true, + source: SkillSource::Custom, + }) + .collect()) +} + // --------------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------------- @@ -1074,24 +1811,11 @@ async fn delete_assistant_resource(dir: &Path, assistant_id: &str) -> Result Result, ExtensionError> { let mut result = Vec::new(); + let mut skill_dirs = Vec::new(); + collect_skill_dirs_recursive(dir, &mut skill_dirs).await?; - let mut entries = match tokio::fs::read_dir(dir).await { - Ok(entries) => entries, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(result), - Err(e) => return Err(ExtensionError::Io(e)), - }; - - while let Ok(Some(entry)) = entries.next_entry().await { - let entry_path = entry.path(); - if !entry_path.is_dir() { - continue; - } - + for entry_path in skill_dirs { let skill_file = entry_path.join(SKILL_MANIFEST_FILE); - if !skill_file.exists() { - continue; - } - match tokio::fs::read_to_string(&skill_file).await { Ok(content) => { if let Some((name, description)) = parse_frontmatter_fields(&content) { @@ -1203,13 +1927,13 @@ fn reject_zip_symlink(entry: &zip::read::ZipFile<'_>) -> Result<(), ExtensionErr if let Some(mode) = entry.unix_mode() && mode & 0o170000 == 0o120000 { - return Err(ExtensionError::PathTraversal(entry.name().to_string())); + return Err(ExtensionError::SkillImportSymlinkEntry(entry.name().to_string())); } Ok(()) } fn zip_error(err: zip::result::ZipError) -> ExtensionError { - ExtensionError::InvalidSkillPath(format!("Invalid zip archive: {err}")) + ExtensionError::SkillImportInvalidZip(err.to_string()) } /// Parse SKILL.md frontmatter to extract name and description. @@ -1426,6 +2150,7 @@ async fn create_symlink(src: &Path, dst: &Path) -> Result<(), ExtensionError> { #[cfg(test)] mod tests { use super::*; + use aionui_db::{ISkillRepository, SqliteSkillRepository, init_database_memory}; use std::io::Write; use tempfile::TempDir; @@ -1777,7 +2502,7 @@ mod tests { // ----------------------------------------------------------------------- #[tokio::test] - async fn list_builtin_auto_skills_from_disk_override() { + async fn list_auto_inject_skills_from_disk_override() { let tmp = TempDir::new().unwrap(); let paths = make_disk_builtin_paths(tmp.path()); let builtin_dir = disk_builtin_dir(&paths).to_path_buf(); @@ -1789,7 +2514,7 @@ mod tests { // A top-level built-in skill (NOT under auto-inject/) must be excluded. create_skill_in_dir(&builtin_dir, "review", "Top-level builtin"); - let autos = list_builtin_auto_skills(&paths).await.unwrap(); + let autos = list_auto_inject_skills_from_disk(&paths).await.unwrap(); assert_eq!(autos.len(), 2); let names: std::collections::HashSet<_> = autos.iter().map(|s| s.name.as_str()).collect(); @@ -1803,12 +2528,12 @@ mod tests { } #[tokio::test] - async fn list_builtin_auto_skills_missing_dir_returns_empty() { + async fn list_auto_inject_skills_missing_dir_returns_empty() { let tmp = TempDir::new().unwrap(); let paths = make_disk_builtin_paths(tmp.path()); // No auto-inject/ directory created under the disk override. - let autos = list_builtin_auto_skills(&paths).await.unwrap(); + let autos = list_auto_inject_skills_from_disk(&paths).await.unwrap(); assert!(autos.is_empty()); } @@ -1868,62 +2593,149 @@ mod tests { } #[tokio::test] - async fn import_skill_with_symlink_creates_link() { + async fn import_skills_imports_selected_skill_manifest_parent() { let tmp = TempDir::new().unwrap(); let paths = make_test_paths(tmp.path()); - let source_dir = tmp.path().join("link-skill"); + let source_dir = tmp.path().join("single-skill"); std::fs::create_dir_all(&source_dir).unwrap(); std::fs::write( source_dir.join(SKILL_MANIFEST_FILE), - "---\nname: linked\ndescription: Linked skill\n---\nBody", + "---\nname: selected-manifest\ndescription: Selected manifest skill\n---\nBody", ) .unwrap(); - let name = import_skill_with_symlink(&paths, &source_dir).await.unwrap(); - assert_eq!(name, "linked"); + let outcome = import_skills(&paths, &source_dir.join(SKILL_MANIFEST_FILE)) + .await + .unwrap(); + assert_eq!(outcome.imported, vec!["selected-manifest"]); + assert!(outcome.failed.is_empty()); + + let imported_path = paths.user_skills_dir.join("selected-manifest"); + assert!(!imported_path.is_symlink()); + assert!(imported_path.join(SKILL_MANIFEST_FILE).exists()); - let link_path = paths.user_skills_dir.join("linked"); - assert!(link_path.is_symlink()); + std::fs::remove_dir_all(&source_dir).unwrap(); + assert!(imported_path.join(SKILL_MANIFEST_FILE).exists()); } #[tokio::test] - async fn import_skills_with_symlink_imports_selected_skill_manifest_parent() { + async fn import_skills_imports_parent_directory_children() { let tmp = TempDir::new().unwrap(); let paths = make_test_paths(tmp.path()); - let source_dir = tmp.path().join("single-skill"); + let source_dir = tmp.path().join("skill-pack"); + create_skill_in_dir(&source_dir, "alpha", "Alpha skill"); + create_skill_in_dir(&source_dir, "beta", "Beta skill"); + + let outcome = import_skills(&paths, &source_dir).await.unwrap(); + assert_eq!(outcome.imported, vec!["alpha", "beta"]); + assert!(outcome.failed.is_empty()); + assert!(!paths.user_skills_dir.join("alpha").is_symlink()); + assert!(!paths.user_skills_dir.join("beta").is_symlink()); + assert!(paths.user_skills_dir.join("alpha").join(SKILL_MANIFEST_FILE).exists()); + assert!(paths.user_skills_dir.join("beta").join(SKILL_MANIFEST_FILE).exists()); + } + + #[tokio::test] + async fn import_skills_imports_mixed_folder_without_copying_non_skill_siblings() { + let tmp = TempDir::new().unwrap(); + let paths = make_test_paths(tmp.path()); + + let source_dir = tmp.path().join("mixed-import-root"); + std::fs::create_dir_all(&source_dir).unwrap(); + std::fs::write(source_dir.join("README.md"), "import notes").unwrap(); + std::fs::write(source_dir.join(".DS_Store"), "metadata").unwrap(); + std::fs::write(source_dir.join("skills.zip"), "not a real zip").unwrap(); + create_skill_in_dir(&source_dir, "alpha", "Alpha skill"); + create_skill_in_dir(&source_dir.join("nested-pack"), "beta", "Beta skill"); + + let outcome = import_skills(&paths, &source_dir).await.unwrap(); + + assert_eq!(outcome.imported, vec!["alpha", "beta"]); + assert!(outcome.failed.is_empty()); + assert!(!paths.user_skills_dir.join("mixed-import-root").exists()); + assert!(!paths.user_skills_dir.join("README.md").exists()); + assert!(!paths.user_skills_dir.join(".DS_Store").exists()); + assert!(!paths.user_skills_dir.join("skills.zip").exists()); + assert!(paths.user_skills_dir.join("alpha").join(SKILL_MANIFEST_FILE).exists()); + assert!(paths.user_skills_dir.join("beta").join(SKILL_MANIFEST_FILE).exists()); + } + + #[tokio::test] + async fn import_skill_copies_small_files_without_name_based_filtering() { + let tmp = TempDir::new().unwrap(); + let paths = make_test_paths(tmp.path()); + + let source_dir = tmp.path().join("small-files-source"); std::fs::create_dir_all(&source_dir).unwrap(); std::fs::write( source_dir.join(SKILL_MANIFEST_FILE), - "---\nname: selected-manifest\ndescription: Selected manifest skill\n---\nBody", + "---\nname: small-files\ndescription: Small files\n---\nBody", ) .unwrap(); + std::fs::write(source_dir.join(".DS_Store"), "finder metadata").unwrap(); + std::fs::create_dir_all(source_dir.join(".git")).unwrap(); + std::fs::write(source_dir.join(".git/config"), "repo metadata").unwrap(); - let names = import_skills_with_symlink(&paths, &source_dir.join(SKILL_MANIFEST_FILE)) - .await - .unwrap(); - assert_eq!(names, vec!["selected-manifest"]); + let name = import_skill(&paths, &source_dir).await.unwrap(); - let link_path = paths.user_skills_dir.join("selected-manifest"); - assert!(link_path.is_symlink()); - assert_eq!(std::fs::read_link(&link_path).unwrap(), source_dir); - assert!(link_path.join(SKILL_MANIFEST_FILE).exists()); + assert_eq!(name, "small-files"); + let imported = paths.user_skills_dir.join("small-files"); + assert!(imported.join(SKILL_MANIFEST_FILE).exists()); + assert!(imported.join(".DS_Store").exists()); + assert!(imported.join(".git/config").exists()); } #[tokio::test] - async fn import_skills_with_symlink_imports_parent_directory_children() { + async fn import_skill_rejects_oversized_files_without_leaving_partial_copy() { let tmp = TempDir::new().unwrap(); let paths = make_test_paths(tmp.path()); - let source_dir = tmp.path().join("skill-pack"); - create_skill_in_dir(&source_dir, "alpha", "Alpha skill"); - create_skill_in_dir(&source_dir, "beta", "Beta skill"); + let source_dir = tmp.path().join("large-source"); + std::fs::create_dir_all(&source_dir).unwrap(); + std::fs::write( + source_dir.join(SKILL_MANIFEST_FILE), + "---\nname: too-large\ndescription: Too large\n---\nBody", + ) + .unwrap(); + let large_file = std::fs::File::create(source_dir.join("movie.mp4")).unwrap(); + large_file.set_len(MAX_SKILL_IMPORT_FILE_BYTES + 1).unwrap(); - let names = import_skills_with_symlink(&paths, &source_dir).await.unwrap(); - assert_eq!(names, vec!["alpha", "beta"]); - assert!(paths.user_skills_dir.join("alpha").is_symlink()); - assert!(paths.user_skills_dir.join("beta").is_symlink()); + let result = import_skill(&paths, &source_dir).await; + + assert!(matches!(result, Err(ExtensionError::SkillImportFileTooLarge { .. }))); + assert!(!paths.user_skills_dir.join("too-large").exists()); + assert!(!has_import_staging_dirs(&paths.user_skills_dir)); + } + + #[tokio::test] + async fn import_skills_replaces_dangling_link_with_copy() { + let tmp = TempDir::new().unwrap(); + let paths = make_test_paths(tmp.path()); + + let stale_source = tmp.path().join("stale-source"); + create_skill_in_dir(&stale_source, "dangling", "Stale source"); + let stale_skill_dir = stale_source.join("dangling"); + let target = paths.user_skills_dir.join("dangling"); + tokio::fs::create_dir_all(&paths.user_skills_dir).await.unwrap(); + create_symlink(&stale_skill_dir, &target).await.unwrap(); + std::fs::remove_dir_all(&stale_source).unwrap(); + + let fresh_source = tmp.path().join("fresh-source"); + create_skill_in_dir(&fresh_source, "dangling", "Fresh source"); + + let outcome = import_skills(&paths, &fresh_source).await.unwrap(); + + assert_eq!(outcome.imported, vec!["dangling"]); + assert!(outcome.failed.is_empty()); + assert!(!target.is_symlink()); + assert!(target.join(SKILL_MANIFEST_FILE).exists()); + assert!( + std::fs::read_to_string(target.join(SKILL_MANIFEST_FILE)) + .unwrap() + .contains("Fresh source") + ); } #[tokio::test] @@ -1946,9 +2758,9 @@ mod tests { ) .unwrap(); - import_skill_with_symlink(&paths, &older_dir).await.unwrap(); + import_skill(&paths, &older_dir).await.unwrap(); std::thread::sleep(std::time::Duration::from_millis(20)); - import_skill_with_symlink(&paths, &newer_dir).await.unwrap(); + import_skill(&paths, &newer_dir).await.unwrap(); let skills = list_available_skills(&paths).await.unwrap(); let names: Vec<_> = skills.into_iter().map(|skill| skill.name).collect(); @@ -1957,7 +2769,98 @@ mod tests { } #[tokio::test] - async fn import_skills_with_symlink_imports_zip_package() { + async fn list_available_skills_with_repo_reads_database_without_filesystem_backfill() { + let tmp = TempDir::new().unwrap(); + let paths = make_test_paths(tmp.path()); + let repo = make_test_skill_repo().await; + + create_skill_in_dir(&paths.user_skills_dir, "existing-disk-skill", "Existing disk skill"); + + let skills = list_available_skills_with_repo(&paths, &repo).await.unwrap(); + + assert!(!skills.iter().any(|skill| skill.name == "existing-disk-skill")); + assert!(repo.find_by_name("existing-disk-skill").await.unwrap().is_none()); + } + + #[tokio::test] + async fn sync_skill_catalog_into_repo_backfills_existing_user_skill_dirs() { + let tmp = TempDir::new().unwrap(); + let paths = make_test_paths(tmp.path()); + let repo = make_test_skill_repo().await; + + create_skill_in_dir(&paths.user_skills_dir, "existing-disk-skill", "Existing disk skill"); + + sync_skill_catalog_into_repo(&paths, &repo).await.unwrap(); + + let row = repo.find_by_name("existing-disk-skill").await.unwrap().unwrap(); + assert_eq!(row.source, "user"); + assert_eq!( + row.path, + paths.user_skills_dir.join("existing-disk-skill").to_string_lossy() + ); + } + + #[tokio::test] + async fn list_available_skills_with_repo_does_not_restore_soft_deleted_disk_skill() { + let tmp = TempDir::new().unwrap(); + let paths = make_test_paths(tmp.path()); + let repo = make_test_skill_repo().await; + + create_skill_in_dir(tmp.path(), "deleted-disk-skill", "Deleted disk skill"); + import_skill_with_repo(&paths, &repo, &tmp.path().join("deleted-disk-skill")) + .await + .unwrap(); + delete_skill_with_repo(&paths, &repo, "deleted-disk-skill") + .await + .unwrap(); + + sync_skill_catalog_into_repo(&paths, &repo).await.unwrap(); + let skills = list_available_skills_with_repo(&paths, &repo).await.unwrap(); + + assert!(!skills.iter().any(|skill| skill.name == "deleted-disk-skill")); + assert!(repo.find_by_name("deleted-disk-skill").await.unwrap().is_none()); + assert!(repo.find_by_name_any("deleted-disk-skill").await.unwrap().is_some()); + } + + #[tokio::test] + async fn list_available_skills_with_repo_syncs_builtin_auto_inject_and_cron_sources_into_repo() { + let tmp = TempDir::new().unwrap(); + let paths = make_disk_builtin_paths(tmp.path()); + let repo = make_test_skill_repo().await; + let builtin_dir = disk_builtin_dir(&paths).to_path_buf(); + let auto_dir = builtin_dir.join(BUILTIN_AUTO_SKILLS_SUBDIR); + + create_skill_in_dir(&builtin_dir, "debug", "Debugging skill"); + create_skill_in_dir(&auto_dir, "cron", "Auto injected cron skill"); + create_skill_in_dir(&paths.cron_skills_dir, "scheduled-task", "Scheduled task skill"); + + sync_skill_catalog_into_repo(&paths, &repo).await.unwrap(); + let skills = list_available_skills_with_repo(&paths, &repo).await.unwrap(); + + let debug = skills.iter().find(|skill| skill.name == "debug").unwrap(); + assert_eq!(debug.source, SkillSource::Builtin); + assert_eq!(debug.relative_location.as_deref(), Some("debug/SKILL.md")); + let debug_row = repo.find_by_name("debug").await.unwrap().unwrap(); + assert_eq!(debug_row.source, "builtin"); + + let auto_cron = skills.iter().find(|skill| skill.name == "cron").unwrap(); + assert_eq!(auto_cron.source, SkillSource::Builtin); + assert_eq!( + auto_cron.relative_location.as_deref(), + Some("auto-inject/cron/SKILL.md") + ); + let auto_cron_row = repo.find_by_name("cron").await.unwrap().unwrap(); + assert_eq!(auto_cron_row.source, "builtin"); + + let scheduled = skills.iter().find(|skill| skill.name == "scheduled-task").unwrap(); + assert_eq!(scheduled.source, SkillSource::Cron); + assert_eq!(scheduled.relative_location, None); + let scheduled_row = repo.find_by_name("scheduled-task").await.unwrap().unwrap(); + assert_eq!(scheduled_row.source, "cron"); + } + + #[tokio::test] + async fn import_skills_imports_zip_package() { let tmp = TempDir::new().unwrap(); let paths = make_test_paths(tmp.path()); let zip_path = tmp.path().join("skills.zip"); @@ -1977,8 +2880,9 @@ mod tests { ], ); - let names = import_skills_with_symlink(&paths, &zip_path).await.unwrap(); - assert_eq!(names, vec!["zip-one", "zip-two"]); + let outcome = import_skills(&paths, &zip_path).await.unwrap(); + assert_eq!(outcome.imported, vec!["zip-one", "zip-two"]); + assert!(outcome.failed.is_empty()); assert!(paths.user_skills_dir.join("zip-one").join(SKILL_MANIFEST_FILE).exists()); assert!(paths.user_skills_dir.join("zip-one").join("data.txt").exists()); assert!(!paths.user_skills_dir.join("zip-one").is_symlink()); @@ -1986,14 +2890,14 @@ mod tests { } #[tokio::test] - async fn import_skills_with_symlink_rejects_zip_slip_entries() { + async fn import_skills_rejects_zip_slip_entries() { let tmp = TempDir::new().unwrap(); let paths = make_test_paths(tmp.path()); let zip_path = tmp.path().join("evil.zip"); write_test_zip(&zip_path, &[("../escape.txt", "outside")]); - let result = import_skills_with_symlink(&paths, &zip_path).await; + let result = import_skills(&paths, &zip_path).await; assert!(matches!(result, Err(ExtensionError::PathTraversal(_)))); assert!(!tmp.path().join("escape.txt").exists()); } @@ -2017,7 +2921,7 @@ mod tests { } #[tokio::test] - async fn import_skill_with_symlink_rejects_traversal_name() { + async fn import_skills_rejects_traversal_name() { let tmp = TempDir::new().unwrap(); let paths = make_test_paths(tmp.path()); @@ -2029,12 +2933,12 @@ mod tests { ) .unwrap(); - let result = import_skill_with_symlink(&paths, &source_dir).await; + let result = import_skills(&paths, &source_dir).await; assert!(matches!(result, Err(ExtensionError::PathTraversal(_)))); } #[tokio::test] - async fn import_skill_with_symlink_rejects_invalid_yaml_frontmatter() { + async fn import_skills_rejects_invalid_yaml_frontmatter() { let tmp = TempDir::new().unwrap(); let paths = make_test_paths(tmp.path()); @@ -2046,8 +2950,8 @@ mod tests { ) .unwrap(); - let result = import_skill_with_symlink(&paths, &source_dir).await; - assert!(matches!(result, Err(ExtensionError::InvalidSkillPath(_)))); + let result = import_skills(&paths, &source_dir).await; + assert!(matches!(result, Err(ExtensionError::SkillInvalidFrontmatter(_)))); assert!(!paths.user_skills_dir.join("invalid-frontmatter").exists()); } @@ -2062,6 +2966,38 @@ mod tests { assert!(!paths.user_skills_dir.join("to-delete").exists()); } + #[tokio::test] + async fn delete_custom_skill_with_repo_soft_deletes_without_removing_files() { + let tmp = TempDir::new().unwrap(); + let paths = make_test_paths(tmp.path()); + let repo = make_test_skill_repo().await; + + create_skill_in_dir(tmp.path(), "historical", "Used by an old conversation"); + import_skill_with_repo(&paths, &repo, &tmp.path().join("historical")) + .await + .unwrap(); + + delete_skill_with_repo(&paths, &repo, "historical").await.unwrap(); + + assert!( + paths + .user_skills_dir + .join("historical") + .join(SKILL_MANIFEST_FILE) + .exists() + ); + + let listed = list_available_skills_with_repo(&paths, &repo).await.unwrap(); + assert!(!listed.iter().any(|skill| skill.name == "historical")); + + let resolved = + materialize_skills_for_agent_with_repo(&paths, &repo, "old-conversation", &["historical".to_owned()]) + .await + .unwrap(); + assert_eq!(resolved.len(), 1); + assert_eq!(resolved[0].source_path, paths.user_skills_dir.join("historical")); + } + #[tokio::test] async fn delete_builtin_skill_rejected() { let tmp = TempDir::new().unwrap(); @@ -2185,6 +3121,11 @@ mod tests { &paths.builtin_skills_dir } + async fn make_test_skill_repo() -> SqliteSkillRepository { + let db = init_database_memory().await.unwrap(); + SqliteSkillRepository::new(db.pool().clone()) + } + fn create_skill_in_dir(base: &Path, name: &str, description: &str) { let dir = base.join(name); std::fs::create_dir_all(&dir).unwrap(); @@ -2195,6 +3136,14 @@ mod tests { .unwrap(); } + fn has_import_staging_dirs(dir: &Path) -> bool { + std::fs::read_dir(dir) + .ok() + .into_iter() + .flat_map(|entries| entries.filter_map(Result::ok)) + .any(|entry| entry.file_name().to_string_lossy().starts_with(IMPORT_STAGING_PREFIX)) + } + fn create_resolved_test_skill(source_root: &Path, name: &str) -> ResolvedAgentSkill { let source_path = source_root.join(name); std::fs::create_dir_all(&source_path).unwrap(); @@ -2275,7 +3224,7 @@ mod tests { let tmp = TempDir::new().unwrap(); let paths = make_embedded_paths(tmp.path()).await; - let autos = list_builtin_auto_skills(&paths).await.unwrap(); + let autos = list_auto_inject_skills_from_disk(&paths).await.unwrap(); assert!(autos.len() >= 3, "expected ≥3 auto-inject entries, got {}", autos.len()); for item in &autos { assert!( @@ -2331,7 +3280,7 @@ mod tests { let auto_dir = builtin_dir.join(BUILTIN_AUTO_SKILLS_SUBDIR); create_skill_in_dir(&auto_dir, "fixture-only", "Fixture-only skill"); - let autos = list_builtin_auto_skills(&paths).await.unwrap(); + let autos = list_auto_inject_skills_from_disk(&paths).await.unwrap(); let names: Vec<&str> = autos.iter().map(|s| s.name.as_str()).collect(); assert!( names.contains(&"fixture-only"), diff --git a/crates/aionui-extension/tests/assistant_dispatch_test.rs b/crates/aionui-extension/tests/assistant_dispatch_test.rs index 9929294a2..48e5a37c1 100644 --- a/crates/aionui-extension/tests/assistant_dispatch_test.rs +++ b/crates/aionui-extension/tests/assistant_dispatch_test.rs @@ -8,7 +8,7 @@ use std::path::PathBuf; use std::sync::{Arc, Mutex}; -use aionui_api_types::{ApiResponse, AssistantSource}; +use aionui_api_types::{ApiResponse, AssistantSource, SkillImportLimitsResponse}; use aionui_extension::classifier::{AssistantClassifier, AssistantRuleDispatcher}; use aionui_extension::error::ExtensionError; use aionui_extension::external_paths::ExternalPathsManager; @@ -139,10 +139,13 @@ async fn router_with_dispatcher(dispatcher: Arc) -> axum::Router assistant_skills_dir: root.join("assistant-skills"), }; let ext_mgr = Arc::new(ExternalPathsManager::with_file(root.join("paths.json")).await); + let db = aionui_db::init_database_memory().await.unwrap(); + let skill_repo = Arc::new(aionui_db::SqliteSkillRepository::new(db.pool().clone())); std::mem::forget(tmp); let state = SkillRouterState { skill_paths: paths, + skill_repo, external_paths_manager: ext_mgr, assistant_dispatcher: Some(dispatcher), }; @@ -182,6 +185,31 @@ fn write_body(assistant_id: &str, content: &str, locale: Option<&str>) -> Vec = body_json(resp).await; + let limits = body.data.expect("import limits response data"); + assert_eq!(limits.max_file_bytes, 10 * 1024 * 1024); + assert_eq!(limits.max_total_bytes, 50 * 1024 * 1024); +} + #[tokio::test] async fn read_rule_routes_through_dispatcher_for_builtin() { let mut rule_content = std::collections::HashMap::new(); diff --git a/crates/aionui-extension/tests/skill_integration_test.rs b/crates/aionui-extension/tests/skill_integration_test.rs index 7b8c67caa..515d04ce3 100644 --- a/crates/aionui-extension/tests/skill_integration_test.rs +++ b/crates/aionui-extension/tests/skill_integration_test.rs @@ -9,9 +9,9 @@ use std::path::Path; use aionui_extension::external_paths::ExternalPathsManager; use aionui_extension::skill_service::{ NamedPath, SkillPaths, delete_assistant_rule, delete_assistant_skill, delete_skill, - detect_and_count_external_skills, export_skill_with_symlink, import_skill, import_skill_with_symlink, - list_available_skills, read_assistant_rule, read_assistant_skill, read_builtin_rule, read_builtin_skill, - read_skill_info, resolve_skill_paths, scan_for_skills, write_assistant_rule, write_assistant_skill, + detect_and_count_external_skills, export_skill_with_symlink, import_skill, list_available_skills, + read_assistant_rule, read_assistant_skill, read_builtin_rule, read_builtin_skill, read_skill_info, + resolve_skill_paths, scan_for_skills, write_assistant_rule, write_assistant_skill, }; use tempfile::TempDir; @@ -140,9 +140,9 @@ async fn sm3_import_skill_copy() { assert!(imported.join("helper.py").exists()); } -/// SM-4: Import skill (symlink). +/// SM-4: Import skill replaces existing imported copy. #[tokio::test] -async fn sm4_import_skill_symlink() { +async fn sm4_import_skill_replaces_existing_copy() { let tmp = TempDir::new().unwrap(); let paths = make_paths(tmp.path()); @@ -154,15 +154,22 @@ async fn sm4_import_skill_symlink() { ) .unwrap(); - let name = import_skill_with_symlink(&paths, &source).await.unwrap(); + let name = import_skill(&paths, &source).await.unwrap(); assert_eq!(name, "linked"); - let link = paths.user_skills_dir.join("linked"); - assert!(link.is_symlink()); + std::fs::write( + source.join(SKILL_MD), + "---\nname: linked\ndescription: Linked skill\n---\nUpdated body.", + ) + .unwrap(); - // Verify content is accessible through the symlink - let content = std::fs::read_to_string(link.join(SKILL_MD)).unwrap(); - assert!(content.contains("Linked skill")); + let name = import_skill(&paths, &source).await.unwrap(); + assert_eq!(name, "linked"); + + let imported = paths.user_skills_dir.join("linked"); + assert!(!imported.is_symlink()); + let content = std::fs::read_to_string(imported.join(SKILL_MD)).unwrap(); + assert!(content.contains("Updated body")); } /// SM-5: Export skill (symlink). @@ -196,6 +203,9 @@ async fn sm6_delete_custom_skill() { delete_skill(&paths, "deletable").await.unwrap(); assert!(!paths.user_skills_dir.join("deletable").exists()); + + let skills = list_available_skills(&paths).await.unwrap(); + assert!(!skills.iter().any(|skill| skill.name == "deletable")); } /// SM-7: Delete built-in skill → rejected.