diff --git a/CHANGELOG.md b/CHANGELOG.md index b7fc815..6766dd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ --- -latest_version: 3.2.20 -released: 2026-05-29 +latest_version: 3.2.21 +released: 2026-05-30 --- # OneBrain CLI Changelog (v3.x · Rust) @@ -12,6 +12,16 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [Unreleased] +## [3.2.21] — 2026-05-30 — cache-clean hardening + +- fix(cache-clean): orphan cache dirs under an UNregistered marketplace are now + swept even when a registered marketplace exists (was gated on `dirs.is_empty()`). +- fix(cache-clean): `remove_dir_all` failures are surfaced (counted + stderr + warning) instead of silently dropped — `clean_plugin_cache` now returns + `CacheCleanOutcome { removed, failed }`. +- Verified the Step 9 sweep runs unconditionally on every real Claude update + (only `--dry-run` skips); added a clarifying comment. + ## [3.2.20] — 2026-05-29 — completions: exclude hidden commands - fix(cli): shell completions no longer list hidden/internal/legacy subcommands diff --git a/Cargo.lock b/Cargo.lock index 5391c13..136714c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -997,7 +997,7 @@ checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" [[package]] name = "onebrain-cache" -version = "3.2.20" +version = "3.2.21" dependencies = [ "chrono", "onebrain-core", @@ -1010,7 +1010,7 @@ dependencies = [ [[package]] name = "onebrain-cli" -version = "3.2.20" +version = "3.2.21" dependencies = [ "anyhow", "assert_cmd", @@ -1039,7 +1039,7 @@ dependencies = [ [[package]] name = "onebrain-core" -version = "3.2.20" +version = "3.2.21" dependencies = [ "chrono", "indexmap", @@ -1055,7 +1055,7 @@ dependencies = [ [[package]] name = "onebrain-fs" -version = "3.2.20" +version = "3.2.21" dependencies = [ "chrono", "dirs", diff --git a/Cargo.toml b/Cargo.toml index 8375675..eba87a4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ default-members = ["crates/*"] resolver = "2" [workspace.package] -version = "3.2.20" +version = "3.2.21" edition = "2021" license = "AGPL-3.0-only" authors = ["OneBrain Contributors"] diff --git a/crates/onebrain-cli/src/commands/doctor.rs b/crates/onebrain-cli/src/commands/doctor.rs index cd97a81..74f6530 100644 --- a/crates/onebrain-cli/src/commands/doctor.rs +++ b/crates/onebrain-cli/src/commands/doctor.rs @@ -628,11 +628,13 @@ fn fix_plugin_cache(json: bool) -> FixOutcome { }; status_line(json, "running: clean plugin cache"); // `None` → clean derives `/.claude/plugins/cache`. - let removed = clean_plugin_cache(&installed, None); + let outcome = clean_plugin_cache(&installed, None); + let removed = outcome.removed; // Honest result: re-detect after the sweep. If versions remain, a removal // failed (permissions, open handle, race) — report Failed rather than a // misleading "Fixed" with exit 0, so the user learns why the `plugin-cache` - // warning will still be there on the next doctor run. + // warning will still be there on the next doctor run. `outcome.failed` + // already counted those failures (and warned on stderr per path). let remaining = detect_stale_plugin_cache(&installed, None); if remaining.is_empty() { FixOutcome::Fixed(format!( @@ -641,7 +643,8 @@ fn fix_plugin_cache(json: bool) -> FixOutcome { )) } else { FixOutcome::Failed(format!( - "removed {removed}, but {} version(s) remain — check permissions on ~/.claude/plugins/cache", + "removed {removed}, {} failed, {} version(s) remain — check permissions on ~/.claude/plugins/cache", + outcome.failed, remaining.len() )) } diff --git a/crates/onebrain-fs/src/vault_sync/cache_clean.rs b/crates/onebrain-fs/src/vault_sync/cache_clean.rs index 90898be..60e7cca 100644 --- a/crates/onebrain-fs/src/vault_sync/cache_clean.rs +++ b/crates/onebrain-fs/src/vault_sync/cache_clean.rs @@ -1,8 +1,8 @@ //! Step 9 · detect + clean obsolete OneBrain cache versions under //! `//onebrain/`. `clean_plugin_cache` ports Bun's -//! `cleanPluginCache` (non-fatal — returns the count of successful removals); -//! `detect_stale_plugin_cache` is the read-only sibling added for the -//! `doctor` plugin-cache check. +//! `cleanPluginCache` (non-fatal — returns a [`CacheCleanOutcome`] of the +//! successful + failed removals); `detect_stale_plugin_cache` is the read-only +//! sibling added for the `doctor` plugin-cache check. //! //! Two public entry points share ONE discovery + enumeration pass: //! - [`detect_stale_plugin_cache`] — **read-only**; lists the orphan version @@ -13,9 +13,10 @@ //! Both walk the identical set of dirs (`discover_onebrain_cache_dirs` → //! `version_dirs_under`), so detection and removal can only diverge on the //! *removal step itself*: a `remove_dir_all` that fails (permission, race) is -//! not counted, so `clean`'s returned count is "removed", which may be **less -//! than** `detect`'s "found" when a removal fails. Callers that need to confirm -//! the cache is actually clean should re-run `detect_stale_plugin_cache` after. +//! counted in `CacheCleanOutcome::failed` (not `removed`), so `clean`'s +//! `removed` count may be **less than** `detect`'s "found" when a removal +//! fails. Callers that need to confirm the cache is actually clean should +//! re-run `detect_stale_plugin_cache` after. //! //! Why removing *every* cached version dir is safe: when OneBrain is pinned to //! the vault-local copy (`installPath` → `/.claude/plugins/onebrain`), @@ -75,12 +76,13 @@ fn resolve_cache_dir( /// Discover every `//onebrain/` directory. /// -/// Primary source: the marketplaces registered in `installed_plugins.json` -/// (keys shaped `onebrain@`). Fallback: if that file is missing or -/// unreadable, glob every `/*/onebrain/` so an unregistered orphan is -/// still found. The fallback path only ever joins real `read_dir` entries, so -/// it cannot traverse outside the cache; the JSON-key path is sanitized via -/// [`is_safe_marketplace`]. +/// Unions two sources (deduped): +/// 1. Marketplaces registered in `installed_plugins.json` (keys shaped +/// `onebrain@`), sanitized via [`is_safe_marketplace`]. +/// 2. An UNCONDITIONAL glob of `/*/onebrain/`, so an orphan under an +/// unregistered marketplace is found even when a registered marketplace also +/// exists. The glob only ever joins real `read_dir` entries, so it cannot +/// traverse outside the cache. fn discover_onebrain_cache_dirs(installed_plugins_path: &Path, cache_dir: &Path) -> Vec { let mut dirs: Vec = Vec::new(); @@ -107,21 +109,27 @@ fn discover_onebrain_cache_dirs(installed_plugins_path: &Path, cache_dir: &Path) } } - // Fallback glob — covers orphans whose marketplace is no longer registered - // in installed_plugins.json (exactly the 2.2.4 case we hit). - if dirs.is_empty() { - if let Ok(read) = fs::read_dir(cache_dir) { - // `.flatten()` drops `Err` entries (e.g. a racing unlink) and yields - // only the readable `DirEntry`s. - for entry in read.flatten() { - let candidate = entry.path().join("onebrain"); - if candidate.exists() { - dirs.push(candidate); - } + // ALWAYS also glob the cache: covers orphans whose marketplace is no longer + // registered in installed_plugins.json (the 2.2.4 case) EVEN WHEN a + // registered marketplace also exists. Previously gated on `dirs.is_empty()`, + // which missed an unregistered orphan whenever any registered marketplace + // was present. + if let Ok(read) = fs::read_dir(cache_dir) { + // `.flatten()` drops `Err` entries (e.g. a racing unlink) and yields + // only the readable `DirEntry`s. + for entry in read.flatten() { + let candidate = entry.path().join("onebrain"); + if candidate.exists() { + dirs.push(candidate); } } } + // A registered marketplace also surfaces in the glob — dedup so each + // `.../onebrain/` dir is enumerated once. + dirs.sort(); + dirs.dedup(); + dirs } @@ -185,31 +193,50 @@ pub fn detect_stale_plugin_cache( stale } +/// Outcome of a destructive cache sweep. `removed` + `failed` together account +/// for every version dir discovered, so a caller can report partial success +/// instead of silently dropping failures. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct CacheCleanOutcome { + pub removed: u64, + pub failed: u64, +} + /// **Destructive.** Step 9 entry point + `doctor --fix` worker. Removes every -/// version dir under the discovered onebrain cache dirs. Returns the count -/// actually **removed** (may be less than [`detect_stale_plugin_cache`] found -/// if a removal fails). Non-fatal — a failed removal is simply not counted. +/// version dir under the discovered onebrain cache dirs. Returns a +/// [`CacheCleanOutcome`] whose `removed` + `failed` together account for every +/// version dir found, so `removed` may be less than [`detect_stale_plugin_cache`] +/// found when a removal fails. Non-fatal — a failed removal is counted in +/// `failed` (and an stderr warning is emitted), never silently swallowed. pub fn clean_plugin_cache( installed_plugins_path: &Path, installed_plugins_cache_dir: Option<&Path>, -) -> u64 { +) -> CacheCleanOutcome { let Some(cache_dir) = resolve_cache_dir(installed_plugins_path, installed_plugins_cache_dir) else { - return 0; + return CacheCleanOutcome::default(); }; - let mut removed = 0u64; + let mut out = CacheCleanOutcome::default(); for plugin_dir in discover_onebrain_cache_dirs(installed_plugins_path, &cache_dir) { for path in version_dirs_under(&plugin_dir) { - // `remove_dir_all` is recursive; `.is_ok()` gates the counter so a - // permission error on one dir doesn't abort the rest. - if fs::remove_dir_all(&path).is_ok() { - removed += 1; + // `remove_dir_all` is recursive; the failure branch counts + warns so + // a permission error on one dir doesn't abort the rest and isn't lost. + match fs::remove_dir_all(&path) { + Ok(()) => out.removed += 1, + Err(e) => { + out.failed += 1; + // Surface, don't swallow: a stale cache dir we couldn't + // remove will keep shadowing the vault-local plugin. + eprintln!( + "vault-sync: failed to remove stale cache dir {}: {e}", + path.display() + ); + } } } } - - removed + out } #[cfg(test)] @@ -242,8 +269,9 @@ mod tests { } }), ); - let n = clean_plugin_cache(&installed, Some(&cache_dir)); - assert_eq!(n, 2); + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!(out.removed, 2); + assert_eq!(out.failed, 0); assert!(!v_a.exists()); assert!(!v_b.exists()); } @@ -253,8 +281,9 @@ mod tests { let dir = tempdir().unwrap(); let installed = dir.path().join("installed_plugins.json"); fs::write(&installed, "{}").unwrap(); - let n = clean_plugin_cache(&installed, Some(&dir.path().join("nonexistent-cache"))); - assert_eq!(n, 0); + let out = clean_plugin_cache(&installed, Some(&dir.path().join("nonexistent-cache"))); + assert_eq!(out.removed, 0); + assert_eq!(out.failed, 0); } #[test] @@ -265,8 +294,8 @@ mod tests { fs::create_dir_all(&v).unwrap(); let installed = dir.path().join("installed_plugins.json"); // No file written — fallback glob path. - let n = clean_plugin_cache(&installed, Some(&cache_dir)); - assert_eq!(n, 1); + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!(out.removed, 1); } // ── detection (read-only) ─────────────────────────────────────────────── @@ -323,8 +352,8 @@ mod tests { ); let detected = detect_stale_plugin_cache(&installed, Some(&cache_dir)); assert_eq!(detected.len(), 2); - let removed = clean_plugin_cache(&installed, Some(&cache_dir)); - assert_eq!(removed as usize, detected.len()); + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!(out.removed as usize, detected.len()); } #[test] @@ -346,8 +375,8 @@ mod tests { let detected = detect_stale_plugin_cache(&installed, Some(&cache_dir)); assert_eq!(detected.len(), 1, "only the version dir is stale"); assert_eq!(detected[0].version, "3.0.0"); - let removed = clean_plugin_cache(&installed, Some(&cache_dir)); - assert_eq!(removed, 1); + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!(out.removed, 1); assert!(stray.exists(), "stray file must survive the sweep"); } @@ -369,11 +398,74 @@ mod tests { &serde_json::json!({ "plugins": { "onebrain@../escape": [{"id":"onebrain"}] } }), ); - let removed = clean_plugin_cache(&installed, Some(&cache_dir)); - assert_eq!(removed, 0, "traversal key must not reach outside the cache"); + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!( + out.removed, 0, + "traversal key must not reach outside the cache" + ); assert!(escape.exists(), "the escape target must be untouched"); } + #[test] + fn glob_catches_orphan_under_unregistered_marketplace() { + // Registered marketplace "mktA" has a version dir; an ORPHAN lives under + // unregistered "mktB". Pre-fix, the `if dirs.is_empty()` gate skipped the + // glob (dirs already had mktA), so mktB's orphan was never cleaned. + let dir = tempdir().unwrap(); + let cache_dir = dir.path().join("cache"); + let registered = cache_dir.join("mktA/onebrain/3.2.0"); + let orphan = cache_dir.join("mktB/onebrain/2.2.4"); + fs::create_dir_all(®istered).unwrap(); + fs::create_dir_all(&orphan).unwrap(); + let installed = dir.path().join("installed_plugins.json"); + write_json( + &installed, + &serde_json::json!({ "plugins": { "onebrain@mktA": [{"id":"onebrain"}] } }), + ); + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!( + out.removed, 2, + "both registered + orphan version dirs removed" + ); + assert!(!registered.exists()); + assert!( + !orphan.exists(), + "orphan under unregistered marketplace must be cleaned" + ); + } + + #[test] + #[cfg(unix)] + fn remove_failure_is_counted_and_surfaced_not_swallowed() { + use std::os::unix::fs::PermissionsExt; + let dir = tempdir().unwrap(); + let cache_dir = dir.path().join("cache"); + let onebrain = cache_dir.join("mkt/onebrain"); + let version = onebrain.join("9.9.9"); + fs::create_dir_all(&version).unwrap(); + let installed = dir.path().join("installed_plugins.json"); + write_json( + &installed, + &serde_json::json!({ "plugins": { "onebrain@mkt": [{"id":"onebrain"}] } }), + ); + // Make the parent `onebrain/` non-writable so removing the child dir fails + // (can't unlink within a read-only directory on Unix). + fs::set_permissions(&onebrain, fs::Permissions::from_mode(0o500)).unwrap(); + + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!( + out.removed, 0, + "removal failed → nothing counted as removed" + ); + assert_eq!( + out.failed, 1, + "the failure is COUNTED, not silently swallowed" + ); + + // Restore perms so tempdir cleanup succeeds. + fs::set_permissions(&onebrain, fs::Permissions::from_mode(0o700)).unwrap(); + } + #[test] fn symlinked_version_dir_is_skipped() { // A symlink under `.../onebrain/` is classified as a symlink (not a @@ -394,8 +486,8 @@ mod tests { ); assert!(detect_stale_plugin_cache(&installed, Some(&cache_dir)).is_empty()); - let removed = clean_plugin_cache(&installed, Some(&cache_dir)); - assert_eq!(removed, 0); + let out = clean_plugin_cache(&installed, Some(&cache_dir)); + assert_eq!(out.removed, 0); // The symlink target's contents must be untouched. assert!(real_target.join("keep").exists()); } diff --git a/crates/onebrain-fs/src/vault_sync/orchestrate.rs b/crates/onebrain-fs/src/vault_sync/orchestrate.rs index 8e144c2..d5ec86b 100644 --- a/crates/onebrain-fs/src/vault_sync/orchestrate.rs +++ b/crates/onebrain-fs/src/vault_sync/orchestrate.rs @@ -175,16 +175,26 @@ pub fn run_vault_sync(vault_root: &Path, opts: VaultSyncOptions) -> VaultSyncRes } } + // Unconditional on every real Claude update (NOT gated on a version + // change): a no-op/up-to-date sync still sweeps stale cache version + // dirs. Only `--dry-run` skips this (it short-circuits the whole + // vault-sync earlier). Verified 2026-05-30 — closes item 1 of the + // plugin-update-robustness reopen. progress.start("🧹", "Cleaning cache"); - let cache_removed = clean_plugin_cache( + let outcome = clean_plugin_cache( &installed_plugins_path, installed_plugins_cache_dir.as_deref(), ); - result.cache_removed = cache_removed; - if cache_removed > 0 { + result.cache_removed = outcome.removed; + if outcome.removed > 0 || outcome.failed > 0 { progress.stop(&format!( - "{cache_removed} cached version{} removed", - if cache_removed == 1 { "" } else { "s" } + "{} removed{}", + outcome.removed, + if outcome.failed > 0 { + format!(", {} failed (see warnings)", outcome.failed) + } else { + String::new() + } )); } else { progress.stop("no cache to clean");