diff --git a/src-tauri/src/services/skill_deployment.rs b/src-tauri/src/services/skill_deployment.rs index a80b3b17c..d669e7c04 100644 --- a/src-tauri/src/services/skill_deployment.rs +++ b/src-tauri/src/services/skill_deployment.rs @@ -84,7 +84,7 @@ impl PiSkillDeploymentService { existing, Some(true), )?; - cleanup_stale_deployments(db, skill, &destination_key)?; + cleanup_stale_deployments_after_commit(db, skill, &destination_key); } else { remove_all_recorded_deployments(db, skill, Some(false))?; } @@ -124,7 +124,7 @@ impl PiSkillDeploymentService { Some(true), )?; } - cleanup_stale_deployments(db, skill, &destination_key)?; + cleanup_stale_deployments_after_commit(db, skill, &destination_key); } else { remove_all_recorded_deployments(db, skill, Some(false))?; } @@ -411,7 +411,8 @@ fn reconcile_skill_unlocked(db: &Arc, skill: &InstalledSkill) -> Resul existing, None, )?; - cleanup_stale_deployments(db, skill, &destination_key) + cleanup_stale_deployments_after_commit(db, skill, &destination_key); + Ok(()) } else { remove_all_recorded_deployments(db, skill, None) } @@ -430,6 +431,25 @@ fn cleanup_stale_deployments( Ok(()) } +/// `deploy` atomically commits the new native destination, its ownership +/// ledger, and (when requested) desired state before old-root cleanup begins. +/// A drifted old root must remain visible as stale ownership evidence, but it +/// must not turn that committed publication into an error: several callers +/// compensate a returned error by deleting the Skill's SSOT/database row. +fn cleanup_stale_deployments_after_commit( + db: &Arc, + skill: &InstalledSkill, + current_destination_key: &str, +) { + if let Err(error) = cleanup_stale_deployments(db, skill, current_destination_key) { + log::warn!( + "Pi Skill '{}' was published at its current agent root, but stale deployment cleanup \ + remains pending: {error}", + skill.id + ); + } +} + fn remove_all_recorded_deployments( db: &Arc, skill: &InstalledSkill, @@ -1238,4 +1258,96 @@ mod tests { destination_key(&new_destination) ); } + + #[test] + #[serial_test::serial] + fn drifted_old_root_is_post_commit_status_not_a_failed_new_publication() { + struct EnvGuard { + key: &'static str, + previous: Option, + } + impl EnvGuard { + fn set(key: &'static str, value: &Path) -> Self { + let previous = std::env::var_os(key); + std::env::set_var(key, value); + Self { key, previous } + } + } + impl Drop for EnvGuard { + fn drop(&mut self) { + match self.previous.take() { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + if self.key == "CC_SWITCH_TEST_HOME" { + let _ = crate::settings::reload_settings(); + } + } + } + + let temp = tempfile::tempdir().expect("tempdir"); + let _home = EnvGuard::set("CC_SWITCH_TEST_HOME", temp.path()); + crate::settings::reload_settings().expect("reload settings"); + let old_root = temp.path().join("old-pi"); + let new_root = temp.path().join("new-pi"); + let _pi_root = EnvGuard::set("PI_CODING_AGENT_DIR", &old_root); + let source = SkillService::get_ssot_dir() + .expect("SSOT") + .join("drifted-relocation"); + fs::create_dir_all(&source).expect("source"); + fs::write( + source.join("SKILL.md"), + "---\nname: drifted-relocation\ndescription: relocation test\n---\n", + ) + .expect("manifest"); + let mut skill = InstalledSkill { + id: "local:drifted-relocation".to_string(), + name: "Drifted relocation".to_string(), + description: Some("relocation test".to_string()), + directory: "drifted-relocation".to_string(), + repo_owner: None, + repo_name: None, + repo_branch: None, + readme_url: None, + apps: SkillApps::only(&AppType::Pi), + installed_at: 1, + content_hash: None, + updated_at: 1, + }; + let db = Arc::new(Database::memory().expect("database")); + db.save_skill(&skill).expect("save skill"); + PiSkillDeploymentService::reconcile_skill(&db, &skill).expect("old deployment"); + let old_destination = old_root.join("skills").join(&skill.directory); + remove_path(&old_destination).expect("replace old deployment"); + fs::create_dir_all(&old_destination).expect("foreign old destination"); + fs::write(old_destination.join("external.txt"), "external").expect("external drift"); + + std::env::set_var("PI_CODING_AGENT_DIR", &new_root); + PiSkillDeploymentService::toggle(&db, &mut skill, true) + .expect("the new publication is already committed"); + + let new_destination = new_root.join("skills").join(&skill.directory); + assert!(fs::symlink_metadata(&new_destination).is_ok()); + assert_eq!( + fs::read_to_string(old_destination.join("external.txt")).expect("external survives"), + "external" + ); + let stored = db + .get_installed_skill(&skill.id) + .expect("read desired state") + .expect("skill remains"); + assert!(stored.apps.pi); + assert_eq!( + db.get_pi_skill_deployments(&skill.id) + .expect("ledger") + .len(), + 2, + "old drift evidence and the committed new deployment must both remain" + ); + let status = + PiSkillDeploymentService::inspect_all(&db).expect("inspect")[&skill.id].clone(); + assert_eq!(status.ownership, PiSkillOwnership::Stale); + assert!(status.effectively_discovered); + assert!(status.issue.is_some_and(|issue| issue.contains("previous"))); + } }