diff --git a/src-tauri/src/services/prompt.rs b/src-tauri/src/services/prompt.rs index 6788fb6de..a1064abce 100644 --- a/src-tauri/src/services/prompt.rs +++ b/src-tauri/src/services/prompt.rs @@ -28,9 +28,6 @@ impl PromptService { state: &AppState, app: AppType, ) -> Result, AppError> { - if matches!(app, AppType::Pi) { - Self::reconcile_pi_portable_import(state)?; - } state.db.get_prompts(app.as_str()) } @@ -697,4 +694,55 @@ mod tests { assert!(prompts.values().all(|prompt| !prompt.enabled)); assert!(!temp.path().join("AGENTS.md").exists()); } + + #[test] + #[serial] + fn reading_pi_prompts_does_not_adopt_external_agents_drift() { + let temp = tempfile::tempdir().expect("tempdir"); + let _restore = EnvRestore::set("PI_CODING_AGENT_DIR", temp.path()); + std::fs::write(temp.path().join("AGENTS.md"), "managed-before").expect("seed AGENTS.md"); + let state = AppState::new(Arc::new(Database::memory().expect("database"))); + state + .db + .save_prompt( + AppType::Pi.as_str(), + &Prompt { + id: "managed".to_string(), + name: "Managed".to_string(), + content: "managed-before".to_string(), + description: None, + enabled: true, + created_at: Some(1), + updated_at: Some(1), + }, + ) + .expect("seed prompt"); + state + .db + .save_prompt( + AppType::Pi.as_str(), + &Prompt { + id: "other".to_string(), + name: "Other".to_string(), + content: "other-content".to_string(), + description: None, + enabled: false, + created_at: Some(2), + updated_at: Some(2), + }, + ) + .expect("seed alternate prompt"); + + std::fs::write(temp.path().join("AGENTS.md"), "external-after").expect("external edit"); + let prompts = PromptService::get_prompts(&state, AppType::Pi).expect("read prompt list"); + assert_eq!(prompts["managed"].content, "managed-before"); + assert!( + PromptService::enable_prompt(&state, AppType::Pi, "other").is_err(), + "the write boundary must still report the external drift conflict" + ); + assert_eq!( + std::fs::read_to_string(temp.path().join("AGENTS.md")).expect("live AGENTS.md"), + "external-after" + ); + } } diff --git a/src-tauri/src/services/proxy.rs b/src-tauri/src/services/proxy.rs index 758e38836..b4acbe858 100644 --- a/src-tauri/src/services/proxy.rs +++ b/src-tauri/src/services/proxy.rs @@ -722,6 +722,26 @@ impl ProxyService { return crate::settings::update_settings(next); } + let direct_patch = direct_pi_projection_patch(self.db.as_ref())?; + let new_native_before = crate::pi_config::document::snapshot_pi_provider_values( + &new_models_path, + direct_patch.keys().cloned(), + )?; + for (provider_key, expected) in &direct_patch { + if let Some(existing_value) = new_native_before + .values + .get(provider_key) + .and_then(Option::as_ref) + { + if Some(existing_value) != expected.as_ref() { + return Err(AppError::Conflict(format!( + "Pi directory change would overwrite unowned provider key '{provider_key}' in {}", + new_models_path.display() + ))); + } + } + } + // Prompt operations and directory ownership share one sendable mutex. // Holding it across runtime publication prevents a prompt write from // committing against the new root while a failed directory move is @@ -731,6 +751,19 @@ impl ProxyService { if !existing.pi_takeover_enabled { crate::settings::update_settings(next)?; + if let Err(error) = + crate::pi_config::document::apply_pi_provider_patch(&new_models_path, &direct_patch) + { + let settings_restored = crate::settings::update_settings(existing.clone()).is_ok(); + let new_native_restored = crate::pi_config::document::restore_pi_provider_values( + &new_models_path, + &new_native_before, + ) + .is_ok(); + return Err(AppError::Config(format!( + "failed to publish managed Pi providers in the new directory: {error}; rollback: settings={settings_restored}, native={new_native_restored}" + ))); + } if let Err(error) = PromptService::reconcile_pi_native_under_guard(self.db.as_ref(), &prompt_guard) { @@ -739,8 +772,35 @@ impl ProxyService { .db .save_prompt_selection(AppType::Pi.as_str(), &previous_prompts) .is_ok(); + let new_native_restored = crate::pi_config::document::restore_pi_provider_values( + &new_models_path, + &new_native_before, + ) + .is_ok(); return Err(AppError::Config(format!( - "failed to reconcile Pi prompts in the new directory: {error}; rollback: settings={settings_restored}, prompts={prompts_restored}" + "failed to reconcile Pi prompts in the new directory: {error}; rollback: settings={settings_restored}, prompts={prompts_restored}, native={new_native_restored}" + ))); + } + if let Err(error) = + crate::services::skill_deployment::PiSkillDeploymentService::reconcile_all(&self.db) + { + let settings_restored = crate::settings::update_settings(existing.clone()).is_ok(); + let prompts_restored = self + .db + .save_prompt_selection(AppType::Pi.as_str(), &previous_prompts) + .is_ok(); + let new_native_restored = crate::pi_config::document::restore_pi_provider_values( + &new_models_path, + &new_native_before, + ) + .is_ok(); + let skills_restored = settings_restored + && crate::services::skill_deployment::PiSkillDeploymentService::reconcile_all( + &self.db, + ) + .is_ok(); + return Err(AppError::Config(format!( + "failed to reconcile Pi Skills in the new directory: {error}; rollback: settings={settings_restored}, prompts={prompts_restored}, native={new_native_restored}, skills={skills_restored}" ))); } return Ok(()); @@ -776,28 +836,69 @@ impl ProxyService { .db .save_prompt_selection(AppType::Pi.as_str(), &previous_prompts) .is_ok(); + let new_native_restored = crate::pi_config::document::restore_pi_provider_values( + &new_models_path, + &new_native_before, + ) + .is_ok(); + let skills_restored = settings_restored + && crate::services::skill_deployment::PiSkillDeploymentService::reconcile_all( + &self.db, + ) + .is_ok(); let runtime_restored = self.reconcile_pi_runtime_at_epoch(epoch).await.is_ok(); return Err(AppError::Config(format!( - "failed to reconcile Pi prompts in the new directory: {error}; rollback: settings={settings_restored}, prompts={prompts_restored}, gateway={runtime_restored}" + "failed to reconcile Pi prompts in the new directory: {error}; rollback: settings={settings_restored}, prompts={prompts_restored}, native={new_native_restored}, skills={skills_restored}, gateway={runtime_restored}" ))); } - if let Err(error) = self.reconcile_pi_runtime_at_epoch(epoch).await { - let new_direct_restored = self - .restore_pi_direct_projection_at(&new_models_path) - .is_ok(); + if let Err(error) = + crate::services::skill_deployment::PiSkillDeploymentService::reconcile_all(&self.db) + { let settings_restored = crate::settings::update_settings(existing.clone()).is_ok(); let prompts_restored = self .db .save_prompt_selection(AppType::Pi.as_str(), &previous_prompts) .is_ok(); + let new_native_restored = crate::pi_config::document::restore_pi_provider_values( + &new_models_path, + &new_native_before, + ) + .is_ok(); + let skills_restored = settings_restored + && crate::services::skill_deployment::PiSkillDeploymentService::reconcile_all( + &self.db, + ) + .is_ok(); + let runtime_restored = self.reconcile_pi_runtime_at_epoch(epoch).await.is_ok(); + return Err(AppError::Config(format!( + "failed to reconcile Pi Skills in the new directory: {error}; rollback: settings={settings_restored}, prompts={prompts_restored}, native={new_native_restored}, skills={skills_restored}, gateway={runtime_restored}" + ))); + } + + if let Err(error) = self.reconcile_pi_runtime_at_epoch(epoch).await { + let settings_restored = crate::settings::update_settings(existing.clone()).is_ok(); + let prompts_restored = self + .db + .save_prompt_selection(AppType::Pi.as_str(), &previous_prompts) + .is_ok(); + let new_native_restored = crate::pi_config::document::restore_pi_provider_values( + &new_models_path, + &new_native_before, + ) + .is_ok(); + let skills_restored = settings_restored + && crate::services::skill_deployment::PiSkillDeploymentService::reconcile_all( + &self.db, + ) + .is_ok(); let old_gateway_restored = if settings_restored { self.reconcile_pi_runtime_at_epoch(epoch).await.is_ok() } else { self.pi_runtime.republish_current(epoch).await.is_ok() }; return Err(AppError::Config(format!( - "failed to publish Pi in the new native directory: {error}; rollback: new_direct={new_direct_restored}, settings={settings_restored}, prompts={prompts_restored}, old_gateway={old_gateway_restored}" + "failed to publish Pi in the new native directory: {error}; rollback: native={new_native_restored}, settings={settings_restored}, prompts={prompts_restored}, skills={skills_restored}, old_gateway={old_gateway_restored}" ))); } @@ -4001,6 +4102,10 @@ mod tests { Some(value) => env::set_var("CC_SWITCH_TEST_HOME", value), None => env::remove_var("CC_SWITCH_TEST_HOME"), } + // Tests mutate the process-global settings cache after redirecting + // the home directory. Restore the cache after the environment so + // later serial tests do not inherit a dead temporary Pi override. + let _ = crate::settings::reload_settings(); } } @@ -4123,6 +4228,33 @@ mod tests { crate::settings::update_settings(settings).expect("set old Pi directory"); let db = Arc::new(Database::memory().expect("in-memory database")); + let skill_source = crate::services::skill::SkillService::get_ssot_dir() + .expect("SSOT") + .join("directory-move"); + std::fs::create_dir_all(&skill_source).expect("skill source"); + std::fs::write( + skill_source.join("SKILL.md"), + "---\nname: directory-move\ndescription: directory move\n---\n", + ) + .expect("skill manifest"); + db.save_skill(&crate::app_config::InstalledSkill { + id: "local:directory-move".to_string(), + name: "Directory move".to_string(), + description: Some("directory move".to_string()), + directory: "directory-move".to_string(), + repo_owner: None, + repo_name: None, + repo_branch: None, + readme_url: None, + apps: crate::app_config::SkillApps::only(&AppType::Pi), + installed_at: 1, + content_hash: None, + updated_at: 1, + }) + .expect("save skill"); + crate::services::skill_deployment::PiSkillDeploymentService::reconcile_all(&db) + .expect("deploy skill in old root"); + assert!(old_dir.join("skills").join("directory-move").exists()); db.save_prompt( AppType::Pi.as_str(), &crate::prompt::Prompt { @@ -4223,6 +4355,18 @@ mod tests { std::fs::read_to_string(new_dir.join("AGENTS.md")).expect("new AGENTS"), "new-root-agents" ); + assert!( + !old_dir.join("skills").join("directory-move").exists(), + "verified stale Skill ownership must be cleaned after new deployment" + ); + assert!(new_dir.join("skills").join("directory-move").exists()); + let deployments = db + .get_pi_skill_deployments("local:directory-move") + .expect("skill deployments"); + assert_eq!(deployments.len(), 1); + assert!(deployments[0] + .destination + .contains(new_dir.to_string_lossy().as_ref())); service .set_takeover_for_app("pi", false) @@ -4230,6 +4374,84 @@ mod tests { .expect("disable Pi takeover"); } + #[tokio::test] + #[serial] + async fn changing_pi_directory_rejects_an_unowned_managed_key_before_side_effects() { + let home = TempHome::new(); + crate::settings::reload_settings().expect("reload isolated settings"); + let old_dir = home.dir.path().join("old-pi"); + let new_dir = home.dir.path().join("new-pi"); + std::fs::create_dir_all(&old_dir).expect("old Pi directory"); + std::fs::create_dir_all(&new_dir).expect("new Pi directory"); + + let managed = json!({ + "name": "Managed Pi", + "api": "openai-responses", + "baseUrl": "https://managed.example/v1", + "apiKey": "managed-key", + "models": [{"id": "model-a", "name": "Model A"}] + }); + let foreign = json!({ + "api": "anthropic-messages", + "baseUrl": "https://foreign.example", + "apiKey": "foreign-secret", + "models": [{"id": "foreign-model"}] + }); + let foreign_document = serde_json::to_vec_pretty(&json!({ + "providers": {"managed-pi": foreign} + })) + .expect("foreign models"); + std::fs::write(new_dir.join("models.json"), &foreign_document).expect("seed foreign key"); + + let mut settings = crate::settings::get_settings(); + settings.pi_config_dir = Some(old_dir.to_string_lossy().into_owned()); + settings.pi_takeover_enabled = false; + crate::settings::update_settings(settings).expect("old Pi settings"); + let db = Arc::new(Database::memory().expect("database")); + db.create_pi_catalog_provider( + NewProviderAggregate::from_input( + AppType::Pi.as_str(), + ProviderMutationInput { + id: "managed-pi".to_string(), + name: "Managed Pi".to_string(), + settings_config: managed, + website_url: None, + category: None, + created_at: None, + sort_index: Some(0), + notes: None, + meta: None, + icon: Some("pi".to_string()), + icon_color: None, + in_failover_queue: false, + }, + ) + .expect("aggregate"), + "managed-pi", + ) + .expect("managed provider"); + + let service = ProxyService::new(db); + let switch_guard = service.lock_switch_for_app(AppType::Pi.as_str()).await; + let existing = crate::settings::get_settings(); + let mut next = existing.clone(); + next.pi_config_dir = Some(new_dir.to_string_lossy().into_owned()); + let error = service + .replace_settings_with_pi_directory_boundary_under_lock(&switch_guard, &existing, next) + .await + .expect_err("unowned target key must reject the directory move"); + assert!(error.to_string().contains("overwrite unowned provider key")); + assert_eq!( + crate::settings::get_settings().pi_config_dir, + existing.pi_config_dir, + "preflight rejection must not switch the native authority" + ); + assert_eq!( + std::fs::read(new_dir.join("models.json")).expect("foreign models remain"), + foreign_document + ); + } + #[tokio::test] #[serial] async fn changing_pi_directory_without_takeover_reconciles_missing_agents_truth() { @@ -4247,6 +4469,35 @@ mod tests { crate::settings::update_settings(settings).expect("old directory settings"); let db = Arc::new(Database::memory().expect("database")); + let direct = json!({ + "name": "Managed direct Pi", + "api": "openai-responses", + "baseUrl": "https://direct.example/v1", + "apiKey": "direct-key", + "models": [{"id": "direct-model", "name": "Direct model"}] + }); + db.create_pi_catalog_provider( + NewProviderAggregate::from_input( + AppType::Pi.as_str(), + ProviderMutationInput { + id: "managed-direct".to_string(), + name: "Managed direct Pi".to_string(), + settings_config: direct.clone(), + website_url: None, + category: None, + created_at: None, + sort_index: Some(0), + notes: None, + meta: None, + icon: Some("pi".to_string()), + icon_color: None, + in_failover_queue: false, + }, + ) + .expect("aggregate"), + "managed-direct", + ) + .expect("managed direct provider"); db.save_prompt( AppType::Pi.as_str(), &crate::prompt::Prompt { @@ -4282,6 +4533,15 @@ mod tests { "old-only" ); assert!(!new_dir.join("AGENTS.md").exists()); + let new_models: Value = serde_json::from_slice( + &std::fs::read(new_dir.join("models.json")).expect("new direct models"), + ) + .expect("parse new direct models"); + assert_eq!( + new_models.pointer("/providers/managed-direct"), + Some(&direct), + "a direct-mode directory change must publish every managed provider in the new root" + ); } #[tokio::test] diff --git a/src-tauri/src/services/skill.rs b/src-tauri/src/services/skill.rs index 960b1e576..fc507cd59 100644 --- a/src-tauri/src/services/skill.rs +++ b/src-tauri/src/services/skill.rs @@ -3012,6 +3012,35 @@ impl SkillService { Ok(()) } + /// Copy into a unique sibling and publish with an OS no-replace rename. + /// A concurrent installer can win, but its directory is never overwritten. + fn copy_dir_noreplace(src: &Path, dest: &Path) -> Result<()> { + let parent = dest + .parent() + .ok_or_else(|| anyhow!("Skill destination has no parent: {}", dest.display()))?; + fs::create_dir_all(parent)?; + let name = dest + .file_name() + .and_then(|value| value.to_str()) + .ok_or_else(|| anyhow!("Skill destination has an invalid name: {}", dest.display()))?; + let staged = parent.join(format!( + ".{name}.cc-switch-install-{}", + uuid::Uuid::new_v4().simple() + )); + if let Err(error) = Self::copy_dir_recursive(src, &staged) { + let _ = fs::remove_dir_all(&staged); + return Err(error); + } + if let Err(error) = crate::pi_config::shared_file::publish_path_noreplace(&staged, dest) { + let _ = fs::remove_dir_all(&staged); + return Err(anyhow!( + "Skill destination was created concurrently ({}): {error}", + dest.display() + )); + } + Ok(()) + } + fn resolve_uninstall_backup_source(skill: &InstalledSkill) -> Result> { // 返回值会被整目录复制进 ~/.cc-switch/skill-backups/ 并由 get_skill_backups // 在界面上列出——脏 directory 在这里等于任意文件读取 + 外泄通道。 @@ -3382,6 +3411,16 @@ impl SkillService { let deployment_guard = matches!(current_app, AppType::Pi) .then(crate::services::skill_deployment::PiSkillDeploymentService::operation_guard); + let pi_source_digest = if deployment_guard.is_some() { + Some( + crate::services::skill_deployment::PiSkillDeploymentService::source_digest( + &skill_dir, + ) + .map_err(|error| anyhow!(error.to_string()))?, + ) + } else { + None + }; // 复制到 SSOT let dest = ssot_dir.join(&install_name); @@ -3392,7 +3431,7 @@ impl SkillService { Some("uninstallFirst"), ))); } - Self::copy_dir_recursive(&skill_dir, &dest)?; + Self::copy_dir_noreplace(&skill_dir, &dest)?; // 计算内容哈希 let content_hash = Self::compute_dir_hash(&dest).ok(); @@ -3418,28 +3457,56 @@ impl SkillService { // 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()); + let cleanup = + crate::services::skill_deployment::PiSkillDeploymentService::remove_source_if_unchanged( + &dest, + pi_source_digest + .as_deref() + .expect("Pi ZIP publication has a source digest"), + ); + return match cleanup { + Ok(()) => Err(error.into()), + Err(cleanup) => Err(anyhow!( + "Pi Skill ZIP database write failed ({error}); SSOT rollback failed ({cleanup})" + )), + }; } if let Err(error) = crate::services::skill_deployment::PiSkillDeploymentService::toggle_under_guard( guard, db, &mut skill, true, ) { + if let Err(deployment_rollback) = + crate::services::skill_deployment::PiSkillDeploymentService::remove_before_uninstall_under_guard( + guard, db, &skill, + ) + { + return Err(anyhow!( + "Pi Skill ZIP install failed ({error}); native ownership rollback failed ({deployment_rollback}); the DB row and SSOT were retained as recovery evidence" + )); + } 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(), + if !matches!(&db_rollback, Ok(true)) { + return Err(anyhow!( + "Pi Skill ZIP install failed ({error}); DB rollback failed ({}); SSOT was retained", + match db_rollback { Ok(false) => "row missing".to_string(), Err(value) => value.to_string(), - }, - file_result - .err() - .map_or_else(|| "ok".to_string(), |value| value.to_string()) + Ok(true) => unreachable!(), + } + )); + } + let file_rollback = + crate::services::skill_deployment::PiSkillDeploymentService::remove_source_if_unchanged( + &dest, + pi_source_digest + .as_deref() + .expect("Pi ZIP publication has a source digest"), + ); + return match file_rollback { + Ok(()) => Err(anyhow!(error.to_string())), + Err(file_error) => Err(anyhow!( + "Pi Skill ZIP install failed ({error}); SSOT rollback failed ({file_error})" )), }; } @@ -4359,6 +4426,34 @@ mod tests { ); } + #[test] + fn no_replace_directory_publish_preserves_an_existing_destination() { + let temp = tempdir().expect("tempdir"); + let source = temp.path().join("source"); + let destination = temp.path().join("destination"); + fs::create_dir(&source).expect("source"); + fs::create_dir(&destination).expect("destination"); + fs::write(source.join("SKILL.md"), "managed").expect("source manifest"); + fs::write(destination.join("SKILL.md"), "external").expect("external manifest"); + + SkillService::copy_dir_noreplace(&source, &destination) + .expect_err("an existing destination must win atomically"); + assert_eq!( + fs::read_to_string(destination.join("SKILL.md")).expect("external destination"), + "external" + ); + assert!( + fs::read_dir(temp.path()) + .expect("temp root") + .all(|entry| !entry + .expect("entry") + .file_name() + .to_string_lossy() + .contains("cc-switch-install")), + "a rejected staged publication must be cleaned" + ); + } + #[test] fn extract_local_zip_hands_back_a_guard_that_owns_the_tree() { use std::io::Write; diff --git a/src-tauri/src/services/skill_deployment.rs b/src-tauri/src/services/skill_deployment.rs index b2622d737..a80b3b17c 100644 --- a/src-tauri/src/services/skill_deployment.rs +++ b/src-tauri/src/services/skill_deployment.rs @@ -70,10 +70,10 @@ impl PiSkillDeploymentService { skill: &mut InstalledSkill, enabled: bool, ) -> Result<(), AppError> { - let destination = skill_destination(skill)?; - let destination_key = destination_key(&destination); - let existing = db.get_pi_skill_deployment(&skill.id, &destination_key)?; if enabled { + let destination = skill_destination(skill)?; + let destination_key = destination_key(&destination); + let existing = db.get_pi_skill_deployment(&skill.id, &destination_key)?; let source = skill_source(skill)?; deploy( db, @@ -84,15 +84,9 @@ impl PiSkillDeploymentService { existing, Some(true), )?; + cleanup_stale_deployments(db, skill, &destination_key)?; } else { - remove_owned( - db, - skill, - &destination, - &destination_key, - existing, - Some(false), - )?; + remove_all_recorded_deployments(db, skill, Some(false))?; } skill.apps.pi = enabled; Ok(()) @@ -130,15 +124,9 @@ impl PiSkillDeploymentService { Some(true), )?; } + cleanup_stale_deployments(db, skill, &destination_key)?; } else { - remove_owned( - db, - skill, - &destination, - &destination_key, - existing, - Some(false), - )?; + remove_all_recorded_deployments(db, skill, Some(false))?; } skill.apps.pi = enabled; Ok(()) @@ -188,10 +176,7 @@ impl PiSkillDeploymentService { db: &Arc, skill: &InstalledSkill, ) -> Result<(), AppError> { - let destination = skill_destination(skill)?; - let key = destination_key(&destination); - let existing = db.get_pi_skill_deployment(&skill.id, &key)?; - remove_owned(db, skill, &destination, &key, existing, None) + remove_all_recorded_deployments(db, skill, None) } pub(crate) fn inspect_all( @@ -206,6 +191,35 @@ impl PiSkillDeploymentService { }) .collect() } + + pub(crate) fn source_digest(path: &Path) -> Result { + tree_digest(path) + } + + /// Remove a just-published SSOT tree only if the exact bytes installed by + /// this operation still own the path. The namespace move precedes digest + /// validation so an external replacement is restored, never recursively + /// deleted after a check/use race. + pub(crate) fn remove_source_if_unchanged( + path: &Path, + expected_digest: &str, + ) -> Result<(), AppError> { + let staged = stage_destination(path)?; + let observed = tree_digest(&staged); + if !matches!(observed, Ok(ref digest) if digest == expected_digest) { + restore_staged_destination(&staged, path).map_err(|rollback| { + AppError::Conflict(format!( + "Pi Skill SSOT ownership changed and rollback failed ({rollback}); recovery tree: {}", + staged.display() + )) + })?; + return Err(AppError::Conflict(format!( + "Pi Skill SSOT changed before rollback: {}", + path.display() + ))); + } + remove_path(&staged) + } } #[derive(Debug)] @@ -323,18 +337,27 @@ fn inspect_skill_status( ) -> Result { let destination = skill_destination(skill)?; let destination_key = destination_key(&destination); - let deployment = db.get_pi_skill_deployment(&skill.id, &destination_key)?; + let deployments = db.get_pi_skill_deployments(&skill.id)?; + let deployment = deployments + .iter() + .find(|deployment| deployment.destination_key == destination_key); + let has_stale_destination = deployments + .iter() + .any(|deployment| deployment.destination_key != destination_key); let manifest = destination.join("SKILL.md"); let discovered = discovery.by_manifest.get(&manifest); let destination_exists = fs::symlink_metadata(&destination).is_ok(); let owned_deployment = deployment - .as_ref() .is_some_and(|deployment| verify_owned_destination(deployment, &destination).is_ok()); - let ownership = match (deployment.is_some(), destination_exists, owned_deployment) { - (_, _, true) => PiSkillOwnership::Owned, - (true, _, false) => PiSkillOwnership::Stale, - (false, true, false) => PiSkillOwnership::Foreign, - (false, false, false) => PiSkillOwnership::Absent, + let ownership = if has_stale_destination { + PiSkillOwnership::Stale + } else { + match (deployment.is_some(), destination_exists, owned_deployment) { + (_, _, true) => PiSkillOwnership::Owned, + (true, _, false) => PiSkillOwnership::Stale, + (false, true, false) => PiSkillOwnership::Foreign, + (false, false, false) => PiSkillOwnership::Absent, + } }; let (discovery_status, discovery_issue) = discovered.cloned().unwrap_or_else(|| { ( @@ -343,14 +366,18 @@ fn inspect_skill_status( ) }); let effectively_discovered = discovery_status == PiSkillDiscovery::Active; - let issue = match ownership { - PiSkillOwnership::Stale => { - Some("recorded Pi deployment no longer matches the live filesystem".to_string()) + let issue = if has_stale_destination { + Some("recorded Pi deployment remains at a previous agent root".to_string()) + } else { + match ownership { + PiSkillOwnership::Stale => { + Some("recorded Pi deployment no longer matches the live filesystem".to_string()) + } + PiSkillOwnership::Foreign if skill.apps.pi => { + Some("desired Pi Skill collides with an unowned live destination".to_string()) + } + _ => discovery_issue, } - PiSkillOwnership::Foreign if skill.apps.pi => { - Some("desired Pi Skill collides with an unowned live destination".to_string()) - } - _ => discovery_issue, }; Ok(SkillAppStatus { desired_enabled: skill.apps.pi, @@ -370,10 +397,10 @@ fn deployment_lock() -> std::sync::MutexGuard<'static, ()> { } fn reconcile_skill_unlocked(db: &Arc, skill: &InstalledSkill) -> Result<(), AppError> { - let destination = skill_destination(skill)?; - let destination_key = destination_key(&destination); - let existing = db.get_pi_skill_deployment(&skill.id, &destination_key)?; if skill.apps.pi { + let destination = skill_destination(skill)?; + let destination_key = destination_key(&destination); + let existing = db.get_pi_skill_deployment(&skill.id, &destination_key)?; let source = skill_source(skill)?; deploy( db, @@ -383,9 +410,64 @@ fn reconcile_skill_unlocked(db: &Arc, skill: &InstalledSkill) -> Resul &destination_key, existing, None, - ) + )?; + cleanup_stale_deployments(db, skill, &destination_key) } else { - remove_owned(db, skill, &destination, &destination_key, existing, None) + remove_all_recorded_deployments(db, skill, None) + } +} + +fn cleanup_stale_deployments( + db: &Arc, + skill: &InstalledSkill, + current_destination_key: &str, +) -> Result<(), AppError> { + for deployment in db.get_pi_skill_deployments(&skill.id)? { + if deployment.destination_key != current_destination_key { + remove_recorded_deployment(db, skill, deployment)?; + } + } + Ok(()) +} + +fn remove_all_recorded_deployments( + db: &Arc, + skill: &InstalledSkill, + desired_enabled: Option, +) -> Result<(), AppError> { + if let Some(desired_enabled) = desired_enabled { + // Desired state is one row-level authority, independent of how many + // old agent roots still have device-local ownership evidence. + db.set_pi_skill_desired(&skill.id, desired_enabled)?; + } + for deployment in db.get_pi_skill_deployments(&skill.id)? { + remove_recorded_deployment(db, skill, deployment)?; + } + Ok(()) +} + +fn remove_recorded_deployment( + db: &Arc, + skill: &InstalledSkill, + deployment: SkillDeployment, +) -> Result<(), AppError> { + let destination = PathBuf::from(&deployment.destination); + if destination_key(&destination) != deployment.destination_key { + return Err(AppError::Conflict(format!( + "Pi Skill '{}' has inconsistent recorded destination identity", + skill.id + ))); + } + match fs::symlink_metadata(&destination) { + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + db.delete_pi_skill_deployment(&skill.id, &deployment.destination_key)?; + Ok(()) + } + Err(error) => Err(AppError::io(&destination, error)), + Ok(_) => { + let key = deployment.destination_key.clone(); + remove_owned(db, skill, &destination, &key, Some(deployment), None) + } } } @@ -1076,4 +1158,84 @@ mod tests { .as_deref() .is_some_and(|issue| issue.contains("description"))); } + + #[test] + #[serial_test::serial] + fn agent_root_relocation_deploys_new_destination_before_cleaning_old_ownership() { + 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("relocated"); + fs::create_dir_all(&source).expect("source"); + fs::write( + source.join("SKILL.md"), + "---\nname: relocated\ndescription: relocation test\n---\n", + ) + .expect("manifest"); + let skill = InstalledSkill { + id: "local:relocated".to_string(), + name: "Relocated".to_string(), + description: Some("relocation test".to_string()), + directory: "relocated".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("relocated"); + assert!(fs::symlink_metadata(&old_destination).is_ok()); + + std::env::set_var("PI_CODING_AGENT_DIR", &new_root); + PiSkillDeploymentService::reconcile_skill(&db, &skill).expect("relocate deployment"); + let new_destination = new_root.join("skills").join("relocated"); + assert!(fs::symlink_metadata(&new_destination).is_ok()); + assert!( + fs::symlink_metadata(&old_destination).is_err(), + "the verified old deployment must be cleaned only after new publication" + ); + let deployments = db + .get_pi_skill_deployments(&skill.id) + .expect("deployment ledger"); + assert_eq!(deployments.len(), 1); + assert_eq!( + deployments[0].destination_key, + destination_key(&new_destination) + ); + } }