From 03061f64955ac656519aa5e477e5de946c7ba235 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:29:07 -0700 Subject: [PATCH 01/18] test(plugin): cover atomic replacement rollback --- .../crates/cmux-tui/src/plugin_manager.rs | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 064505a21d80..f76b07a80b15 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -768,6 +768,105 @@ mod tests { fs::remove_dir_all(root).unwrap(); } + struct FailingRenameFilesystem { + fail_at: usize, + calls: Cell, + } + + impl InstallFilesystem for FailingRenameFilesystem { + fn rename(&self, from: &Path, to: &Path) -> std::io::Result<()> { + let call = self.calls.get() + 1; + self.calls.set(call); + if call == self.fail_at { + return Err(std::io::Error::other("injected rename failure")); + } + fs::rename(from, to) + } + + fn remove_dir_all(&self, path: &Path) -> std::io::Result<()> { + fs::remove_dir_all(path) + } + + fn remove_file(&self, path: &Path) -> std::io::Result<()> { + fs::remove_file(path) + } + } + + fn replacement_fixture(label: &str) -> (PathBuf, PathBuf, PathBuf, PathBuf) { + let root = std::env::temp_dir().join(format!( + "cmux-plugin-replacement-test-{label}-{}-{}", + std::process::id(), + now_nanos() + )); + let target = root.join("demo"); + let registry = root.join(".registry"); + let metadata_path = registry.join("demo.json"); + let temp_dir = root.join(".install"); + let metadata_temp = registry.join(".demo.tmp"); + fs::create_dir_all(target.join("bin")).unwrap(); + fs::write(target.join("marker"), "old").unwrap(); + fs::create_dir_all(®istry).unwrap(); + fs::write( + &metadata_path, + r#"{"id":"sidebar_plugin_11111111111111111111111111111111"} +"#, + ) + .unwrap(); + fs::create_dir_all(temp_dir.join("bin")).unwrap(); + fs::write(temp_dir.join("marker"), "new").unwrap(); + fs::write( + &metadata_temp, + r#"{"id":"sidebar_plugin_22222222222222222222222222222222"} +"#, + ) + .unwrap(); + (root, target, temp_dir, metadata_temp) + } + + #[test] + fn replacement_failure_after_backup_restores_plugin_and_metadata() { + let (root, target, temp_dir, metadata_temp) = replacement_fixture("rollback"); + let filesystem = FailingRenameFilesystem { fail_at: 2, calls: Cell::new(0) }; + let error = replace_installed_plugin_with_fs( + &filesystem, + &root, + "demo", + &temp_dir, + &metadata_temp, + true, + ) + .unwrap_err(); + assert!(error.to_string().contains("back up")); + assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "old"); + assert_eq!( + read_registry_metadata(&root, "demo").unwrap().id, + "sidebar_plugin_11111111111111111111111111111111" + ); + assert!(temp_dir.exists()); + assert!(metadata_temp.exists()); + fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn replacement_commits_new_plugin_and_metadata_together() { + let (root, target, temp_dir, metadata_temp) = replacement_fixture("success"); + replace_installed_plugin(&root, "demo", &temp_dir, &metadata_temp, true).unwrap(); + assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "new"); + assert_eq!( + read_registry_metadata(&root, "demo").unwrap().id, + "sidebar_plugin_22222222222222222222222222222222" + ); + assert!(!temp_dir.exists()); + assert!(!metadata_temp.exists()); + let visible_entries = fs::read_dir(&root) + .unwrap() + .filter_map(Result::ok) + .filter(|entry| !entry.file_name().to_string_lossy().starts_with('.')) + .count(); + assert_eq!(visible_entries, 1); + fs::remove_dir_all(root).unwrap(); + } + #[test] fn plugin_snapshot_matches_the_closed_catalog_shape() { let root = std::env::temp_dir().join(format!( From c778b568e1f7117989f4de618ba4d119dab0e9c4 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:29:10 -0700 Subject: [PATCH 02/18] fix(plugin): replace installs with rollback-safe transaction --- .../crates/cmux-tui/src/plugin_manager.rs | 170 +++++++++++++++--- 1 file changed, 150 insertions(+), 20 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index f76b07a80b15..f96ec6a9d820 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -157,6 +157,7 @@ fn install_command(positionals: &[String], options: &CliOptions) -> Result Result { let manifest = read_manifest(&temp_dir) .map_err(|error| ManagerError::validation(None, error.to_string()))?; @@ -176,12 +177,10 @@ fn install_command(positionals: &[String], options: &CliOptions) -> Result Result PathBuf { install_root.join(".registry").join(format!("{name}.json")) } -fn replace_registry_metadata( +trait InstallFilesystem { + fn rename(&self, from: &Path, to: &Path) -> std::io::Result<()>; + fn remove_dir_all(&self, path: &Path) -> std::io::Result<()>; + fn remove_file(&self, path: &Path) -> std::io::Result<()>; +} + +struct StandardInstallFilesystem; + +impl InstallFilesystem for StandardInstallFilesystem { + fn rename(&self, from: &Path, to: &Path) -> std::io::Result<()> { + fs::rename(from, to) + } + + fn remove_dir_all(&self, path: &Path) -> std::io::Result<()> { + fs::remove_dir_all(path) + } + + fn remove_file(&self, path: &Path) -> std::io::Result<()> { + fs::remove_file(path) + } +} + +fn unique_backup_path(parent: &Path, name: &str, suffix: &str) -> PathBuf { + loop { + let path = parent.join(format!(".{name}.{}-{}{suffix}", std::process::id(), now_nanos())); + if !path.exists() { + return path; + } + } +} + +fn replace_installed_plugin( install_root: &Path, name: &str, - metadata: &PluginRegistryMetadata, + temp_dir: &Path, + metadata_temp: &Path, +) -> anyhow::Result<()> { + replace_installed_plugin_with_fs( + &StandardInstallFilesystem, + install_root, + name, + temp_dir, + metadata_temp, + ) +} + +fn replace_installed_plugin_with_fs( + filesystem: &F, + install_root: &Path, + name: &str, + temp_dir: &Path, + metadata_temp: &Path, ) -> anyhow::Result<()> { + let target = install_root.join(name); + let target_exists = target.exists(); + let metadata_path = registry_metadata_path(install_root, name); + let target_backup = unique_backup_path(install_root, name, ".plugin-backup"); + let metadata_backup = + unique_backup_path(&install_root.join(".registry"), name, ".metadata-backup.json"); + let metadata_exists = metadata_path.exists(); + let mut target_backed_up = false; + let mut metadata_backed_up = false; + let mut target_installed = false; + let mut metadata_installed = false; + + let result = (|| -> anyhow::Result<()> { + if target_exists { + filesystem.rename(&target, &target_backup).map_err(|error| { + anyhow::anyhow!("failed to back up {}: {error}", target.display()) + })?; + target_backed_up = true; + } + if metadata_exists { + filesystem.rename(&metadata_path, &metadata_backup).map_err(|error| { + anyhow::anyhow!("failed to back up {}: {error}", metadata_path.display()) + })?; + metadata_backed_up = true; + } + filesystem + .rename(temp_dir, &target) + .map_err(|error| anyhow::anyhow!("failed to install {}: {error}", target.display()))?; + target_installed = true; + filesystem.rename(metadata_temp, &metadata_path).map_err(|error| { + anyhow::anyhow!("failed to persist {}: {error}", metadata_path.display()) + })?; + metadata_installed = true; + Ok(()) + })(); + + if let Err(error) = result { + let mut rollback_errors = Vec::new(); + if metadata_installed + && let Err(rollback_error) = filesystem.rename(&metadata_path, metadata_temp) + { + rollback_errors.push(format!("metadata staging: {rollback_error}")); + } + if metadata_backed_up + && let Err(rollback_error) = filesystem.rename(&metadata_backup, &metadata_path) + { + rollback_errors.push(format!("metadata restore: {rollback_error}")); + } + if target_installed && let Err(rollback_error) = filesystem.rename(&target, temp_dir) { + rollback_errors.push(format!("plugin staging: {rollback_error}")); + } + if target_backed_up && let Err(rollback_error) = filesystem.rename(&target_backup, &target) + { + rollback_errors.push(format!("plugin restore: {rollback_error}")); + } + if !rollback_errors.is_empty() { + anyhow::bail!("{error}; rollback failed: {}", rollback_errors.join("; ")); + } + return Err(error); + } + + // The replacement is committed once both new paths are in place. Cleanup is + // deliberately best effort: retaining an old same-parent backup is safe and + // preserves recovery data if the process loses access after commit. + let _ = filesystem.remove_dir_all(&target_backup); + let _ = filesystem.remove_file(&metadata_backup); + Ok(()) +} + +fn write_registry_metadata_temp( + install_root: &Path, + name: &str, + metadata: &PluginRegistryMetadata, +) -> anyhow::Result { validate_plugin_name(name)?; validate_plugin_id(&metadata.id)?; let registry = install_root.join(".registry"); fs::create_dir_all(®istry)?; - let path = registry_metadata_path(install_root, name); let temp = registry.join(format!(".{name}.{}-{}.tmp", std::process::id(), now_nanos())); let encoded = serde_json::to_vec(metadata)?; let mut file = fs::OpenOptions::new().create_new(true).write(true).open(&temp)?; @@ -602,6 +727,16 @@ fn replace_registry_metadata( file.write_all(b"\n")?; file.sync_all()?; drop(file); + Ok(temp) +} + +fn replace_registry_metadata( + install_root: &Path, + name: &str, + metadata: &PluginRegistryMetadata, +) -> anyhow::Result<()> { + let path = registry_metadata_path(install_root, name); + let temp = write_registry_metadata_temp(install_root, name, metadata)?; if let Err(error) = fs::rename(&temp, &path) { let _ = fs::remove_file(&temp); return Err(anyhow::anyhow!("failed to persist {}: {error}", path.display())); @@ -682,6 +817,7 @@ fn now_nanos() -> u128 { #[cfg(test)] mod tests { use super::*; + use std::cell::Cell; fn manifest_text(name: &str) -> String { format!( @@ -827,15 +963,9 @@ mod tests { fn replacement_failure_after_backup_restores_plugin_and_metadata() { let (root, target, temp_dir, metadata_temp) = replacement_fixture("rollback"); let filesystem = FailingRenameFilesystem { fail_at: 2, calls: Cell::new(0) }; - let error = replace_installed_plugin_with_fs( - &filesystem, - &root, - "demo", - &temp_dir, - &metadata_temp, - true, - ) - .unwrap_err(); + let error = + replace_installed_plugin_with_fs(&filesystem, &root, "demo", &temp_dir, &metadata_temp) + .unwrap_err(); assert!(error.to_string().contains("back up")); assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "old"); assert_eq!( @@ -850,7 +980,7 @@ mod tests { #[test] fn replacement_commits_new_plugin_and_metadata_together() { let (root, target, temp_dir, metadata_temp) = replacement_fixture("success"); - replace_installed_plugin(&root, "demo", &temp_dir, &metadata_temp, true).unwrap(); + replace_installed_plugin(&root, "demo", &temp_dir, &metadata_temp).unwrap(); assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "new"); assert_eq!( read_registry_metadata(&root, "demo").unwrap().id, From 8b85dccad9a0229b3c2e3d3c2674aa9652ff9f0a Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:42:46 -0700 Subject: [PATCH 03/18] fix(plugin): recover interrupted install transactions --- .../crates/cmux-tui/src/plugin_manager.rs | 452 ++++++++++++++++-- 1 file changed, 422 insertions(+), 30 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index f96ec6a9d820..41fa378212b4 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -4,6 +4,7 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{SystemTime, UNIX_EPOCH}; +use fs4::FileExt; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; @@ -125,7 +126,45 @@ struct PluginRegistryMetadata { id: String, } +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +enum InstallJournalPhase { + Prepared, + Committed, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct InstallJournal { + name: String, + target_backup: PathBuf, + metadata_backup: PathBuf, + temp_dir: PathBuf, + metadata_temp: PathBuf, + target_existed: bool, + metadata_existed: bool, + config_snapshot: Option, + phase: InstallJournalPhase, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct ConfigSnapshot { + path: PathBuf, + sidebar_plugin: Option, +} + +struct PluginOperationLock(fs::File); + +fn acquire_plugin_operation_lock() -> anyhow::Result { + let root = install_root()?; + fs::create_dir_all(&root)?; + let path = root.join(".install.lock"); + let file = fs::OpenOptions::new().create(true).read(true).write(true).open(path)?; + file.lock()?; + Ok(PluginOperationLock(file)) +} + pub(crate) fn execute(positionals: &[String], options: CliOptions) -> Result { + let _lock = acquire_plugin_operation_lock().map_err(ManagerError::Failure)?; match positionals.first().map(String::as_str) { Some("install") => install_command(positionals, &options), Some("list") => list_command(positionals, &options), @@ -149,6 +188,7 @@ fn install_command(positionals: &[String], options: &CliOptions) -> Result Result anyhow::Result> { let root = install_root()?; + reconcile_install_transactions(&root)?; let selected = selected_plugin_cwd()?; let mut plugins = Vec::new(); let entries = match fs::read_dir(&root) { @@ -620,6 +671,167 @@ fn unique_backup_path(parent: &Path, name: &str, suffix: &str) -> PathBuf { } } +fn install_journal_path(install_root: &Path, name: &str) -> PathBuf { + install_root.join(format!(".{name}.install-journal.json")) +} + +fn write_install_journal(path: &Path, journal: &InstallJournal) -> anyhow::Result<()> { + let parent = path.parent().unwrap_or_else(|| Path::new(".")); + let temp = parent.join(format!( + ".{}.{}-journal.tmp", + path.file_name().unwrap().to_string_lossy(), + now_nanos() + )); + let encoded = serde_json::to_vec(journal)?; + let result = (|| -> anyhow::Result<()> { + let mut file = fs::OpenOptions::new().create_new(true).write(true).open(&temp)?; + file.write_all(&encoded)?; + file.write_all(b"\n")?; + file.sync_all()?; + drop(file); + fs::rename(&temp, path)?; + sync_directory(parent)?; + Ok(()) + })(); + if result.is_err() { + let _ = fs::remove_file(&temp); + } + result +} + +#[cfg(unix)] +fn sync_directory(path: &Path) -> anyhow::Result<()> { + fs::File::open(path)?.sync_all()?; + Ok(()) +} + +#[cfg(not(unix))] +fn sync_directory(_path: &Path) -> anyhow::Result<()> { + Ok(()) +} + +fn remove_path_if_present(path: &Path) -> anyhow::Result<()> { + let metadata = match fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error.into()), + }; + if metadata.is_dir() { + fs::remove_dir_all(path)?; + } else { + fs::remove_file(path)?; + } + Ok(()) +} + +fn reconcile_install_transactions(install_root: &Path) -> anyhow::Result<()> { + let entries = match fs::read_dir(install_root) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error.into()), + }; + for entry in entries { + let entry = entry?; + let path = entry.path(); + let Some(name) = path.file_name().and_then(|value| value.to_str()) else { continue }; + if !name.starts_with('.') || !name.ends_with(".install-journal.json") { + continue; + } + let journal: InstallJournal = + serde_json::from_slice(&fs::read(&path)?).map_err(|error| { + anyhow::anyhow!("invalid install journal {}: {error}", path.display()) + })?; + match journal.phase { + InstallJournalPhase::Committed => { + remove_path_if_present(&journal.target_backup)?; + remove_path_if_present(&journal.metadata_backup)?; + remove_path_if_present(&journal.temp_dir)?; + remove_path_if_present(&journal.metadata_temp)?; + } + InstallJournalPhase::Prepared => { + if let Some(snapshot) = &journal.config_snapshot { + restore_config_snapshot(snapshot)?; + } + if journal.target_existed { + if journal.target_backup.exists() { + remove_path_if_present(&install_root.join(&journal.name))?; + fs::rename(&journal.target_backup, install_root.join(&journal.name))?; + } + } else { + remove_path_if_present(&install_root.join(&journal.name))?; + } + if journal.metadata_existed { + let metadata_path = registry_metadata_path(install_root, &journal.name); + if journal.metadata_backup.exists() { + remove_path_if_present(&metadata_path)?; + fs::rename(&journal.metadata_backup, metadata_path)?; + } + } else { + remove_path_if_present(®istry_metadata_path(install_root, &journal.name))?; + } + remove_path_if_present(&journal.temp_dir)?; + remove_path_if_present(&journal.metadata_temp)?; + } + } + remove_path_if_present(&path)?; + } + Ok(()) +} + +fn restore_config_snapshot(snapshot: &ConfigSnapshot) -> anyhow::Result<()> { + let mut root = match fs::read_to_string(&snapshot.path) { + Ok(text) if text.trim().is_empty() => json!({}), + Ok(text) => serde_json::from_str(&text)?, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => json!({}), + Err(error) => return Err(error.into()), + }; + let Some(root_object) = root.as_object_mut() else { + anyhow::bail!("{} must contain a JSON object", snapshot.path.display()); + }; + match &snapshot.sidebar_plugin { + Some(plugin) => { + let sidebar = root_object.entry("sidebar").or_insert_with(|| json!({})); + if !sidebar.is_object() { + *sidebar = json!({}); + } + sidebar + .as_object_mut() + .expect("sidebar was just made an object") + .insert("plugin".to_string(), plugin.clone()); + } + None => { + if let Some(sidebar) = root_object.get_mut("sidebar") + && let Some(sidebar_object) = sidebar.as_object_mut() + { + sidebar_object.remove("plugin"); + } + } + } + write_config_value_atomic_local(&snapshot.path, &root) +} + +fn write_config_value_atomic_local(path: &Path, value: &Value) -> anyhow::Result<()> { + let parent = path.parent().unwrap_or_else(|| Path::new(".")); + fs::create_dir_all(parent)?; + let file_name = path.file_name().and_then(|name| name.to_str()).unwrap_or("cmux-tui.json"); + let temp = + parent.join(format!(".{file_name}.{}.{}-restore.tmp", std::process::id(), now_nanos())); + let result = (|| -> anyhow::Result<()> { + let mut file = fs::OpenOptions::new().create_new(true).write(true).open(&temp)?; + serde_json::to_writer_pretty(&mut file, value)?; + file.write_all(b"\n")?; + file.sync_all()?; + drop(file); + fs::rename(&temp, path)?; + sync_directory(parent)?; + Ok(()) + })(); + if result.is_err() { + let _ = fs::remove_file(&temp); + } + result +} + fn replace_installed_plugin( install_root: &Path, name: &str, @@ -632,15 +844,52 @@ fn replace_installed_plugin( name, temp_dir, metadata_temp, + None, + || Ok(()), ) } -fn replace_installed_plugin_with_fs( +fn replace_installed_plugin_with_config anyhow::Result<()>>( + install_root: &Path, + name: &str, + temp_dir: &Path, + metadata_temp: &Path, + config_snapshot: Option, + after_install: C, +) -> anyhow::Result<()> { + replace_installed_plugin_with_fs( + &StandardInstallFilesystem, + install_root, + name, + temp_dir, + metadata_temp, + config_snapshot, + after_install, + ) +} + +fn capture_config_snapshot() -> anyhow::Result { + let path = config::config_path()?; + let sidebar_plugin = match fs::read_to_string(&path) { + Ok(text) if text.trim().is_empty() => None, + Ok(text) => serde_json::from_str::(&text)? + .get("sidebar") + .and_then(|sidebar| sidebar.get("plugin")) + .cloned(), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + Err(error) => return Err(error.into()), + }; + Ok(ConfigSnapshot { path, sidebar_plugin }) +} + +fn replace_installed_plugin_with_fs anyhow::Result<()>>( filesystem: &F, install_root: &Path, name: &str, temp_dir: &Path, metadata_temp: &Path, + config_snapshot: Option, + after_install: C, ) -> anyhow::Result<()> { let target = install_root.join(name); let target_exists = target.exists(); @@ -649,6 +898,19 @@ fn replace_installed_plugin_with_fs( let metadata_backup = unique_backup_path(&install_root.join(".registry"), name, ".metadata-backup.json"); let metadata_exists = metadata_path.exists(); + let journal_path = install_journal_path(install_root, name); + let journal = InstallJournal { + name: name.to_string(), + target_backup: target_backup.clone(), + metadata_backup: metadata_backup.clone(), + temp_dir: temp_dir.to_path_buf(), + metadata_temp: metadata_temp.to_path_buf(), + target_existed: target_exists, + metadata_existed: metadata_exists, + config_snapshot, + phase: InstallJournalPhase::Prepared, + }; + write_install_journal(&journal_path, &journal)?; let mut target_backed_up = false; let mut metadata_backed_up = false; let mut target_installed = false; @@ -675,27 +937,57 @@ fn replace_installed_plugin_with_fs( anyhow::anyhow!("failed to persist {}: {error}", metadata_path.display()) })?; metadata_installed = true; + after_install()?; + write_install_journal( + &journal_path, + &InstallJournal { phase: InstallJournalPhase::Committed, ..journal.clone() }, + )?; Ok(()) })(); if let Err(error) = result { let mut rollback_errors = Vec::new(); - if metadata_installed - && let Err(rollback_error) = filesystem.rename(&metadata_path, metadata_temp) - { - rollback_errors.push(format!("metadata staging: {rollback_error}")); + if metadata_installed { + if let Err(rollback_error) = filesystem.rename(&metadata_path, metadata_temp) { + rollback_errors.push(format!("metadata staging: {rollback_error}")); + match filesystem.remove_file(&metadata_path) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => rollback_errors.push(format!("metadata removal: {error}")), + } + } } - if metadata_backed_up - && let Err(rollback_error) = filesystem.rename(&metadata_backup, &metadata_path) - { - rollback_errors.push(format!("metadata restore: {rollback_error}")); + if metadata_backed_up { + if let Err(rollback_error) = filesystem.rename(&metadata_backup, &metadata_path) { + rollback_errors.push(format!("metadata restore: {rollback_error}")); + } } - if target_installed && let Err(rollback_error) = filesystem.rename(&target, temp_dir) { - rollback_errors.push(format!("plugin staging: {rollback_error}")); + if target_installed { + if let Err(rollback_error) = filesystem.rename(&target, temp_dir) { + rollback_errors.push(format!("plugin staging: {rollback_error}")); + match filesystem.remove_dir_all(&target) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => rollback_errors.push(format!("plugin removal: {error}")), + } + } + } + if target_backed_up { + if let Err(rollback_error) = filesystem.rename(&target_backup, &target) { + rollback_errors.push(format!("plugin restore: {rollback_error}")); + } } - if target_backed_up && let Err(rollback_error) = filesystem.rename(&target_backup, &target) + if let Some(snapshot) = &journal.config_snapshot + && let Err(rollback_error) = restore_config_snapshot(snapshot) { - rollback_errors.push(format!("plugin restore: {rollback_error}")); + rollback_errors.push(format!("config restore: {rollback_error}")); + } + if rollback_errors.is_empty() { + if let Err(rollback_error) = filesystem.remove_file(&journal_path) + && rollback_error.kind() != std::io::ErrorKind::NotFound + { + rollback_errors.push(format!("journal cleanup: {rollback_error}")); + } } if !rollback_errors.is_empty() { anyhow::bail!("{error}; rollback failed: {}", rollback_errors.join("; ")); @@ -703,11 +995,23 @@ fn replace_installed_plugin_with_fs( return Err(error); } - // The replacement is committed once both new paths are in place. Cleanup is - // deliberately best effort: retaining an old same-parent backup is safe and - // preserves recovery data if the process loses access after commit. - let _ = filesystem.remove_dir_all(&target_backup); - let _ = filesystem.remove_file(&metadata_backup); + // The replacement is committed once both new paths are in place. Keep the + // committed journal until all cleanup succeeds, so a later invocation can + // retry cleanup if access is temporarily unavailable. + let backup_dir_clean = match filesystem.remove_dir_all(&target_backup) { + Ok(()) => true, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => true, + Err(_) => false, + }; + let metadata_backup_clean = match filesystem.remove_file(&metadata_backup) { + Ok(()) => true, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => true, + Err(_) => false, + }; + let backups_clean = backup_dir_clean && metadata_backup_clean; + if backups_clean { + let _ = filesystem.remove_file(&journal_path); + } Ok(()) } @@ -963,9 +1267,16 @@ mod tests { fn replacement_failure_after_backup_restores_plugin_and_metadata() { let (root, target, temp_dir, metadata_temp) = replacement_fixture("rollback"); let filesystem = FailingRenameFilesystem { fail_at: 2, calls: Cell::new(0) }; - let error = - replace_installed_plugin_with_fs(&filesystem, &root, "demo", &temp_dir, &metadata_temp) - .unwrap_err(); + let error = replace_installed_plugin_with_fs( + &filesystem, + &root, + "demo", + &temp_dir, + &metadata_temp, + None, + || Ok(()), + ) + .unwrap_err(); assert!(error.to_string().contains("back up")); assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "old"); assert_eq!( @@ -997,6 +1308,87 @@ mod tests { fs::remove_dir_all(root).unwrap(); } + #[test] + fn reconciliation_restores_an_interrupted_replacement() { + let (root, target, temp_dir, metadata_temp) = replacement_fixture("reconcile"); + let metadata_path = registry_metadata_path(&root, "demo"); + let target_backup = root.join(".demo.recovery.plugin-backup"); + let metadata_backup = root.join(".registry/.demo.recovery.metadata-backup.json"); + fs::rename(&target, &target_backup).unwrap(); + fs::rename(&metadata_path, &metadata_backup).unwrap(); + fs::rename(&temp_dir, &target).unwrap(); + let journal_path = install_journal_path(&root, "demo"); + write_install_journal( + &journal_path, + &InstallJournal { + name: "demo".into(), + target_backup: target_backup.clone(), + metadata_backup: metadata_backup.clone(), + temp_dir: temp_dir.clone(), + metadata_temp: metadata_temp.clone(), + target_existed: true, + metadata_existed: true, + config_snapshot: None, + phase: InstallJournalPhase::Prepared, + }, + ) + .unwrap(); + + reconcile_install_transactions(&root).unwrap(); + assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "old"); + assert_eq!( + read_registry_metadata(&root, "demo").unwrap().id, + "sidebar_plugin_11111111111111111111111111111111" + ); + assert!(!journal_path.exists()); + assert!(!target_backup.exists()); + assert!(!metadata_backup.exists()); + assert!(!metadata_temp.exists()); + fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn config_callback_failure_rolls_back_filesystem_and_config() { + let (root, target, temp_dir, metadata_temp) = replacement_fixture("config-rollback"); + let config_path = root.join("config.json"); + fs::write( + &config_path, + "{\"sidebar\":{\"plugin\":{\"command\":[\"old\"]}},\"theme\":{\"name\":\"keep\"}}\n", + ) + .unwrap(); + let snapshot = ConfigSnapshot { + path: config_path.clone(), + sidebar_plugin: Some(json!({"command": ["old"]})), + }; + let error = replace_installed_plugin_with_fs( + &StandardInstallFilesystem, + &root, + "demo", + &temp_dir, + &metadata_temp, + Some(snapshot), + || { + fs::write( + &config_path, + "{\"sidebar\":{\"plugin\":{\"command\":[\"new\"]}},\"theme\":{\"name\":\"keep\"}}\n", + )?; + anyhow::bail!("injected config failure") + }, + ) + .unwrap_err(); + assert!(error.to_string().contains("injected config failure")); + assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "old"); + let restored: Value = + serde_json::from_str(&fs::read_to_string(config_path).unwrap()).unwrap(); + assert_eq!(restored["sidebar"]["plugin"]["command"], json!(["old"])); + assert_eq!(restored["theme"]["name"], json!("keep")); + assert_eq!( + read_registry_metadata(&root, "demo").unwrap().id, + "sidebar_plugin_11111111111111111111111111111111" + ); + fs::remove_dir_all(root).unwrap(); + } + #[test] fn plugin_snapshot_matches_the_closed_catalog_shape() { let root = std::env::temp_dir().join(format!( From fe4bd0ada7e74ae16d09e9b3ae1a5ec8be90f853 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:08:24 -0700 Subject: [PATCH 04/18] fix(plugin): constrain crash journal filesystem paths --- .../crates/cmux-tui/src/plugin_manager.rs | 236 +++++++++++++++++- 1 file changed, 229 insertions(+), 7 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 41fa378212b4..835ede0f449b 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -1,5 +1,5 @@ use std::fs; -use std::io::Write; +use std::io::{Read, Write}; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -127,6 +127,7 @@ struct PluginRegistryMetadata { } #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] #[serde(rename_all = "snake_case")] enum InstallJournalPhase { Prepared, @@ -134,6 +135,7 @@ enum InstallJournalPhase { } #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] struct InstallJournal { name: String, target_backup: PathBuf, @@ -147,16 +149,22 @@ struct InstallJournal { } #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] struct ConfigSnapshot { path: PathBuf, sidebar_plugin: Option, } +const INVALID_INSTALL_JOURNAL_DIR: &str = ".invalid-install-journals"; +const MAX_INSTALL_JOURNAL_BYTES: usize = 1024 * 1024; +const JOURNAL_QUARANTINE_ATTEMPTS: usize = 16; + struct PluginOperationLock(fs::File); fn acquire_plugin_operation_lock() -> anyhow::Result { let root = install_root()?; fs::create_dir_all(&root)?; + ensure_real_directory(&root)?; let path = root.join(".install.lock"); let file = fs::OpenOptions::new().create(true).read(true).write(true).open(path)?; file.lock()?; @@ -684,7 +692,14 @@ fn write_install_journal(path: &Path, journal: &InstallJournal) -> anyhow::Resul )); let encoded = serde_json::to_vec(journal)?; let result = (|| -> anyhow::Result<()> { - let mut file = fs::OpenOptions::new().create_new(true).write(true).open(&temp)?; + let mut options = fs::OpenOptions::new(); + options.create_new(true).write(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC).mode(0o600); + } + let mut file = options.open(&temp)?; file.write_all(&encoded)?; file.write_all(b"\n")?; file.sync_all()?; @@ -699,6 +714,38 @@ fn write_install_journal(path: &Path, journal: &InstallJournal) -> anyhow::Resul result } +/// Read a recovery journal without following a final-component symlink or +/// accepting an unbounded file. Journals control cleanup after a crash, so a +/// malformed or attacker-written file must never become an arbitrary file +/// operation. +fn read_install_journal(path: &Path) -> anyhow::Result> { + let mut options = fs::OpenOptions::new(); + options.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC); + } + let mut file = options.open(path)?; + let metadata = file.metadata()?; + anyhow::ensure!(metadata.is_file(), "install journal is not a regular file"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + anyhow::ensure!( + metadata.permissions().mode() & 0o7777 == 0o600, + "install journal permissions are not private" + ); + } + let mut bytes = Vec::new(); + file.take((MAX_INSTALL_JOURNAL_BYTES + 1) as u64).read_to_end(&mut bytes)?; + anyhow::ensure!( + bytes.len() <= MAX_INSTALL_JOURNAL_BYTES, + "install journal exceeds {MAX_INSTALL_JOURNAL_BYTES} bytes" + ); + Ok(bytes) +} + #[cfg(unix)] fn sync_directory(path: &Path) -> anyhow::Result<()> { fs::File::open(path)?.sync_all()?; @@ -724,7 +771,118 @@ fn remove_path_if_present(path: &Path) -> anyhow::Result<()> { Ok(()) } +fn ensure_real_directory(path: &Path) -> anyhow::Result<()> { + let metadata = fs::symlink_metadata(path)?; + anyhow::ensure!( + metadata.is_dir() && !metadata.file_type().is_symlink(), + "plugin state directory is not a real directory" + ); + Ok(()) +} + +fn direct_child_named(parent: &Path, path: &Path, predicate: impl FnOnce(&str) -> bool) -> bool { + path.parent() == Some(parent) + && path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(predicate) +} + +fn validate_install_journal_paths( + install_root: &Path, + journal_path: &Path, + journal: &InstallJournal, +) -> anyhow::Result<()> { + validate_plugin_name(&journal.name)?; + anyhow::ensure!( + journal_path == install_journal_path(install_root, &journal.name), + "install journal name does not match its path" + ); + anyhow::ensure!( + direct_child_named(install_root, &journal.target_backup, |name| { + name.starts_with(&format!(".{}.", journal.name)) && name.ends_with(".plugin-backup") + }), + "install journal target backup is outside the install root" + ); + anyhow::ensure!( + direct_child_named(install_root, &journal.temp_dir, |name| { + name.starts_with(".install-") + }), + "install journal temporary directory is outside the install root" + ); + let registry = install_root.join(".registry"); + if fs::symlink_metadata(®istry).is_ok() { + ensure_real_directory(®istry)?; + } + anyhow::ensure!( + direct_child_named(®istry, &journal.metadata_backup, |name| { + name.starts_with(&format!(".{}.", journal.name)) && name.ends_with(".metadata-backup.json") + }), + "install journal metadata backup is outside the registry" + ); + anyhow::ensure!( + direct_child_named(®istry, &journal.metadata_temp, |name| { + name.starts_with(&format!(".{}.", journal.name)) && name.ends_with(".tmp") + }), + "install journal metadata staging path is outside the registry" + ); + if let Some(snapshot) = &journal.config_snapshot { + anyhow::ensure!( + snapshot.path == config::config_path()?, + "install journal config snapshot path is not the active config" + ); + } + Ok(()) +} + +fn quarantine_install_journal(install_root: &Path, path: &Path) -> anyhow::Result<()> { + let quarantine = install_root.join(INVALID_INSTALL_JOURNAL_DIR); + match fs::symlink_metadata(&quarantine) { + Ok(metadata) => anyhow::ensure!(metadata.is_dir(), "invalid journal quarantine is not a directory"), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + match fs::create_dir(&quarantine) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { + ensure_real_directory(&quarantine)?; + } + Err(error) => return Err(error.into()), + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + fs::set_permissions(&quarantine, fs::Permissions::from_mode(0o700))?; + } + } + Err(error) => return Err(error.into()), + } + let basename = path + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("invalid-install-journal"); + for attempt in 0..JOURNAL_QUARANTINE_ATTEMPTS { + let destination = quarantine.join(format!( + "{basename}.{}-{attempt}.quarantined", + std::process::id() + )); + if fs::symlink_metadata(&destination).is_ok() { + continue; + } + match fs::rename(path, &destination) { + Ok(()) => return Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error.into()), + } + } + anyhow::bail!("could not quarantine invalid install journal") +} + fn reconcile_install_transactions(install_root: &Path) -> anyhow::Result<()> { + if let Ok(metadata) = fs::symlink_metadata(install_root) { + anyhow::ensure!( + metadata.is_dir() && !metadata.file_type().is_symlink(), + "plugin install root is not a real directory" + ); + } let entries = match fs::read_dir(install_root) { Ok(entries) => entries, Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), @@ -737,10 +895,24 @@ fn reconcile_install_transactions(install_root: &Path) -> anyhow::Result<()> { if !name.starts_with('.') || !name.ends_with(".install-journal.json") { continue; } - let journal: InstallJournal = - serde_json::from_slice(&fs::read(&path)?).map_err(|error| { - anyhow::anyhow!("invalid install journal {}: {error}", path.display()) - })?; + let journal_bytes = match read_install_journal(&path) { + Ok(bytes) => bytes, + Err(_) => { + quarantine_install_journal(install_root, &path)?; + continue; + } + }; + let journal: InstallJournal = match serde_json::from_slice(&journal_bytes) { + Ok(journal) => journal, + Err(_) => { + quarantine_install_journal(install_root, &path)?; + continue; + } + }; + if validate_install_journal_paths(install_root, &path, &journal).is_err() { + quarantine_install_journal(install_root, &path)?; + continue; + } match journal.phase { InstallJournalPhase::Committed => { remove_path_if_present(&journal.target_backup)?; @@ -817,7 +989,14 @@ fn write_config_value_atomic_local(path: &Path, value: &Value) -> anyhow::Result let temp = parent.join(format!(".{file_name}.{}.{}-restore.tmp", std::process::id(), now_nanos())); let result = (|| -> anyhow::Result<()> { - let mut file = fs::OpenOptions::new().create_new(true).write(true).open(&temp)?; + let mut options = fs::OpenOptions::new(); + options.create_new(true).write(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC).mode(0o600); + } + let mut file = options.open(&temp)?; serde_json::to_writer_pretty(&mut file, value)?; file.write_all(b"\n")?; file.sync_all()?; @@ -891,6 +1070,11 @@ fn replace_installed_plugin_with_fs anyhow: config_snapshot: Option, after_install: C, ) -> anyhow::Result<()> { + ensure_real_directory(install_root)?; + let registry = install_root.join(".registry"); + if fs::symlink_metadata(®istry).is_ok() { + ensure_real_directory(®istry)?; + } let target = install_root.join(name); let target_exists = target.exists(); let metadata_path = registry_metadata_path(install_root, name); @@ -1024,6 +1208,7 @@ fn write_registry_metadata_temp( validate_plugin_id(&metadata.id)?; let registry = install_root.join(".registry"); fs::create_dir_all(®istry)?; + ensure_real_directory(®istry)?; let temp = registry.join(format!(".{name}.{}-{}.tmp", std::process::id(), now_nanos())); let encoded = serde_json::to_vec(metadata)?; let mut file = fs::OpenOptions::new().create_new(true).write(true).open(&temp)?; @@ -1347,6 +1532,43 @@ mod tests { fs::remove_dir_all(root).unwrap(); } + #[test] + fn invalid_journal_paths_are_quarantined_without_touching_outside_files() { + let root = std::env::temp_dir().join(format!( + "cmux-plugin-journal-boundary-{}-{}", + std::process::id(), + now_nanos() + )); + let registry = root.join(".registry"); + let outside = root.with_file_name(format!("{}-outside", root.file_name().unwrap().to_string_lossy())); + fs::create_dir_all(®istry).unwrap(); + fs::write(&outside, b"keep").unwrap(); + let journal_path = install_journal_path(&root, "demo"); + write_install_journal( + &journal_path, + &InstallJournal { + name: "demo".into(), + target_backup: outside.clone(), + metadata_backup: registry.join(".demo.recovery.metadata-backup.json"), + temp_dir: root.join(".install-1-2"), + metadata_temp: registry.join(".demo.1-2.tmp"), + target_existed: false, + metadata_existed: false, + config_snapshot: None, + phase: InstallJournalPhase::Committed, + }, + ) + .unwrap(); + + reconcile_install_transactions(&root).unwrap(); + assert_eq!(fs::read(&outside).unwrap(), b"keep"); + assert!(!journal_path.exists()); + let quarantined = root.join(INVALID_INSTALL_JOURNAL_DIR); + assert_eq!(fs::read_dir(quarantined).unwrap().count(), 1); + fs::remove_file(outside).unwrap(); + fs::remove_dir_all(root).unwrap(); + } + #[test] fn config_callback_failure_rolls_back_filesystem_and_config() { let (root, target, temp_dir, metadata_temp) = replacement_fixture("config-rollback"); From 9da0327a19c4db02fa13275792e34022b077293f Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:31:52 -0700 Subject: [PATCH 05/18] style: format plugin journal hardening --- .../crates/cmux-tui/src/plugin_manager.rs | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 835ede0f449b..9940d6e14bf1 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -782,10 +782,7 @@ fn ensure_real_directory(path: &Path) -> anyhow::Result<()> { fn direct_child_named(parent: &Path, path: &Path, predicate: impl FnOnce(&str) -> bool) -> bool { path.parent() == Some(parent) - && path - .file_name() - .and_then(|name| name.to_str()) - .is_some_and(predicate) + && path.file_name().and_then(|name| name.to_str()).is_some_and(predicate) } fn validate_install_journal_paths( @@ -816,7 +813,8 @@ fn validate_install_journal_paths( } anyhow::ensure!( direct_child_named(®istry, &journal.metadata_backup, |name| { - name.starts_with(&format!(".{}.", journal.name)) && name.ends_with(".metadata-backup.json") + name.starts_with(&format!(".{}.", journal.name)) + && name.ends_with(".metadata-backup.json") }), "install journal metadata backup is outside the registry" ); @@ -838,7 +836,9 @@ fn validate_install_journal_paths( fn quarantine_install_journal(install_root: &Path, path: &Path) -> anyhow::Result<()> { let quarantine = install_root.join(INVALID_INSTALL_JOURNAL_DIR); match fs::symlink_metadata(&quarantine) { - Ok(metadata) => anyhow::ensure!(metadata.is_dir(), "invalid journal quarantine is not a directory"), + Ok(metadata) => { + anyhow::ensure!(metadata.is_dir(), "invalid journal quarantine is not a directory") + } Err(error) if error.kind() == std::io::ErrorKind::NotFound => { match fs::create_dir(&quarantine) { Ok(()) => {} @@ -855,15 +855,11 @@ fn quarantine_install_journal(install_root: &Path, path: &Path) -> anyhow::Resul } Err(error) => return Err(error.into()), } - let basename = path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("invalid-install-journal"); + let basename = + path.file_name().and_then(|name| name.to_str()).unwrap_or("invalid-install-journal"); for attempt in 0..JOURNAL_QUARANTINE_ATTEMPTS { - let destination = quarantine.join(format!( - "{basename}.{}-{attempt}.quarantined", - std::process::id() - )); + let destination = + quarantine.join(format!("{basename}.{}-{attempt}.quarantined", std::process::id())); if fs::symlink_metadata(&destination).is_ok() { continue; } @@ -1540,7 +1536,8 @@ mod tests { now_nanos() )); let registry = root.join(".registry"); - let outside = root.with_file_name(format!("{}-outside", root.file_name().unwrap().to_string_lossy())); + let outside = + root.with_file_name(format!("{}-outside", root.file_name().unwrap().to_string_lossy())); fs::create_dir_all(®istry).unwrap(); fs::write(&outside, b"keep").unwrap(); let journal_path = install_journal_path(&root, "demo"); From 7050d99441fd165f42ab02289a7e98ea101e25a6 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 03:27:03 -0700 Subject: [PATCH 06/18] test(plugin): require private manager failure diagnostics --- .../crates/cmux-tui/src/plugin_manager.rs | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 9940d6e14bf1..e037f927e415 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -49,7 +49,9 @@ impl ManagerError { } details } - Self::Failure(error) => json!({"reason": error.to_string()}), + Self::Failure(_) => { + json!({"reason": crate::localization::catalog().plugin.operation_failed}) + } } } @@ -63,7 +65,9 @@ impl std::fmt::Display for ManagerError { match self { Self::Usage(message) => formatter.write_str(message), Self::Validation { reason, .. } => formatter.write_str(reason), - Self::Failure(error) => std::fmt::Display::fmt(error, formatter), + Self::Failure(_) => { + formatter.write_str(crate::localization::catalog().plugin.operation_failed) + } } } } @@ -1302,6 +1306,19 @@ fn now_nanos() -> u128 { #[cfg(test)] mod tests { use super::*; + + #[test] + fn manager_failure_response_redacts_internal_error_details() { + let error = ManagerError::from(anyhow::anyhow!( + "failed to read /private/plugin-state/secret.toml: injected detail" + )); + let rendered = error.to_string(); + let details = error.details().to_string(); + assert!(!rendered.contains("/private/plugin-state/secret.toml")); + assert!(!rendered.contains("injected detail")); + assert!(!details.contains("/private/plugin-state/secret.toml")); + assert!(!details.contains("injected detail")); + } use std::cell::Cell; fn manifest_text(name: &str) -> String { From 444a5af88c4bed225a1ac4e8b8120244f24b698c Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 03:27:56 -0700 Subject: [PATCH 07/18] fix(plugin): localize private operation failure response --- cmux-tui/crates/cmux-tui/src/localization.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/cmux-tui/crates/cmux-tui/src/localization.rs b/cmux-tui/crates/cmux-tui/src/localization.rs index 0899c55b4f5c..fecdd4cfa597 100644 --- a/cmux-tui/crates/cmux-tui/src/localization.rs +++ b/cmux-tui/crates/cmux-tui/src/localization.rs @@ -122,6 +122,11 @@ pub(crate) struct SessionMessages { mux_subscription_recovery_failed: &'static str, } +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct PluginMessages { + pub operation_failed: &'static str, +} + impl SessionMessages { pub(crate) fn mux_subscription_recovery_failed(&self, error: &str) -> String { self.mux_subscription_recovery_failed.replace("{error}", error) @@ -1096,6 +1101,7 @@ pub(crate) struct Catalog { pub graphics: GraphicsMessages, pub terminal: TerminalMessages, pub session: SessionMessages, + pub plugin: PluginMessages, pub session_reset: SessionResetMessages, pub machine_agent: MachineAgentMessages, pub menu: MenuMessages, @@ -1235,6 +1241,9 @@ static ENGLISH: Catalog = Catalog { mux_subscription_recovered: "Mux event backlog overflowed; subscription recovered", mux_subscription_recovery_failed: "Mux event backlog recovery failed; queued input was discarded while retrying: {error}", }, + plugin: PluginMessages { + operation_failed: "Plugin operation failed; retry the command", + }, session_reset: SessionResetMessages { help: " cmux session reset-state [--force --confirm-reset ] [--state ]\n Preview or confirm a scoped saved-state reset", exact_name_required: "session reset-state requires an exact session name", @@ -1881,6 +1890,9 @@ static JAPANESE: Catalog = Catalog { mux_subscription_recovered: "Mux イベントの滞留が上限を超えました。購読を復旧しました", mux_subscription_recovery_failed: "Mux イベントの滞留から復旧できませんでした。再試行中のキュー入力を破棄しました: {error}", }, + plugin: PluginMessages { + operation_failed: "プラグイン操作に失敗しました。コマンドを再試行してください", + }, session_reset: SessionResetMessages { help: " cmux session reset-state [--force --confirm-reset ] [--state ]\n スコープ付き保存状態のリセットをプレビューまたは確認実行", exact_name_required: "session reset-state には正確なセッション名が必要です", From 8e06fbf13dddaf2a57c390434812889ada84c28f Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 03:52:16 -0700 Subject: [PATCH 08/18] fix(plugin): redact install validation details --- cmux-tui/crates/cmux-tui/src/plugin_manager.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index e037f927e415..4a081e89e829 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -211,18 +211,16 @@ fn install_command(positionals: &[String], options: &CliOptions) -> Result Result { - let manifest = read_manifest(&temp_dir) - .map_err(|error| ManagerError::validation(None, error.to_string()))?; + let manifest = read_manifest(&temp_dir).map_err(|_| { + ManagerError::validation(Some("manifest"), "plugin manifest is invalid") + })?; let name = installed_name(&manifest, options.name.as_deref()) - .map_err(|error| ManagerError::validation(Some("name"), error.to_string()))?; + .map_err(|_| ManagerError::validation(Some("name"), "plugin name is invalid"))?; let target = root.join(&name); if target.exists() && !options.force { return Err(ManagerError::validation( Some("name"), - format!( - "plugin {name:?} is already installed at {}; use --force to replace it", - target.display() - ), + "plugin is already installed; use --force to replace it", )); } run_build_if_needed(&manifest, &temp_dir)?; From 26cbfd06c1b26349d89209f539dd3d623b3e9758 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 04:19:48 -0700 Subject: [PATCH 09/18] fix(plugin): use exclusive lock API --- cmux-tui/crates/cmux-tui/src/plugin_manager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 4a081e89e829..165e2714c5b1 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -171,7 +171,7 @@ fn acquire_plugin_operation_lock() -> anyhow::Result { ensure_real_directory(&root)?; let path = root.join(".install.lock"); let file = fs::OpenOptions::new().create(true).read(true).write(true).open(path)?; - file.lock()?; + file.lock_exclusive()?; Ok(PluginOperationLock(file)) } From 51a9d6bd7c84c55be4559373c82344510aecf9c6 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 04:25:05 -0700 Subject: [PATCH 10/18] style(plugin): format localized failure message --- cmux-tui/crates/cmux-tui/src/localization.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/localization.rs b/cmux-tui/crates/cmux-tui/src/localization.rs index fecdd4cfa597..ac39dea84c81 100644 --- a/cmux-tui/crates/cmux-tui/src/localization.rs +++ b/cmux-tui/crates/cmux-tui/src/localization.rs @@ -1241,9 +1241,7 @@ static ENGLISH: Catalog = Catalog { mux_subscription_recovered: "Mux event backlog overflowed; subscription recovered", mux_subscription_recovery_failed: "Mux event backlog recovery failed; queued input was discarded while retrying: {error}", }, - plugin: PluginMessages { - operation_failed: "Plugin operation failed; retry the command", - }, + plugin: PluginMessages { operation_failed: "Plugin operation failed; retry the command" }, session_reset: SessionResetMessages { help: " cmux session reset-state [--force --confirm-reset ] [--state ]\n Preview or confirm a scoped saved-state reset", exact_name_required: "session reset-state requires an exact session name", From 453a6b451e053b3639f61665b383d656b9c6c794 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 04:32:34 -0700 Subject: [PATCH 11/18] fix(plugin): use fs4 lock API --- cmux-tui/crates/cmux-tui/src/plugin_manager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 165e2714c5b1..4a081e89e829 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -171,7 +171,7 @@ fn acquire_plugin_operation_lock() -> anyhow::Result { ensure_real_directory(&root)?; let path = root.join(".install.lock"); let file = fs::OpenOptions::new().create(true).read(true).write(true).open(path)?; - file.lock_exclusive()?; + file.lock()?; Ok(PluginOperationLock(file)) } From bb45b5c45cefbd5584bb552a4e55ac3008768dd6 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:28:14 -0700 Subject: [PATCH 12/18] fix(plugin): sync registry before commit --- cmux-tui/crates/cmux-tui/src/plugin_manager.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 4a081e89e829..ba9474348c71 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -1119,6 +1119,7 @@ fn replace_installed_plugin_with_fs anyhow: anyhow::anyhow!("failed to persist {}: {error}", metadata_path.display()) })?; metadata_installed = true; + sync_directory(®istry)?; after_install()?; write_install_journal( &journal_path, From 513dc43c6ee4cc799ad191ae569614a2dc0a9614 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:36:50 -0700 Subject: [PATCH 13/18] fix(plugin): avoid locking read-only commands --- .../crates/cmux-tui/src/plugin_manager.rs | 96 +++++++++++++++---- 1 file changed, 80 insertions(+), 16 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index ba9474348c71..ef0d30cce2ef 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -176,16 +176,32 @@ fn acquire_plugin_operation_lock() -> anyhow::Result { } pub(crate) fn execute(positionals: &[String], options: CliOptions) -> Result { - let _lock = acquire_plugin_operation_lock().map_err(ManagerError::Failure)?; - match positionals.first().map(String::as_str) { - Some("install") => install_command(positionals, &options), - Some("list") => list_command(positionals, &options), - Some("use") => use_command(positionals, &options), - Some("update") => update_command(positionals, &options), - Some("remove") => remove_command(positionals, &options), - Some(other) => Err(ManagerError::Usage(format!("unknown plugin subcommand {other:?}"))), - None => Err(ManagerError::Usage("plugin subcommand is required".to_string())), - } + with_optional_operation_lock( + command_requires_operation_lock(positionals), + || acquire_plugin_operation_lock().map_err(ManagerError::Failure), + || match positionals.first().map(String::as_str) { + Some("install") => install_command(positionals, &options), + Some("list") => list_command(positionals, &options), + Some("use") => use_command(positionals, &options), + Some("update") => update_command(positionals, &options), + Some("remove") => remove_command(positionals, &options), + Some(other) => Err(ManagerError::Usage(format!("unknown plugin subcommand {other:?}"))), + None => Err(ManagerError::Usage("plugin subcommand is required".to_string())), + }, + ) +} + +fn command_requires_operation_lock(positionals: &[String]) -> bool { + matches!(positionals.first().map(String::as_str), Some("install" | "use" | "update" | "remove")) +} + +fn with_optional_operation_lock( + requires_lock: bool, + acquire: impl FnOnce() -> Result, + operation: impl FnOnce() -> Result, +) -> Result { + let _lock = requires_lock.then(acquire).transpose()?; + operation() } fn install_command(positionals: &[String], options: &CliOptions) -> Result { @@ -274,7 +290,7 @@ fn list_command(positionals: &[String], options: &CliOptions) -> Result Result Result { persist_sidebar_plugin(None)?; - let plugins = installed_plugins()?; + let plugins = installed_plugins(true)?; Ok(json!({"plugins": plugins.iter().map(plugin_json).collect::>()})) } @@ -379,9 +395,13 @@ fn reject_plugin_flags( Ok(()) } -fn installed_plugins() -> anyhow::Result> { +fn installed_plugins(reconcile: bool) -> anyhow::Result> { let root = install_root()?; - reconcile_install_transactions(&root)?; + // Read-only listing skips recovery so a read-only install root does not + // need a lock file. Mutating callers hold the operation lock and reconcile. + if reconcile { + reconcile_install_transactions(&root)?; + } let selected = selected_plugin_cwd()?; let mut plugins = Vec::new(); let entries = match fs::read_dir(&root) { @@ -430,7 +450,7 @@ fn resolve_installed_plugin(selector: &str) -> Result u128 { #[cfg(test)] mod tests { use super::*; + use std::cell::Cell; #[test] fn manager_failure_response_redacts_internal_error_details() { @@ -1318,7 +1339,50 @@ mod tests { assert!(!details.contains("/private/plugin-state/secret.toml")); assert!(!details.contains("injected detail")); } - use std::cell::Cell; + + #[test] + fn read_only_plugin_command_skips_operation_lock() { + let operation_ran = Cell::new(false); + let result = with_optional_operation_lock( + false, + || panic!("read-only plugin command must not acquire the operation lock"), + || { + operation_ran.set(true); + Ok::<_, ManagerError>(()) + }, + ); + assert!(result.is_ok()); + assert!(operation_ran.get()); + } + + #[test] + fn mutating_plugin_command_acquires_operation_lock_before_running() { + let lock_acquired = Cell::new(false); + let operation_ran = Cell::new(false); + let result = with_optional_operation_lock( + true, + || { + lock_acquired.set(true); + Err(ManagerError::Usage("lock probe".to_string())) + }, + || { + operation_ran.set(true); + Ok::<_, ManagerError>(()) + }, + ); + assert!(result.is_err()); + assert!(lock_acquired.get()); + assert!(!operation_ran.get()); + } + + #[test] + fn only_mutating_plugin_commands_require_operation_lock() { + assert!(!command_requires_operation_lock(&["list".to_string()])); + for command in ["install", "use", "update", "remove"] { + assert!(command_requires_operation_lock(&[command.to_string()])); + } + assert!(!command_requires_operation_lock(&[])); + } fn manifest_text(name: &str) -> String { format!( From 49735bae42600082e387854c0a22174ce2a7fb8a Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:44:14 -0700 Subject: [PATCH 14/18] fix(plugin): fail closed on pending installs --- .../crates/cmux-tui/src/plugin_manager.rs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index ef0d30cce2ef..a60b7b388145 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -401,6 +401,8 @@ fn installed_plugins(reconcile: bool) -> anyhow::Result> { // need a lock file. Mutating callers hold the operation lock and reconcile. if reconcile { reconcile_install_transactions(&root)?; + } else { + reject_listing_with_pending_transaction(&root)?; } let selected = selected_plugin_cwd()?; let mut plugins = Vec::new(); @@ -439,6 +441,22 @@ fn installed_plugins(reconcile: bool) -> anyhow::Result> { Ok(plugins) } +fn reject_listing_with_pending_transaction(install_root: &Path) -> anyhow::Result<()> { + let entries = match fs::read_dir(install_root) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error.into()), + }; + for entry in entries { + let entry = entry?; + let Some(name) = entry.file_name().to_str().map(str::to_owned) else { continue }; + if name.starts_with('.') && name.ends_with(".install-journal.json") { + anyhow::bail!("plugin installation recovery is pending"); + } + } + Ok(()) +} + fn resolve_installed_plugin(selector: &str) -> Result { let forced_name = selector.strip_prefix("name:"); let selector = forced_name.unwrap_or(selector); @@ -1384,6 +1402,36 @@ mod tests { assert!(!command_requires_operation_lock(&[])); } + #[test] + fn read_only_listing_allows_a_clean_root_without_creating_a_lock() { + let root = std::env::temp_dir().join(format!( + "cmux-plugin-read-only-list-{}-{}", + std::process::id(), + now_nanos() + )); + fs::create_dir_all(&root).unwrap(); + assert!(reject_listing_with_pending_transaction(&root).is_ok()); + assert!(!root.join(".install.lock").exists()); + fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn read_only_listing_rejects_pending_journal_without_mutating_root() { + let root = std::env::temp_dir().join(format!( + "cmux-plugin-read-only-pending-{}-{}", + std::process::id(), + now_nanos() + )); + fs::create_dir_all(&root).unwrap(); + let journal = root.join(".demo.install-journal.json"); + fs::write(&journal, b"pending").unwrap(); + let error = reject_listing_with_pending_transaction(&root).unwrap_err(); + assert_eq!(error.to_string(), "plugin installation recovery is pending"); + assert!(journal.exists()); + assert!(!root.join(".install.lock").exists()); + fs::remove_dir_all(root).unwrap(); + } + fn manifest_text(name: &str) -> String { format!( r#" From 74c2d430a4da5033836c41e30de8d0d65bf8e976 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:55:47 -0700 Subject: [PATCH 15/18] fix(plugin): order recovery and preserve dangling entries --- .../crates/cmux-tui/src/plugin_manager.rs | 79 ++++++++++++++++--- 1 file changed, 66 insertions(+), 13 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index a60b7b388145..bca4691482da 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -233,7 +233,7 @@ fn install_command(positionals: &[String], options: &CliOptions) -> Result Result Result Result { + // Recovery must complete before changing the user's selection. Otherwise a + // prepared transaction can restore its snapshot after we select the + // builtin plugin, silently undoing the requested operation. + let root = install_root()?; + reconcile_install_transactions(&root)?; persist_sidebar_plugin(None)?; - let plugins = installed_plugins(true)?; + let plugins = installed_plugins(false)?; Ok(json!({"plugins": plugins.iter().map(plugin_json).collect::>()})) } @@ -710,11 +715,13 @@ impl InstallFilesystem for StandardInstallFilesystem { } } -fn unique_backup_path(parent: &Path, name: &str, suffix: &str) -> PathBuf { +fn unique_backup_path(parent: &Path, name: &str, suffix: &str) -> anyhow::Result { loop { let path = parent.join(format!(".{name}.{}-{}{suffix}", std::process::id(), now_nanos())); - if !path.exists() { - return path; + match fs::symlink_metadata(&path) { + Ok(_) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(path), + Err(error) => return Err(error.into()), } } } @@ -797,6 +804,18 @@ fn sync_directory(_path: &Path) -> anyhow::Result<()> { Ok(()) } +/// Returns whether a directory entry exists, including a dangling symlink. +/// `Path::exists` follows symlinks and therefore cannot represent an existing +/// entry whose target has been removed. Transaction replacement and recovery +/// must preserve those entries as well as regular files and directories. +fn path_entry_exists(path: &Path) -> anyhow::Result { + match fs::symlink_metadata(path) { + Ok(_) => Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(error.into()), + } +} + fn remove_path_if_present(path: &Path) -> anyhow::Result<()> { let metadata = match fs::symlink_metadata(path) { Ok(metadata) => metadata, @@ -961,7 +980,7 @@ fn reconcile_install_transactions(install_root: &Path) -> anyhow::Result<()> { restore_config_snapshot(snapshot)?; } if journal.target_existed { - if journal.target_backup.exists() { + if path_entry_exists(&journal.target_backup)? { remove_path_if_present(&install_root.join(&journal.name))?; fs::rename(&journal.target_backup, install_root.join(&journal.name))?; } @@ -970,7 +989,7 @@ fn reconcile_install_transactions(install_root: &Path) -> anyhow::Result<()> { } if journal.metadata_existed { let metadata_path = registry_metadata_path(install_root, &journal.name); - if journal.metadata_backup.exists() { + if path_entry_exists(&journal.metadata_backup)? { remove_path_if_present(&metadata_path)?; fs::rename(&journal.metadata_backup, metadata_path)?; } @@ -1112,12 +1131,12 @@ fn replace_installed_plugin_with_fs anyhow: ensure_real_directory(®istry)?; } let target = install_root.join(name); - let target_exists = target.exists(); + let target_exists = path_entry_exists(&target)?; let metadata_path = registry_metadata_path(install_root, name); - let target_backup = unique_backup_path(install_root, name, ".plugin-backup"); + let target_backup = unique_backup_path(install_root, name, ".plugin-backup")?; let metadata_backup = - unique_backup_path(&install_root.join(".registry"), name, ".metadata-backup.json"); - let metadata_exists = metadata_path.exists(); + unique_backup_path(&install_root.join(".registry"), name, ".metadata-backup.json")?; + let metadata_exists = path_entry_exists(&metadata_path)?; let journal_path = install_journal_path(install_root, name); let journal = InstallJournal { name: name.to_string(), @@ -1597,6 +1616,40 @@ mod tests { fs::remove_dir_all(root).unwrap(); } + #[cfg(unix)] + #[test] + fn replacement_failure_preserves_dangling_plugin_and_metadata_symlinks() { + use std::os::unix::fs::symlink; + + let (root, target, temp_dir, metadata_temp) = replacement_fixture("dangling"); + let metadata_path = registry_metadata_path(&root, "demo"); + fs::remove_dir_all(&target).unwrap(); + fs::remove_file(&metadata_path).unwrap(); + let missing_target = root.join("missing-target"); + let missing_metadata = root.join("missing-metadata"); + symlink(&missing_target, &target).unwrap(); + symlink(&missing_metadata, &metadata_path).unwrap(); + + // The fourth rename is the metadata install. It fails after both + // dangling entries have been backed up, so rollback must restore the + // symlinks rather than treating them as absent. + let filesystem = FailingRenameFilesystem { fail_at: 4, calls: Cell::new(0) }; + let error = replace_installed_plugin_with_fs( + &filesystem, + &root, + "demo", + &temp_dir, + &metadata_temp, + None, + || Ok(()), + ) + .unwrap_err(); + assert!(error.to_string().contains("failed to persist")); + assert_eq!(fs::read_link(&target).unwrap(), missing_target); + assert_eq!(fs::read_link(&metadata_path).unwrap(), missing_metadata); + fs::remove_dir_all(root).unwrap(); + } + #[test] fn replacement_commits_new_plugin_and_metadata_together() { let (root, target, temp_dir, metadata_temp) = replacement_fixture("success"); From 15a3465cc5936397b2a4c742b9b0c0bf8d36ded5 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 07:21:19 -0700 Subject: [PATCH 16/18] fix(plugin): clean up non-directory backups --- .../crates/cmux-tui/src/plugin_manager.rs | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index bca4691482da..03d209ef5455 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -715,6 +715,14 @@ impl InstallFilesystem for StandardInstallFilesystem { } } +fn remove_entry_with_filesystem( + filesystem: &F, + path: &Path, +) -> std::io::Result<()> { + let metadata = fs::symlink_metadata(path)?; + if metadata.is_dir() { filesystem.remove_dir_all(path) } else { filesystem.remove_file(path) } +} + fn unique_backup_path(parent: &Path, name: &str, suffix: &str) -> anyhow::Result { loop { let path = parent.join(format!(".{name}.{}-{}{suffix}", std::process::id(), now_nanos())); @@ -1205,7 +1213,7 @@ fn replace_installed_plugin_with_fs anyhow: if target_installed { if let Err(rollback_error) = filesystem.rename(&target, temp_dir) { rollback_errors.push(format!("plugin staging: {rollback_error}")); - match filesystem.remove_dir_all(&target) { + match remove_entry_with_filesystem(filesystem, &target) { Ok(()) => {} Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} Err(error) => rollback_errors.push(format!("plugin removal: {error}")), @@ -1238,7 +1246,7 @@ fn replace_installed_plugin_with_fs anyhow: // The replacement is committed once both new paths are in place. Keep the // committed journal until all cleanup succeeds, so a later invocation can // retry cleanup if access is temporarily unavailable. - let backup_dir_clean = match filesystem.remove_dir_all(&target_backup) { + let backup_dir_clean = match remove_entry_with_filesystem(filesystem, &target_backup) { Ok(()) => true, Err(error) if error.kind() == std::io::ErrorKind::NotFound => true, Err(_) => false, @@ -1670,6 +1678,28 @@ mod tests { fs::remove_dir_all(root).unwrap(); } + #[test] + fn replacement_cleans_up_a_regular_file_target_backup() { + let (root, target, temp_dir, metadata_temp) = replacement_fixture("file-target"); + fs::remove_dir_all(&target).unwrap(); + fs::write(&target, b"old file").unwrap(); + + replace_installed_plugin(&root, "demo", &temp_dir, &metadata_temp).unwrap(); + assert_eq!(fs::read_to_string(target.join("marker")).unwrap(), "new"); + assert_eq!( + read_registry_metadata(&root, "demo").unwrap().id, + "sidebar_plugin_22222222222222222222222222222222" + ); + assert!(!install_journal_path(&root, "demo").exists()); + assert!( + fs::read_dir(&root) + .unwrap() + .filter_map(Result::ok) + .all(|entry| !entry.file_name().to_string_lossy().contains("plugin-backup")) + ); + fs::remove_dir_all(root).unwrap(); + } + #[test] fn reconciliation_restores_an_interrupted_replacement() { let (root, target, temp_dir, metadata_temp) = replacement_fixture("reconcile"); From 6afe1dd244ef21bc45e9720765e6ce806af0d433 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 09:52:33 -0700 Subject: [PATCH 17/18] fix(tui): use supported plugin lock API --- cmux-tui/crates/cmux-tui/src/plugin_manager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 03d209ef5455..2c8ae518a5c8 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -171,7 +171,7 @@ fn acquire_plugin_operation_lock() -> anyhow::Result { ensure_real_directory(&root)?; let path = root.join(".install.lock"); let file = fs::OpenOptions::new().create(true).read(true).write(true).open(path)?; - file.lock()?; + file.lock_exclusive()?; Ok(PluginOperationLock(file)) } From e49583644e7958ad1972dbb17f24499fc1578c7b Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 28 Aug 2026 10:39:56 -0700 Subject: [PATCH 18/18] fix(tui): use fs4 lock API supported by MSRV --- cmux-tui/crates/cmux-tui/src/plugin_manager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs index 2c8ae518a5c8..1eca0cacfb1a 100644 --- a/cmux-tui/crates/cmux-tui/src/plugin_manager.rs +++ b/cmux-tui/crates/cmux-tui/src/plugin_manager.rs @@ -171,7 +171,7 @@ fn acquire_plugin_operation_lock() -> anyhow::Result { ensure_real_directory(&root)?; let path = root.join(".install.lock"); let file = fs::OpenOptions::new().create(true).read(true).write(true).open(path)?; - file.lock_exclusive()?; + FileExt::lock(&file)?; Ok(PluginOperationLock(file)) }