fix(pi): preserve skill ownership through install failures

src-tauri/src/database/dao/skill_deployments.rs

@@ -70,6 +70,26 @@ fn decode_deployment(row: &rusqlite::Row<'_>) -> rusqlite::Result<SkillDeploymen
 }
 
 impl Database {
+    pub(crate) fn set_pi_skill_desired(
+        &self,
+        skill_id: &str,
+        desired_enabled: bool,
+    ) -> 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,

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::<HashSet<_>>();
         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<std::ffi::OsString>);
+        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(),

src-tauri/src/services/skill_deployment.rs

@@ -283,6 +283,20 @@ fn scan_pi_discovery() -> Result<PiDiscoveryScan, AppError> {
             );
             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<SkillDeployment>,
     desired_enabled: Option<bool>,
 ) -> 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<std::ffi::OsString>);
+        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")));
+    }
 }