From 6b5535956db4187180acc7d989a3048bd40e1bc5 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Fri, 21 Aug 2026 20:59:12 -0600 Subject: [PATCH] fix: harden the bundle lifecycle per code review The path-escape guard that uninstall applies to recorded paths now also covers update's obsolete-file deletion through a shared check, so a tampered store cannot turn either delete site into an arbitrary file removal. Updates gain a working non-interactive path: --yes now applies to --update-bundle (locally modified files, obsolete files, and modified mcp entries are all kept; everything else refreshes), owned mcp entries whose recorded hash still matches the local entry take the remote side without prompting, and the non-TTY conflict bails name the flag that actually works per surface. An update records its new commit and version only after files and mcp entries land, so an aborted update cannot claim content it never wrote. The store gains a version field and rejects stores from newer builds, the corrupt-store error no longer advises the removal that would forfeit ownership tracking, and duplicate records tracking one source abort a rename instead of overwriting a record. Reinstalling from a source URL reclassifies owned unmodified files as silent refreshes just like updates. git runs with GIT_TERMINAL_PROMPT=0 and a null stdin so private or mistyped URLs fail instead of hanging. File comparison fills buffers fully before comparing, deleting an obsolete file prunes emptied directories, mcp.json backfill uses the fsynced atomic writer, --list-bundles no longer triggers builtin backfill, bundle-name completion logs store errors instead of swallowing them and offers --yes, and REPL .uninstall rejects unknown flags. --- src/cli/mod.rs | 21 ++- src/config/bundles.rs | 43 ++++- src/config/install_remote.rs | 325 +++++++++++++++++++++++++++++----- src/config/request_context.rs | 44 ++--- src/function/mod.rs | 5 +- src/main.rs | 10 +- src/repl/mod.rs | 34 ++-- 7 files changed, 384 insertions(+), 98 deletions(-) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index f36e019..1114217 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -216,7 +216,7 @@ pub struct Cli { help_heading = "Installation & Updates" )] pub filter: Option, - /// Overwrite all conflicts without prompting (used with --install) + /// Overwrite all conflicts without prompting (remote installs only) #[arg( long, requires = "install", @@ -228,18 +228,24 @@ pub struct Cli { #[arg( long, value_name = "NAME", + group = "yes_scope", conflicts_with_all = ["uninstall"], help_heading = "Installation & Updates" )] pub update_bundle: Option, /// Uninstall a bundle: delete its owned files and remove its mcp.json entries - #[arg(long, value_name = "NAME", help_heading = "Installation & Updates")] - pub uninstall: Option, - /// Skip uninstall confirmation prompts (locally modified items are still kept) #[arg( long, - requires = "uninstall", - conflicts_with_all = ["install", "install_builtins", "update_bundle"], + value_name = "NAME", + group = "yes_scope", + help_heading = "Installation & Updates" + )] + pub uninstall: Option, + /// Proceed without prompts for --uninstall and --update-bundle (locally modified items are always kept) + #[arg( + long, + requires = "yes_scope", + conflicts_with_all = ["install", "install_builtins"], help_heading = "Installation & Updates" )] pub yes: bool, @@ -562,8 +568,9 @@ mod tests { } #[test] - fn parse_yes_flag_requires_uninstall() { + fn parse_yes_flag_requires_uninstall_or_update_bundle() { assert!(parse(&["--uninstall", "foo", "--yes"]).yes); + assert!(parse(&["--update-bundle", "foo", "--yes"]).yes); assert!(Cli::try_parse_from(["coyote", "--yes"]).is_err()); } diff --git a/src/config/bundles.rs b/src/config/bundles.rs index da51ebd..8b1b29c 100644 --- a/src/config/bundles.rs +++ b/src/config/bundles.rs @@ -100,14 +100,19 @@ pub(crate) struct ResolvedBundleName { pub(crate) same_name_other_source: Option, } +const STORE_VERSION: u32 = 1; + #[derive(Debug, Default, Deserialize)] struct StoreContents { + #[serde(default)] + version: u32, #[serde(default)] bundles: BTreeMap, } #[derive(Serialize)] struct StoreContentsRef<'a> { + version: u32, bundles: &'a BTreeMap, } @@ -137,10 +142,19 @@ impl BundleStore { let contents: StoreContents = serde_yaml::from_str(&content).with_context(|| { format!( "failed to parse {}; refusing to treat it as empty. \ - Fix or remove the file to continue", + Restore or repair the file to continue; removing it forfeits \ + uninstall tracking for every installed bundle", path.display() ) })?; + if contents.version > STORE_VERSION { + bail!( + "{} has store version {}, but this coyote build supports up to \ + {STORE_VERSION}; update coyote or restore a matching store", + path.display(), + contents.version + ); + } Ok(Self { path, bundles: contents.bundles, @@ -149,6 +163,7 @@ impl BundleStore { pub(crate) fn save(&self) -> Result<()> { let content = serde_yaml::to_string(&StoreContentsRef { + version: STORE_VERSION, bundles: &self.bundles, }) .context("failed to serialize the installed-bundles store")?; @@ -277,6 +292,13 @@ impl BundleStore { if let Some(old_key) = existing_key && !already_recorded { + if self.bundles.contains_key(&resolved.name) { + bail!( + "records '{old_key}' and '{}' both track source '{url}'; \ + uninstall one or repair installed-bundles.yaml before continuing", + resolved.name + ); + } let record = self .bundles .remove(&old_key) @@ -348,6 +370,25 @@ impl BundleStore { self.save() } + /// Written only after an update's files and mcp entries land, so an + /// aborted update cannot leave the record claiming a commit whose content + /// never finished applying. + pub(crate) fn set_bundle_versions( + &mut self, + name: &str, + commit: &str, + version: Option, + ) -> Result<()> { + self.ensure_bundle_exists(name)?; + let record = self + .bundles + .get_mut(name) + .expect("bundle existence checked above"); + record.commit = commit.to_string(); + record.version = version; + self.save() + } + pub(crate) fn remove_file_record(&mut self, bundle: &str, path: &str) -> Result<()> { self.ensure_bundle_exists(bundle)?; let record = self diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 8529fa6..c28568f 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -44,9 +44,11 @@ pub fn install_remote(git_url: &str, filter: Option, force: bool) reference.as_deref(), layout.manifest.as_ref(), temp.head_sha(), + false, )?; let plan = plan_changes(&layout)?; + let plan = reclassify_owned_unmodified(plan, &store, &bundle)?; if !plan.files.is_empty() { print_plan_summary(&plan); @@ -60,7 +62,7 @@ pub fn install_remote(git_url: &str, filter: Option, force: bool) if let Some((remote_mcp, local_mcp)) = &plan.mcp_json { let local = local_mcp.exists().then_some(local_mcp.as_path()); - let report = merge_mcp_json(local, remote_mcp, local_mcp, force)?; + let report = merge_mcp_json(local, remote_mcp, local_mcp, force, &HashSet::new(), false)?; record_mcp_merge(&mut store, &bundle, &report)?; print_mcp_merge_report(&report); handle_missing_secrets(&report.missing_secrets)?; @@ -225,7 +227,7 @@ pub fn install_or_update( if filter.is_some() || force { bail!("--filter/--install-force only apply to remote installs, not bundle updates"); } - update_bundle(value) + update_bundle(value, false) } InstallTarget::RemoteSource => install_remote(value, filter, force), InstallTarget::Shorthand => { @@ -267,7 +269,7 @@ pub fn install_or_update_from_repl_args(args: &str) -> Result<()> { /// The whole remote is always processed, including categories a filtered /// install excluded, because filtered installs merge into a single record. -pub fn update_bundle(spec: &str) -> Result<()> { +pub fn update_bundle(spec: &str, assume_yes: bool) -> Result<()> { let (name, ref_override) = parse_url_with_ref(spec)?; let mut store = BundleStore::load()?; @@ -312,6 +314,7 @@ pub fn update_bundle(spec: &str) -> Result<()> { effective_ref.as_deref(), layout.manifest.as_ref(), temp.head_sha(), + true, )?; let plan = plan_changes(&layout)?; @@ -319,19 +322,31 @@ pub fn update_bundle(spec: &str) -> Result<()> { if !plan.files.is_empty() { print_plan_summary(&plan); - apply_plan(&plan, StickyMode::None, &mut store, &bundle)?; + let sticky = if assume_yes { + StickyMode::KeepAll + } else { + StickyMode::None + }; + apply_plan(&plan, sticky, &mut store, &bundle)?; } - handle_obsolete_files(&mut store, &bundle, &plan)?; + handle_obsolete_files(&mut store, &bundle, &plan, assume_yes)?; if let Some((remote_mcp, local_mcp)) = &plan.mcp_json { let local = local_mcp.exists().then_some(local_mcp.as_path()); - let report = merge_mcp_json(local, remote_mcp, local_mcp, false)?; + let auto_take = owned_unmodified_mcp_keys(&store, &bundle, local)?; + let report = merge_mcp_json(local, remote_mcp, local_mcp, false, &auto_take, assume_yes)?; record_mcp_merge(&mut store, &bundle, &report)?; print_mcp_merge_report(&report); handle_missing_secrets(&report.missing_secrets)?; } + let version = layout + .manifest + .as_ref() + .and_then(|m| m.version.clone()) + .unwrap_or_else(|| temp.head_sha().chars().take(7).collect()); + store.set_bundle_versions(&bundle, temp.head_sha(), Some(version))?; store.mark_updated(&bundle)?; Ok(()) @@ -367,6 +382,39 @@ fn reclassify_owned_unmodified( Ok(plan) } +/// Keys of this bundle's mcp entries whose recorded hash still matches the +/// local entry: the bundle wrote them and the user never touched them, so an +/// upstream change takes the remote side without prompting. +fn owned_unmodified_mcp_keys( + store: &BundleStore, + bundle: &str, + local: Option<&Path>, +) -> Result> { + let mut keys = HashSet::new(); + let (Some(local_path), Some(record)) = (local, store.get(bundle)) else { + return Ok(keys); + }; + let content = fs::read_to_string(local_path) + .with_context(|| format!("failed to read local mcp.json at {}", local_path.display()))?; + let config: McpServersConfig = serde_json::from_str(&content) + .with_context(|| format!("failed to parse local mcp.json at {}", local_path.display()))?; + for server in &record.mcp_servers { + let Some(recorded_hash) = server.sha256.as_deref() else { + continue; + }; + let key = server.effective_key(); + let Some(entry) = config.mcp_servers.get(key) else { + continue; + }; + let serialized = serde_json::to_string(entry) + .with_context(|| format!("failed to serialize MCP server '{key}'"))?; + if hash_bytes(serialized.as_bytes()) == recorded_hash { + keys.insert(key.to_string()); + } + } + Ok(keys) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum ObsoleteAction { Keep, @@ -375,7 +423,12 @@ enum ObsoleteAction { /// Kept files stay in the record, so a later uninstall still offers to /// remove them. -fn handle_obsolete_files(store: &mut BundleStore, bundle: &str, plan: &InstallPlan) -> Result<()> { +fn handle_obsolete_files( + store: &mut BundleStore, + bundle: &str, + plan: &InstallPlan, + assume_yes: bool, +) -> Result<()> { let planned: HashSet = plan .files .iter() @@ -394,8 +447,12 @@ fn handle_obsolete_files(store: &mut BundleStore, bundle: &str, plan: &InstallPl .unwrap_or_default(); let config_dir = paths::config_dir(); - let mut sticky: Option = None; + let mut sticky: Option = assume_yes.then_some(ObsoleteAction::Keep); for path in obsolete { + if !is_safe_relative_path(&path) { + eprintln!("skipping suspicious recorded path {path}; keeping its record"); + continue; + } let full = config_dir.join(&path); if !full.exists() { println!("dropped record for obsolete file {path} (already absent locally)"); @@ -403,7 +460,7 @@ fn handle_obsolete_files(store: &mut BundleStore, bundle: &str, plan: &InstallPl continue; } let action = resolve_obsolete(&path, &mut sticky)?; - apply_obsolete_action(store, bundle, &path, &full, action)?; + apply_obsolete_action(store, bundle, &path, &full, &config_dir, action)?; } Ok(()) } @@ -441,6 +498,7 @@ fn apply_obsolete_action( bundle: &str, path: &str, full: &Path, + config_dir: &Path, action: ObsoleteAction, ) -> Result<()> { match action { @@ -451,6 +509,7 @@ fn apply_obsolete_action( fs::remove_file(full) .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}"); } } @@ -623,8 +682,17 @@ fn select_uninstall_candidate(store: &BundleStore, spec: &str) -> Result bool { + let recorded = Path::new(path); + !recorded.is_absolute() + && recorded + .components() + .all(|c| matches!(c, Component::Normal(_))) +} + fn uninstall_owned_files( store: &mut BundleStore, bundle: &str, @@ -635,12 +703,7 @@ fn uninstall_owned_files( let mut summary = UninstallFileSummary::default(); let mut sticky: Option = None; for file in files { - let recorded = Path::new(&file.path); - if recorded.is_absolute() - || !recorded - .components() - .all(|c| matches!(c, Component::Normal(_))) - { + if !is_safe_relative_path(&file.path) { eprintln!( "skipping suspicious recorded path {}; keeping its record", file.path @@ -648,7 +711,7 @@ fn uninstall_owned_files( summary.failed += 1; continue; } - let full = config_dir.join(recorded); + let full = config_dir.join(Path::new(&file.path)); if !full.exists() { println!( "dropped record for missing file {} (already absent locally)", @@ -992,6 +1055,8 @@ fn clone_to_temp(url: &str, reference: Option<&str>) -> Result { fn run_git(args: Vec) -> Result<()> { let output = duct::cmd("git", &args) + .env("GIT_TERMINAL_PROMPT", "0") + .stdin_null() .stderr_to_stdout() .stdout_capture() .unchecked() @@ -1008,6 +1073,8 @@ fn run_git(args: Vec) -> Result<()> { fn run_git_capture(args: Vec) -> Result { let output = duct::cmd("git", &args) + .env("GIT_TERMINAL_PROMPT", "0") + .stdin_null() .stdout_capture() .stderr_capture() .unchecked() @@ -1100,24 +1167,31 @@ pub(crate) struct BundleManifest { /// Returns the record key, possibly migrated or owner-qualified, that all /// subsequent recording must use in place of the requested name. +/// An update passes `preserve_versions` so the record keeps claiming the old +/// commit and version until the new content actually lands on disk. fn register_bundle( store: &mut BundleStore, url: &str, git_ref: Option<&str>, manifest: Option<&BundleManifest>, commit: &str, + preserve_versions: bool, ) -> Result { let resolved = store.resolve_bundle_name(url, manifest.map(|m| m.name.as_str()))?; let version = manifest .and_then(|m| m.version.clone()) .unwrap_or_else(|| commit.chars().take(7).collect()); + let (commit, version) = match (preserve_versions, store.get(&resolved.name)) { + (true, Some(existing)) => (existing.commit.clone(), existing.version.clone()), + _ => (commit.to_string(), Some(version)), + }; store.upsert_bundle( &resolved.name, InstallMetadata { source: url.to_string(), git_ref: git_ref.map(str::to_string), - commit: commit.to_string(), - version: Some(version), + commit, + version, description: manifest.and_then(|m| m.description.clone()), homepage: manifest.and_then(|m| m.homepage.clone()), }, @@ -1490,14 +1564,13 @@ fn files_equal(a: &Path, b: &Path) -> Result { } fn files_equal_streaming(a: &Path, b: &Path) -> Result { - use std::io::Read; let mut fa = fs::File::open(a).with_context(|| format!("open {}", a.display()))?; let mut fb = fs::File::open(b).with_context(|| format!("open {}", b.display()))?; let mut buf_a = [0u8; 8192]; let mut buf_b = [0u8; 8192]; loop { - let na = fa.read(&mut buf_a)?; - let nb = fb.read(&mut buf_b)?; + let na = read_full(&mut fa, &mut buf_a)?; + let nb = read_full(&mut fb, &mut buf_b)?; if na != nb { return Ok(false); } @@ -1510,6 +1583,21 @@ fn files_equal_streaming(a: &Path, b: &Path) -> Result { } } +/// `read` may return short counts without EOF; comparing partially filled +/// buffers positionally would misreport identical files as different. +fn read_full(file: &mut fs::File, buf: &mut [u8]) -> std::io::Result { + use std::io::Read; + let mut filled = 0; + while filled < buf.len() { + let n = file.read(&mut buf[filled..])?; + if n == 0 { + break; + } + filled += n; + } + Ok(filled) +} + fn print_plan_summary(plan: &InstallPlan) { println!("Plan:"); for cat in [ @@ -1692,7 +1780,8 @@ fn resolve_conflict(planned: &PlannedFile, sticky: &mut StickyMode) -> Result, + assume_yes: bool, ) -> Result { let remote_content = fs::read_to_string(remote) .with_context(|| format!("failed to read remote mcp.json at {}", remote.display()))?; @@ -1816,7 +1907,12 @@ fn merge_mcp_json( if local_server == &remote_server { continue; } - match resolve_mcp_conflict(&name, force)? { + let action = if auto_take.contains(&name) { + McpConflictAction::TakeRemote + } else { + resolve_mcp_conflict(&name, force, assume_yes)? + }; + match action { McpConflictAction::KeepLocal => report.kept_local.push(name), McpConflictAction::TakeRemote => { merged.mcp_servers.insert(name.clone(), remote_server); @@ -1882,14 +1978,17 @@ fn merge_mcp_json( Ok(report) } -fn resolve_mcp_conflict(name: &str, force: bool) -> Result { +fn resolve_mcp_conflict(name: &str, force: bool, assume_yes: bool) -> Result { if force { return Ok(McpConflictAction::TakeRemote); } + if assume_yes { + return Ok(McpConflictAction::KeepLocal); + } if !*IS_STDOUT_TERMINAL { bail!( "MCP server '{name}' already exists locally. Refusing to merge non-interactively. \ - Re-run with --install-force or in a terminal." + Re-run in a terminal, with --install-force (installs), or with --yes (updates)." ); } let rename_label = format!("rename remote as \"{name}-remote\""); @@ -2461,7 +2560,7 @@ mod tests { let target = dir.join("target.json"); write_mcp(&remote, FIXTURE_REMOTE); - let report = merge_mcp_json(None, &remote, &target, false).unwrap(); + let report = merge_mcp_json(None, &remote, &target, false, &HashSet::new(), false).unwrap(); assert_eq!(report.added, vec!["alpha", "beta"]); assert!(report.kept_local.is_empty()); @@ -2484,7 +2583,15 @@ mod tests { ); write_mcp(&remote, FIXTURE_REMOTE); - let report = merge_mcp_json(Some(&target), &remote, &target, true).unwrap(); + let report = merge_mcp_json( + Some(&target), + &remote, + &target, + true, + &HashSet::new(), + false, + ) + .unwrap(); assert_eq!(report.added, vec!["beta"]); assert_eq!(report.replaced, vec!["alpha"]); @@ -2513,7 +2620,15 @@ mod tests { ); write_mcp(&remote, FIXTURE_REMOTE); - let err = merge_mcp_json(Some(&target), &remote, &target, false).unwrap_err(); + let err = merge_mcp_json( + Some(&target), + &remote, + &target, + false, + &HashSet::new(), + false, + ) + .unwrap_err(); assert!( err.to_string() @@ -2530,7 +2645,8 @@ mod tests { let target = dir.join("target.json"); write_mcp(&remote, r#"{"mcpServers": {"broken": {"type": "stdio"}}}"#); - let err = merge_mcp_json(None, &remote, &target, false).unwrap_err(); + let err = + merge_mcp_json(None, &remote, &target, false, &HashSet::new(), false).unwrap_err(); assert!( format!("{err:#}").contains("missing a \"command\" field"), @@ -2557,7 +2673,7 @@ mod tests { r#"{"mcpServers": {"x": {"type":"stdio","command":"echo","env":{"K":"{{COYOTE_TEST_MERGE_SECRET}}"}}}}"#, ); - let report = merge_mcp_json(None, &remote, &target, false).unwrap(); + let report = merge_mcp_json(None, &remote, &target, false, &HashSet::new(), false).unwrap(); assert_eq!(report.missing_secrets, vec!["COYOTE_TEST_MERGE_SECRET"]); let _ = fs::remove_dir_all(&dir); @@ -2572,10 +2688,18 @@ mod tests { let target = dir.join("target.json"); write_mcp(&remote, FIXTURE_REMOTE); - merge_mcp_json(None, &remote, &target, false).unwrap(); + merge_mcp_json(None, &remote, &target, false, &HashSet::new(), false).unwrap(); let after_first = fs::read(&target).unwrap(); - let report = merge_mcp_json(Some(&target), &remote, &target, false).unwrap(); + let report = merge_mcp_json( + Some(&target), + &remote, + &target, + false, + &HashSet::new(), + false, + ) + .unwrap(); assert!(report.added.is_empty(), "got: {:?}", report.added); let after_second = fs::read(&target).unwrap(); @@ -3289,7 +3413,7 @@ mod tests { install_remote(repo.to_str().unwrap(), None, false).unwrap(); commit_file(&repo, "macros/hello.yaml", "v2\n"); - update_bundle("refresh-bundle").unwrap(); + update_bundle("refresh-bundle", false).unwrap(); let installed = paths::macros_dir().join("hello.yaml"); assert_eq!(fs::read_to_string(&installed).unwrap(), "v2\n"); @@ -3301,6 +3425,82 @@ mod tests { let _ = fs::remove_dir_all(&src_root); } + #[test] + #[serial] + fn update_with_yes_keeps_modified_files_and_updates_the_rest() { + if *IS_STDOUT_TERMINAL { + eprintln!( + "Skipping update_with_yes_keeps_modified_files_and_updates_the_rest: \ + requires non-TTY stdout" + ); + return; + } + let _guard = TestVaultConfigGuard::new("upd-yes"); + let src_root = fresh_temp_dir("upd-yes-src-"); + let repo = src_root.join("bundle"); + write_src(&repo, BUNDLE_MANIFEST_FILE, "name: yes-bundle\n"); + write_src(&repo, "macros/edited.yaml", "v1\n"); + write_src(&repo, "macros/pristine.yaml", "v1\n"); + init_bundle_repo(&repo); + install_remote(repo.to_str().unwrap(), None, false).unwrap(); + fs::write(paths::macros_dir().join("edited.yaml"), "local\n").unwrap(); + commit_file(&repo, "macros/edited.yaml", "v2\n"); + commit_file(&repo, "macros/pristine.yaml", "v2\n"); + + let err = update_bundle("yes-bundle", false).unwrap_err(); + assert!( + format!("{err:#}").contains("Refusing to overwrite"), + "err: {err:#}" + ); + + update_bundle("yes-bundle", true).unwrap(); + + assert_eq!( + fs::read_to_string(paths::macros_dir().join("edited.yaml")).unwrap(), + "local\n" + ); + assert_eq!( + fs::read_to_string(paths::macros_dir().join("pristine.yaml")).unwrap(), + "v2\n" + ); + let _ = fs::remove_dir_all(&src_root); + } + + #[test] + #[serial] + fn update_takes_remote_for_owned_unmodified_mcp_entries() { + if *IS_STDOUT_TERMINAL { + eprintln!( + "Skipping update_takes_remote_for_owned_unmodified_mcp_entries: \ + requires non-TTY stdout" + ); + return; + } + let _guard = TestVaultConfigGuard::new("upd-mcp-auto"); + let src_root = fresh_temp_dir("upd-mcp-auto-src-"); + let repo = src_root.join("bundle"); + write_src(&repo, BUNDLE_MANIFEST_FILE, "name: mcp-auto-bundle\n"); + write_src( + &repo, + "functions/mcp.json", + r#"{"mcpServers": {"srv": {"type": "stdio", "command": "echo"}}}"#, + ); + init_bundle_repo(&repo); + install_remote(repo.to_str().unwrap(), None, false).unwrap(); + commit_file( + &repo, + "functions/mcp.json", + r#"{"mcpServers": {"srv": {"type": "stdio", "command": "printf"}}}"#, + ); + + update_bundle("mcp-auto-bundle", false).unwrap(); + + let merged = fs::read_to_string(paths::mcp_config_file()).unwrap(); + assert!(merged.contains("printf"), "merged mcp.json: {merged}"); + assert!(!merged.contains("echo"), "merged mcp.json: {merged}"); + let _ = fs::remove_dir_all(&src_root); + } + #[test] fn apply_plan_keep_all_preserves_local_edit_and_stale_record() { let dir = fresh_temp_dir("upd-keep-modified-"); @@ -3412,7 +3612,7 @@ mod tests { fs::create_dir_all(paths::macros_dir()).unwrap(); fs::write(paths::macros_dir().join("user.yaml"), "local\n").unwrap(); - let err = update_bundle("unowned-bundle").unwrap_err(); + let err = update_bundle("unowned-bundle", false).unwrap_err(); assert!( err.to_string().contains("Refusing to overwrite"), @@ -3491,7 +3691,7 @@ mod tests { fs::remove_file(repo.join("macros/gone.yaml")).unwrap(); commit_file(&repo, "macros/keep.yaml", "k2\n"); - update_bundle("obs-keep").unwrap(); + update_bundle("obs-keep", false).unwrap(); assert_eq!( fs::read_to_string(paths::macros_dir().join("gone.yaml")).unwrap(), @@ -3525,7 +3725,8 @@ mod tests { ) .unwrap(); - apply_obsolete_action(&mut store, "omc", &path, &dst, ObsoleteAction::Delete).unwrap(); + apply_obsolete_action(&mut store, "omc", &path, &dst, &dir, ObsoleteAction::Delete) + .unwrap(); assert!(!dst.exists()); let reloaded = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); @@ -3540,12 +3741,11 @@ mod tests { store .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) .unwrap(); - let ghost = dir.join("macros/ghost.yaml"); store .record_file( "omc", FileRecord { - path: provenance_path(&ghost), + path: "macros/ghost-bundle-test.yaml".to_string(), category: "macros".to_string(), sha256: "0".repeat(64), action: FileAction::New, @@ -3557,12 +3757,43 @@ mod tests { mcp_json: None, }; - handle_obsolete_files(&mut store, "omc", &plan).unwrap(); + handle_obsolete_files(&mut store, "omc", &plan, false).unwrap(); assert!(store.get("omc").unwrap().files.is_empty()); let _ = fs::remove_dir_all(&dir); } + #[test] + fn handle_obsolete_never_touches_suspicious_recorded_paths() { + let dir = fresh_temp_dir("upd-obsolete-suspicious-"); + let mut store = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); + store + .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) + .unwrap(); + for hostile in ["/etc/nonexistent-coyote-test", "../escape.yaml"] { + store + .record_file( + "omc", + FileRecord { + path: hostile.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!(store.get("omc").unwrap().files.len(), 2); + let _ = fs::remove_dir_all(&dir); + } + #[test] #[serial] fn update_refreshes_record_metadata_and_stamps_updated_at() { @@ -3595,7 +3826,7 @@ mod tests { homepage: https://example.com/new\n", ); - update_bundle("meta-bundle").unwrap(); + update_bundle("meta-bundle", false).unwrap(); let store = BundleStore::load().unwrap(); let record = store.get("meta-bundle").unwrap(); @@ -3613,7 +3844,7 @@ mod tests { fn update_unknown_bundle_lists_installed_names() { let _guard = TestVaultConfigGuard::new("upd-unknown-name"); - let err = update_bundle("nope").unwrap_err(); + let err = update_bundle("nope", false).unwrap_err(); assert!(err.to_string().contains("none are installed"), "got: {err}"); let src_root = fresh_temp_dir("upd-unknown-src-"); @@ -3623,7 +3854,7 @@ mod tests { init_bundle_repo(&repo); install_remote(repo.to_str().unwrap(), None, false).unwrap(); - let err = update_bundle("nope").unwrap_err(); + let err = update_bundle("nope", false).unwrap_err(); assert!( err.to_string().contains("installed bundles: known-bundle"), @@ -3645,7 +3876,7 @@ mod tests { let newer = commit_file(&repo, "macros/two.yaml", "2\n"); assert_ne!(pinned, newer); - update_bundle("pin-bundle").unwrap(); + update_bundle("pin-bundle", false).unwrap(); let store = BundleStore::load().unwrap(); let record = store.get("pin-bundle").unwrap(); @@ -3667,7 +3898,7 @@ mod tests { install_remote(&format!("{}#{pinned}", repo.display()), None, false).unwrap(); let newer = commit_file(&repo, "macros/two.yaml", "2\n"); - update_bundle(&format!("move-bundle#{newer}")).unwrap(); + update_bundle(&format!("move-bundle#{newer}"), false).unwrap(); let store = BundleStore::load().unwrap(); let record = store.get("move-bundle").unwrap(); @@ -4467,7 +4698,7 @@ mod tests { .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) .unwrap(); - let report = merge_mcp_json(None, &remote, &target, false).unwrap(); + let report = merge_mcp_json(None, &remote, &target, false, &HashSet::new(), false).unwrap(); record_mcp_merge(&mut store, "omc", &report).unwrap(); let written: McpServersConfig = diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 0b75021..3987b89 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -59,6 +59,22 @@ use std::sync::Arc; use std::time::Duration; use std::{env, fs}; +/// Completion must degrade rather than break the prompt, but a corrupt store +/// should not vanish silently: the failure is logged before returning empty. +fn installed_bundle_names() -> Vec { + match BundleStore::load() { + Ok(store) => store + .bundle_names() + .into_iter() + .map(str::to_string) + .collect(), + Err(e) => { + warn!("skipping bundle-name completion: {e:#}"); + Vec::new() + } + } +} + pub struct AutoContinueConfig { pub enabled: bool, pub max_continues: usize, @@ -3263,30 +3279,16 @@ impl RequestContext { ".install" => { let mut values: Vec = AssetCategory::NAMES.iter().map(|s| s.to_string()).collect(); - values.extend( - BundleStore::load() - .map(|store| { - store - .bundle_names() - .into_iter() - .map(str::to_string) - .collect::>() - }) - .unwrap_or_default(), - ); + values.extend(installed_bundle_names()); super::map_completion_values(values) } ".uninstall" => { - let values = BundleStore::load() - .map(|store| { - store - .bundle_names() - .into_iter() - .map(str::to_string) - .collect::>() - }) - .unwrap_or_default(); - super::map_completion_values(values) + let mut values = super::map_completion_values(installed_bundle_names()); + values.push(( + "--yes".to_string(), + Some("Skip the uninstall confirmation".to_string()), + )); + values } ".macro" => { let policy = self.macro_policy(); diff --git a/src/function/mod.rs b/src/function/mod.rs index 7c0b81e..1aefeb5 100644 --- a/src/function/mod.rs +++ b/src/function/mod.rs @@ -493,9 +493,8 @@ impl Functions { let serialized = serde_json::to_string_pretty(&merged).context("failed to serialize merged mcp.json")?; - let tmp = file_path.with_extension("json.tmp"); - fs::write(&tmp, &serialized).context("failed to write temporary mcp.json")?; - fs::rename(&tmp, &file_path).context("failed to finalize mcp.json")?; + write_file_atomic(&file_path, &serialized, None) + .context("failed to write merged mcp.json")?; if !added.is_empty() { println!(" + new MCP servers: {}", added.join(", ")); diff --git a/src/main.rs b/src/main.rs index eebdaf0..3f98954 100644 --- a/src/main.rs +++ b/src/main.rs @@ -127,6 +127,10 @@ async fn main() -> Result<()> { return sandbox::launch(name.clone(), cli.fresh); } + if cli.list_bundles { + return config::list_installed_bundles(); + } + install_builtins()?; if let Some(category) = cli.install_builtins { @@ -143,17 +147,13 @@ async fn main() -> Result<()> { } if let Some(spec) = cli.update_bundle.as_deref() { - return config::update_bundle(spec); + return config::update_bundle(spec, cli.yes); } if let Some(name) = cli.uninstall.as_deref() { return config::uninstall_bundle(name, cli.yes); } - if cli.list_bundles { - return config::list_installed_bundles(); - } - if let Some(client_arg) = &cli.authenticate { let cfg = Config::load_with_interpolation(true).await?; let app_config = AppConfig::from_config(cfg)?; diff --git a/src/repl/mod.rs b/src/repl/mod.rs index 37cf9b3..12c68a9 100644 --- a/src/repl/mod.rs +++ b/src/repl/mod.rs @@ -1197,20 +1197,10 @@ pub async fn run_repl_command( println!("Usage: .delete ") } }, - ".uninstall" => { - let mut assume_yes = false; - let mut names = Vec::new(); - for token in args.unwrap_or("").split_whitespace() { - match token { - "--yes" | "-y" => assume_yes = true, - other => names.push(other), - } - } - match names.as_slice() { - [name] => config::uninstall_bundle(name, assume_yes)?, - _ => println!("Usage: .uninstall [--yes]"), - } - } + ".uninstall" => match parse_repl_uninstall(args) { + Some((name, assume_yes)) => config::uninstall_bundle(&name, assume_yes)?, + None => println!("Usage: .uninstall [--yes]"), + }, ".list" => match args { Some(args) => { ctx.list_assets(args.trim())?; @@ -1602,6 +1592,22 @@ fn parse_repl_install(args: Option<&str>) -> ReplInstallDispatch<'_> { } } +fn parse_repl_uninstall(args: Option<&str>) -> Option<(String, bool)> { + let mut assume_yes = false; + let mut names = Vec::new(); + for token in args.unwrap_or("").split_whitespace() { + match token { + "--yes" | "-y" => assume_yes = true, + other if other.starts_with('-') => return None, + other => names.push(other), + } + } + match names.as_slice() { + [name] => Some((name.to_string(), assume_yes)), + _ => None, + } +} + pub fn builtin_command_names() -> Vec<&'static str> { let mut names: Vec<&'static str> = REPL_COMMANDS .iter()