diff --git a/src-tauri/src/database/dao/skill_deployments.rs b/src-tauri/src/database/dao/skill_deployments.rs index 5add7533e..7f1133ee6 100644 --- a/src-tauri/src/database/dao/skill_deployments.rs +++ b/src-tauri/src/database/dao/skill_deployments.rs @@ -70,6 +70,26 @@ fn decode_deployment(row: &rusqlite::Row<'_>) -> rusqlite::Result Result<(), AppError> { + let conn = lock_conn!(self.conn); + let changed = conn + .execute( + "UPDATE skills SET enabled_pi = ?1 WHERE id = ?2", + params![desired_enabled, skill_id], + ) + .map_err(|error| AppError::Database(error.to_string()))?; + if changed != 1 { + return Err(AppError::Conflict(format!( + "Pi Skill '{skill_id}' disappeared before desired state was saved" + ))); + } + Ok(()) + } + pub(crate) fn get_pi_skill_deployment( &self, skill_id: &str, diff --git a/src-tauri/src/services/skill.rs b/src-tauri/src/services/skill.rs index e8f9ddeb0..960b1e576 100644 --- a/src-tauri/src/services/skill.rs +++ b/src-tauri/src/services/skill.rs @@ -3275,6 +3275,10 @@ impl SkillService { let ssot_dir = Self::get_ssot_dir()?; let mut installed = Vec::new(); let existing_skills = db.get_all_installed_skills()?; + let mut claimed_directories = existing_skills + .values() + .map(|skill| skill.directory.to_ascii_lowercase()) + .collect::>(); let zip_stem = zip_path .file_stem() .and_then(|s| s.to_str()) @@ -3341,6 +3345,32 @@ impl SkillService { ); continue; } + if claimed_directories.contains(&install_name.to_ascii_lowercase()) { + log::warn!( + "Skill directory '{}' appears more than once in the archive, skipping", + install_name + ); + continue; + } + + if matches!(current_app, AppType::Pi) + && meta.as_ref().is_none_or(|metadata| { + metadata + .name + .as_deref() + .is_none_or(|name| name.trim().is_empty()) + || metadata + .description + .as_deref() + .is_none_or(|description| description.trim().is_empty()) + }) + { + return Err(anyhow!(format_skill_error( + "INVALID_SKILL_DIRECTORY", + &[("directory", &install_name)], + Some("checkSkillManifest"), + ))); + } let (name, description) = match meta { Some(m) => ( @@ -3350,10 +3380,17 @@ impl SkillService { None => (install_name.clone(), None), }; + let deployment_guard = matches!(current_app, AppType::Pi) + .then(crate::services::skill_deployment::PiSkillDeploymentService::operation_guard); + // 复制到 SSOT let dest = ssot_dir.join(&install_name); - if dest.exists() { - let _ = fs::remove_dir_all(&dest); + if fs::symlink_metadata(&dest).is_ok() { + return Err(anyhow!(format_skill_error( + "SKILL_DIRECTORY_CONFLICT", + &[("directory", &install_name)], + Some("uninstallFirst"), + ))); } Self::copy_dir_recursive(&skill_dir, &dest)?; @@ -3361,7 +3398,7 @@ impl SkillService { let content_hash = Self::compute_dir_hash(&dest).ok(); // 创建 InstalledSkill 记录 - let skill = InstalledSkill { + let mut skill = InstalledSkill { id: format!("local:{install_name}"), name, description, @@ -3376,17 +3413,47 @@ impl SkillService { updated_at: 0, }; - // 保存到数据库 - db.save_skill(&skill)?; - - // 同步到当前应用目录 - Self::sync_installed_skill_to_app(db, &skill, current_app)?; + if let Some(guard) = deployment_guard.as_ref() { + // The coordinator commits Pi desired state and ownership + // evidence together. Until then the portable row is inert. + skill.apps.pi = false; + if let Err(error) = db.save_skill(&skill) { + let _ = fs::remove_dir_all(&dest); + return Err(error.into()); + } + if let Err(error) = + crate::services::skill_deployment::PiSkillDeploymentService::toggle_under_guard( + guard, db, &mut skill, true, + ) + { + let db_rollback = db.delete_skill(&skill.id); + let file_rollback = fs::remove_dir_all(&dest); + return match (db_rollback, file_rollback) { + (Ok(true), Ok(())) => Err(anyhow!(error.to_string())), + (db_result, file_result) => Err(anyhow!( + "Pi Skill ZIP install failed ({error}); DB rollback: {}; SSOT rollback: {}", + match db_result { + Ok(true) => "ok".to_string(), + Ok(false) => "row missing".to_string(), + Err(value) => value.to_string(), + }, + file_result + .err() + .map_or_else(|| "ok".to_string(), |value| value.to_string()) + )), + }; + } + } else { + db.save_skill(&skill)?; + Self::sync_installed_skill_to_app(db, &skill, current_app)?; + } log::info!( "Skill {} installed from ZIP, enabled for {:?}", skill.name, current_app ); + claimed_directories.insert(install_name.to_ascii_lowercase()); installed.push(skill); } @@ -4504,6 +4571,86 @@ mod tests { ); } + #[test] + #[serial_test::serial] + fn pi_zip_collision_rolls_back_database_and_ssot_without_touching_native_skill() { + use std::io::Write; + use zip::write::SimpleFileOptions; + + struct PiDirGuard(Option); + impl Drop for PiDirGuard { + fn drop(&mut self) { + match self.0.take() { + Some(value) => std::env::set_var("PI_CODING_AGENT_DIR", value), + None => std::env::remove_var("PI_CODING_AGENT_DIR"), + } + } + } + struct StorageLocationGuard(SkillStorageLocation); + impl Drop for StorageLocationGuard { + fn drop(&mut self) { + let _ = crate::settings::set_skill_storage_location(self.0); + } + } + + let temp = tempdir().expect("tempdir"); + let _home_guard = TestHomeGuard::set(temp.path()); + let pi_agent_dir = temp.path().join("pi-agent"); + let _pi_dir_guard = PiDirGuard(std::env::var_os("PI_CODING_AGENT_DIR")); + std::env::set_var("PI_CODING_AGENT_DIR", &pi_agent_dir); + let _storage_guard = StorageLocationGuard(crate::settings::get_skill_storage_location()); + crate::settings::set_skill_storage_location(SkillStorageLocation::CcSwitch) + .expect("isolated SSOT"); + + let native = pi_agent_dir.join("skills").join("collision"); + write_skill(&native, "Native collision"); + fs::write(native.join("native.txt"), "must survive").expect("native bytes"); + + let mut archive = Vec::new(); + { + let mut zip = zip::ZipWriter::new(std::io::Cursor::new(&mut archive)); + let options = SimpleFileOptions::default(); + zip.start_file("collision/SKILL.md", options) + .expect("manifest entry"); + zip.write_all(b"---\nname: Imported\ndescription: Imported collision\n---\n") + .expect("manifest bytes"); + zip.start_file("collision/imported.txt", options) + .expect("payload entry"); + zip.write_all(b"must not remain").expect("payload bytes"); + zip.finish().expect("finish zip"); + } + let zip_path = temp.path().join("collision.zip"); + fs::write(&zip_path, archive).expect("write zip"); + let db = Arc::new(Database::memory().expect("database")); + + SkillService::install_from_zip(&db, &zip_path, &AppType::Pi) + .expect_err("unowned native collision must fail"); + + assert_eq!( + fs::read_to_string(native.join("native.txt")).expect("native survives"), + "must survive" + ); + assert!( + db.get_installed_skill("local:collision") + .expect("read skill") + .is_none(), + "failed ZIP install must not leave desired state" + ); + assert!( + db.get_pi_skill_deployments("local:collision") + .expect("read ledger") + .is_empty(), + "failed ZIP install must not create ownership evidence" + ); + assert!( + !SkillService::get_ssot_dir() + .expect("SSOT") + .join("collision") + .exists(), + "failed ZIP install must compensate its SSOT copy" + ); + } + fn poisoned_skill(id: &str, directory: &str) -> InstalledSkill { InstalledSkill { id: id.to_string(), diff --git a/src-tauri/src/services/skill_deployment.rs b/src-tauri/src/services/skill_deployment.rs index c6b063fbf..b2622d737 100644 --- a/src-tauri/src/services/skill_deployment.rs +++ b/src-tauri/src/services/skill_deployment.rs @@ -283,6 +283,20 @@ fn scan_pi_discovery() -> Result { ); continue; }; + if parsed + .description + .as_deref() + .is_none_or(|description| description.trim().is_empty()) + { + by_manifest.insert( + manifest, + ( + PiSkillDiscovery::Invalid, + Some("SKILL.md has no non-empty frontmatter description".to_string()), + ), + ); + continue; + } if let Some(winner) = winner_by_name.get(&name) { by_manifest.insert( manifest, @@ -606,15 +620,14 @@ fn remove_owned( existing: Option, desired_enabled: Option, ) -> Result<(), AppError> { + if let Some(desired_enabled) = desired_enabled { + // Persist user intent before filesystem validation or cleanup. Drift + // therefore reports a conflict while the toggle remains off and the + // ledger is retained as deletion evidence. + db.set_pi_skill_desired(&skill.id, desired_enabled)?; + } let Some(existing) = existing else { // Foreign/native discovered directories are preserved. - if let Some(desired_enabled) = desired_enabled { - db.delete_pi_skill_deployment_with_desired( - &skill.id, - destination_key, - Some(desired_enabled), - )?; - } return Ok(()); }; verify_owned_destination(&existing, destination)?; @@ -627,9 +640,7 @@ fn remove_owned( })?; return Err(error); } - if let Err(error) = - db.delete_pi_skill_deployment_with_desired(&skill.id, destination_key, desired_enabled) - { + if let Err(error) = db.delete_pi_skill_deployment(&skill.id, destination_key) { restore_staged_destination(&staged, destination).map_err(|rollback| { AppError::Conflict(format!( "Pi Skill ledger cleanup failed ({error}); file rollback failed ({rollback})" @@ -932,6 +943,7 @@ fn remove_path(path: &Path) -> Result<(), AppError> { #[cfg(test)] mod tests { use super::*; + use crate::app_config::SkillApps; #[test] fn digest_includes_hidden_files_and_rejects_symlinks() { @@ -966,4 +978,102 @@ mod tests { "foreign" ); } + + #[test] + fn disabling_a_drifted_owned_skill_persists_intent_and_keeps_ledger() { + let temp = tempfile::tempdir().expect("tempdir"); + let destination = temp.path().join("skill"); + fs::create_dir(&destination).expect("destination"); + fs::write( + destination.join("SKILL.md"), + "---\nname: skill\ndescription: before\n---\n", + ) + .expect("manifest"); + let original_digest = tree_digest(&destination).expect("original digest"); + let mut skill = InstalledSkill { + id: "local:skill".to_string(), + name: "Skill".to_string(), + description: Some("before".to_string()), + directory: "skill".to_string(), + repo_owner: None, + repo_name: None, + repo_branch: None, + readme_url: None, + apps: SkillApps::only(&AppType::Pi), + installed_at: 1, + content_hash: Some(original_digest.clone()), + updated_at: 1, + }; + let db = Arc::new(Database::memory().expect("database")); + db.save_skill(&skill).expect("save skill"); + let key = destination_key(&destination); + db.save_pi_skill_deployment(&SkillDeployment { + skill_id: skill.id.clone(), + destination: destination.to_string_lossy().into_owned(), + destination_key: key.clone(), + method: SkillDeploymentMethod::Copy, + source_identity: format!("path:{};digest:{original_digest}", destination.display()), + deployed_digest: Some(original_digest), + created_at: 1, + updated_at: 1, + }) + .expect("save ledger"); + + fs::write(destination.join("changed.txt"), "external drift").expect("drift"); + let error = remove_owned( + &db, + &skill, + &destination, + &key, + db.get_pi_skill_deployment(&skill.id, &key) + .expect("read ledger"), + Some(false), + ) + .expect_err("drift must block deletion"); + assert!(matches!(error, AppError::Conflict(_))); + + skill = db + .get_installed_skill(&skill.id) + .expect("read skill") + .expect("skill remains"); + assert!(!skill.apps.pi, "desired state must remain disabled"); + assert!( + db.get_pi_skill_deployment(&skill.id, &key) + .expect("read ledger") + .is_some(), + "drift evidence must remain for explicit resolution" + ); + assert!(destination.join("changed.txt").is_file()); + } + + #[test] + #[serial_test::serial] + fn discovery_rejects_a_manifest_without_pinned_required_description() { + struct EnvGuard(Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + match self.0.take() { + Some(value) => std::env::set_var("PI_CODING_AGENT_DIR", value), + None => std::env::remove_var("PI_CODING_AGENT_DIR"), + } + } + } + + let temp = tempfile::tempdir().expect("tempdir"); + let _guard = EnvGuard(std::env::var_os("PI_CODING_AGENT_DIR")); + std::env::set_var("PI_CODING_AGENT_DIR", temp.path()); + let manifest = temp.path().join("skills").join("invalid").join("SKILL.md"); + fs::create_dir_all(manifest.parent().expect("manifest parent")).expect("skills dir"); + fs::write(&manifest, "---\nname: invalid\n---\n").expect("manifest"); + + let discovery = scan_pi_discovery().expect("scan"); + assert_eq!( + discovery.by_manifest[&manifest].0, + PiSkillDiscovery::Invalid + ); + assert!(discovery.by_manifest[&manifest] + .1 + .as_deref() + .is_some_and(|issue| issue.contains("description"))); + } }