From 838335f84dcc20f7bbe58301ba35a75dc600de42 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Fri, 11 Sep 2026 15:28:42 -0600 Subject: [PATCH] feat: let install commands remove files that have been removed from bundles, both internal and external --- src/config/agent.rs | 39 +++++ src/config/install_remote.rs | 296 ++++++++++++++++++++++++++++++++-- src/config/request_context.rs | 83 ++++++++++ 3 files changed, 403 insertions(+), 15 deletions(-) diff --git a/src/config/agent.rs b/src/config/agent.rs index c67cd49..757100e 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -31,6 +31,15 @@ use std::{env, ffi::OsStr, path::Path}; const DEFAULT_AGENT_NAME: &str = "rag"; +const AGENT_DEFINITION_FILES: [&str; 6] = [ + "config.yaml", + "graph.yaml", + "tools.sh", + "tools.py", + "tools.ts", + "tools.js", +]; + pub type AgentVariables = IndexMap; #[derive(Embed)] @@ -95,6 +104,36 @@ impl Agent { } } + let mut bundled_files: HashMap> = HashMap::new(); + for file in AgentAssets::iter() { + if let Some((agent, rest)) = file.as_ref().split_once('/') { + bundled_files + .entry(agent.to_string()) + .or_default() + .insert(rest.to_string()); + } + } + for (agent, files) in &bundled_files { + for candidate in AGENT_DEFINITION_FILES { + if files.contains(candidate) { + continue; + } + let stale_path = paths::agents_data_dir().join(agent).join(candidate); + if stale_path.exists() { + info!( + "Removing stale file no longer shipped by built-in agent '{agent}': {}", + stale_path.display() + ); + if let Err(err) = std::fs::remove_file(&stale_path) { + warn!( + "Failed to remove stale agent file {}: {err}", + stale_path.display() + ); + } + } + } + } + Ok(()) } diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 9b8d0a8..18fdcea 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -464,10 +464,9 @@ fn owned_unmodified_mcp_keys( enum ObsoleteAction { Keep, Delete, + Backup, } -/// Kept files stay in the record, so a later uninstall still offers to -/// remove them. fn handle_obsolete_files( store: &mut BundleStore, bundle: &str, @@ -479,21 +478,21 @@ fn handle_obsolete_files( .iter() .map(|planned| provenance_path(&planned.dst)) .collect(); - let obsolete: Vec = store + let obsolete: Vec<(String, String)> = store .get(bundle) .map(|record| { record .files .iter() - .map(|file| file.path.clone()) - .filter(|path| !planned.contains(path)) + .filter(|file| !planned.contains(&file.path)) + .map(|file| (file.path.clone(), file.sha256.clone())) .collect() }) .unwrap_or_default(); let config_dir = paths::config_dir(); let mut sticky: Option = assume_yes.then_some(ObsoleteAction::Keep); - for path in obsolete { + for (path, recorded_sha) in obsolete { if !is_safe_relative_path(&path) { eprintln!("skipping suspicious recorded path {path}; keeping its record"); continue; @@ -506,13 +505,33 @@ fn handle_obsolete_files( continue; } - let action = resolve_obsolete(&path, &mut sticky)?; - apply_obsolete_action(store, bundle, &path, &full, &config_dir, action)?; + let modified = hash_file(&full)? != recorded_sha; + let action = match (modified, agent_definition_conflict(&path, &planned)) { + (false, _) => ObsoleteAction::Delete, + (true, true) => ObsoleteAction::Backup, + (true, false) => resolve_obsolete(&path, &mut sticky)?, + }; + apply_obsolete_action(store, bundle, &path, &full, &config_dir, action, modified)?; } Ok(()) } +fn agent_definition_conflict(path: &str, planned: &HashSet) -> bool { + let Some(rest) = path.strip_prefix("agents/") else { + return false; + }; + let Some((agent, file)) = rest.split_once('/') else { + return false; + }; + let counterpart = match file { + "config.yaml" => "graph.yaml", + "graph.yaml" => "config.yaml", + _ => return false, + }; + planned.contains(&format!("agents/{agent}/{counterpart}")) +} + fn resolve_obsolete(path: &str, sticky: &mut Option) -> Result { if let Some(action) = *sticky { return Ok(action); @@ -522,7 +541,9 @@ fn resolve_obsolete(path: &str, sticky: &mut Option) -> Result Result<()> { match action { ObsoleteAction::Keep => { @@ -559,7 +581,29 @@ fn apply_obsolete_action( .with_context(|| format!("failed to delete obsolete file {}", full.display()))?; store.remove_file_record(bundle, path)?; prune_empty_dirs(full, config_dir); - println!("deleted obsolete file {path}"); + if modified { + println!("deleted obsolete file {path}"); + } else { + println!("deleted obsolete file {path} (unchanged since install)"); + } + } + ObsoleteAction::Backup => { + let mut backup_name = full.as_os_str().to_os_string(); + backup_name.push(".bak"); + let backup = PathBuf::from(backup_name); + if backup.exists() { + fs::remove_file(&backup).with_context(|| { + format!("failed to remove stale backup {}", backup.display()) + })?; + } + fs::rename(full, &backup) + .with_context(|| format!("failed to back up obsolete file {}", full.display()))?; + store.remove_file_record(bundle, path)?; + println!( + "moved obsolete file {path} to {path}.bak: keeping it would break the agent \ + (both config.yaml and graph.yaml present); your local modifications are \ + preserved in the backup" + ); } } @@ -847,6 +891,9 @@ fn apply_uninstall_file_action( ObsoleteAction::Delete => { delete_owned_file(store, bundle, path, full, config_dir, summary)?; } + ObsoleteAction::Backup => { + unreachable!("uninstall never resolves to a backup action") + } } Ok(()) @@ -1018,6 +1065,9 @@ fn uninstall_mcp_entries( released.push(key.clone()); summary.removed.push(key); } + ObsoleteAction::Backup => { + unreachable!("uninstall never resolves to a backup action") + } } } @@ -3892,10 +3942,34 @@ mod tests { #[test] #[serial] - fn update_keeps_obsolete_files_non_interactively() { + fn update_deletes_unmodified_obsolete_files() { + let _guard = TestVaultConfigGuard::new("upd-obsolete-del"); + let src_root = fresh_temp_dir("upd-obsolete-del-src-"); + let repo = src_root.join("bundle"); + write_src(&repo, BUNDLE_MANIFEST_FILE, "name: obs-del\n"); + write_src(&repo, "macros/keep.yaml", "k\n"); + write_src(&repo, "macros/gone.yaml", "g\n"); + init_bundle_repo(&repo); + install_remote(repo.to_str().unwrap(), None, false).unwrap(); + fs::remove_file(repo.join("macros/gone.yaml")).unwrap(); + commit_file(&repo, "macros/keep.yaml", "k2\n"); + + update_bundle("obs-del", false).unwrap(); + + assert!(!paths::macros_dir().join("gone.yaml").exists()); + let store = BundleStore::load().unwrap(); + let record = store.get("obs-del").unwrap(); + assert!(record.files.iter().all(|f| f.path != "macros/gone.yaml")); + let _ = fs::remove_dir_all(&src_root); + } + + #[test] + #[serial] + fn update_keeps_modified_obsolete_files_non_interactively() { if *IS_STDOUT_TERMINAL { eprintln!( - "Skipping update_keeps_obsolete_files_non_interactively: requires non-TTY stdout" + "Skipping update_keeps_modified_obsolete_files_non_interactively: \ + requires non-TTY stdout" ); return; } @@ -3907,6 +3981,7 @@ mod tests { write_src(&repo, "macros/gone.yaml", "g\n"); init_bundle_repo(&repo); install_remote(repo.to_str().unwrap(), None, false).unwrap(); + fs::write(paths::macros_dir().join("gone.yaml"), "g-local\n").unwrap(); fs::remove_file(repo.join("macros/gone.yaml")).unwrap(); commit_file(&repo, "macros/keep.yaml", "k2\n"); @@ -3914,7 +3989,7 @@ mod tests { assert_eq!( fs::read_to_string(paths::macros_dir().join("gone.yaml")).unwrap(), - "g\n" + "g-local\n" ); let store = BundleStore::load().unwrap(); let record = store.get("obs-keep").unwrap(); @@ -3944,8 +4019,16 @@ mod tests { ) .unwrap(); - apply_obsolete_action(&mut store, "omc", &path, &dst, &dir, ObsoleteAction::Delete) - .unwrap(); + apply_obsolete_action( + &mut store, + "omc", + &path, + &dst, + &dir, + ObsoleteAction::Delete, + false, + ) + .unwrap(); assert!(!dst.exists()); let reloaded = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); @@ -4013,6 +4096,189 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + #[test] + #[serial] + fn handle_obsolete_deletes_unmodified_files_without_prompting() { + let _guard = TestVaultConfigGuard::new("upd-obsolete-unmod"); + let dir = paths::config_dir(); + let mut store = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); + store + .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) + .unwrap(); + let dst = dir.join("macros/gone.yaml"); + write_src(&dir, "macros/gone.yaml", "g"); + store + .record_file( + "omc", + FileRecord { + path: "macros/gone.yaml".to_string(), + category: "macros".to_string(), + sha256: hash_file(&dst).unwrap(), + action: FileAction::New, + }, + ) + .unwrap(); + let plan = InstallPlan { + files: Vec::new(), + mcp_json: None, + }; + + handle_obsolete_files(&mut store, "omc", &plan, false).unwrap(); + + assert!(!dst.exists()); + assert!(store.get("omc").unwrap().files.is_empty()); + } + + #[test] + #[serial] + fn handle_obsolete_keeps_modified_files_non_interactively() { + if *IS_STDOUT_TERMINAL { + eprintln!( + "Skipping handle_obsolete_keeps_modified_files_non_interactively: \ + requires non-TTY stdout" + ); + return; + } + let _guard = TestVaultConfigGuard::new("upd-obsolete-mod"); + let dir = paths::config_dir(); + let mut store = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); + store + .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) + .unwrap(); + let dst = dir.join("macros/gone.yaml"); + write_src(&dir, "macros/gone.yaml", "local edits"); + store + .record_file( + "omc", + FileRecord { + path: "macros/gone.yaml".to_string(), + category: "macros".to_string(), + sha256: "0".repeat(64), + action: FileAction::New, + }, + ) + .unwrap(); + let plan = InstallPlan { + files: Vec::new(), + mcp_json: None, + }; + + handle_obsolete_files(&mut store, "omc", &plan, false).unwrap(); + + assert_eq!(fs::read_to_string(&dst).unwrap(), "local edits"); + assert_eq!(store.get("omc").unwrap().files.len(), 1); + } + + #[test] + #[serial] + fn handle_obsolete_backs_up_modified_agent_definition_conflicts() { + let _guard = TestVaultConfigGuard::new("upd-obsolete-agent-bak"); + let dir = paths::config_dir(); + let mut store = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); + store + .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) + .unwrap(); + let dst = dir.join("agents/foo/config.yaml"); + write_src(&dir, "agents/foo/config.yaml", "local edits"); + store + .record_file( + "omc", + FileRecord { + path: "agents/foo/config.yaml".to_string(), + category: "agents".to_string(), + sha256: "0".repeat(64), + action: FileAction::New, + }, + ) + .unwrap(); + let plan = InstallPlan { + files: vec![PlannedFile { + src: dir.join("bundle-src/agents/foo/graph.yaml"), + dst: dir.join("agents/foo/graph.yaml"), + kind: PlannedKind::New, + top_category: TopCategory::Agents, + }], + mcp_json: None, + }; + + handle_obsolete_files(&mut store, "omc", &plan, false).unwrap(); + + assert!(!dst.exists()); + assert_eq!( + fs::read_to_string(dir.join("agents/foo/config.yaml.bak")).unwrap(), + "local edits" + ); + assert!(store.get("omc").unwrap().files.is_empty()); + } + + #[test] + #[serial] + fn handle_obsolete_deletes_unmodified_agent_definition_conflicts() { + let _guard = TestVaultConfigGuard::new("upd-obsolete-agent-del"); + let dir = paths::config_dir(); + let mut store = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); + store + .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) + .unwrap(); + let dst = dir.join("agents/foo/config.yaml"); + write_src(&dir, "agents/foo/config.yaml", "shipped content"); + store + .record_file( + "omc", + FileRecord { + path: "agents/foo/config.yaml".to_string(), + category: "agents".to_string(), + sha256: hash_file(&dst).unwrap(), + action: FileAction::New, + }, + ) + .unwrap(); + let plan = InstallPlan { + files: vec![PlannedFile { + src: dir.join("bundle-src/agents/foo/graph.yaml"), + dst: dir.join("agents/foo/graph.yaml"), + kind: PlannedKind::New, + top_category: TopCategory::Agents, + }], + mcp_json: None, + }; + + handle_obsolete_files(&mut store, "omc", &plan, false).unwrap(); + + assert!(!dst.exists()); + assert!(!dir.join("agents/foo/config.yaml.bak").exists()); + assert!(store.get("omc").unwrap().files.is_empty()); + } + + #[test] + fn agent_definition_conflict_matches_only_planned_counterparts() { + let planned: HashSet = [ + "agents/foo/graph.yaml".to_string(), + "agents/bar/config.yaml".to_string(), + ] + .into_iter() + .collect(); + + assert!(agent_definition_conflict( + "agents/foo/config.yaml", + &planned + )); + assert!(agent_definition_conflict("agents/bar/graph.yaml", &planned)); + assert!(!agent_definition_conflict( + "agents/foo/graph.yaml", + &planned + )); + assert!(!agent_definition_conflict( + "agents/baz/config.yaml", + &planned + )); + assert!(!agent_definition_conflict( + "agents/foo/scripts/config.yaml", + &planned + )); + assert!(!agent_definition_conflict("macros/config.yaml", &planned)); + } + #[test] #[serial] fn update_refreshes_record_metadata_and_stamps_updated_at() { diff --git a/src/config/request_context.rs b/src/config/request_context.rs index b8c5cdb..285db31 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -8038,6 +8038,86 @@ mod tests { ); } + #[test] + #[serial] + fn install_builtin_agents_removes_stale_definition_files() { + let _guard = TestConfigDirGuard::new(); + + Agent::install_builtin_agents(false).unwrap(); + + let agents_dir = paths::agents_data_dir(); + // Simulate an upgrade from an older install: coder used to ship + // config.yaml + tools.py (now graph.yaml + tools.sh) and demo used + // to ship tools.sh (now tools.py + tools.sh.bak). + write(agents_dir.join("coder").join("config.yaml"), "stale").unwrap(); + write(agents_dir.join("coder").join("tools.py"), "stale").unwrap(); + write(agents_dir.join("demo").join("tools.sh"), "stale").unwrap(); + // Reconciliation must never touch subdirectories, non-definition + // root files, or agents that aren't part of the bundle. + create_dir_all(agents_dir.join("coder").join("sessions")).unwrap(); + write( + agents_dir.join("coder").join("sessions").join("keep.yaml"), + "keep", + ) + .unwrap(); + write(agents_dir.join("coder").join(".env"), "keep").unwrap(); + create_dir_all(agents_dir.join("myagent")).unwrap(); + write(agents_dir.join("myagent").join("config.yaml"), "keep").unwrap(); + + Agent::install_builtin_agents(false).unwrap(); + + assert!( + !agents_dir.join("coder").join("config.yaml").exists(), + "stale config.yaml must be removed for a graph-based bundled agent" + ); + assert!( + !agents_dir.join("coder").join("tools.py").exists(), + "stale tools script must be removed when the bundle no longer ships it" + ); + assert!( + !agents_dir.join("demo").join("tools.sh").exists(), + "stale tools.sh must be removed when the bundle only ships tools.py" + ); + assert!(agents_dir.join("coder").join("graph.yaml").exists()); + assert!(agents_dir.join("coder").join("tools.sh").exists()); + assert!(agents_dir.join("demo").join("tools.py").exists()); + assert!( + agents_dir + .join("coder") + .join("sessions") + .join("keep.yaml") + .exists(), + "files in agent subdirectories must survive reconciliation" + ); + assert!( + agents_dir.join("coder").join(".env").exists(), + "non-definition root files must survive reconciliation" + ); + assert!( + agents_dir.join("myagent").join("config.yaml").exists(), + "custom agents outside the bundle must survive reconciliation" + ); + } + + #[test] + #[serial] + fn install_builtin_agents_removes_stale_graph_for_config_agent() { + let _guard = TestConfigDirGuard::new(); + + Agent::install_builtin_agents(false).unwrap(); + + let agents_dir = paths::agents_data_dir(); + write(agents_dir.join("architect").join("graph.yaml"), "stale").unwrap(); + + Agent::install_builtin_agents(false).unwrap(); + + assert!( + !agents_dir.join("architect").join("graph.yaml").exists(), + "stale graph.yaml must be removed for a config-based bundled agent" + ); + assert!(agents_dir.join("architect").join("config.yaml").exists()); + } + #[test] #[serial] fn install_builtin_skills_force_overwrites_only_with_force() { @@ -8141,6 +8221,8 @@ mod tests { // rule that fires on a shipped asset (or a new bundled graph) must be // recorded here deliberately. let expected_warnings = BTreeMap::from([ + ("adversary".to_string(), Vec::new()), + ("code-reviewer".to_string(), Vec::new()), ( "coder".to_string(), unreachable(&[ @@ -8160,6 +8242,7 @@ mod tests { ("deep-research".to_string(), unreachable(&["ask_topic"])), ("finding-verifier".to_string(), Vec::new()), ("librarian".to_string(), Vec::new()), + ("review-gauntlet".to_string(), Vec::new()), ( "step-runner".to_string(), unreachable(&[