diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e527ca..d018669 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ lockstep and do not carry separate narrative changelogs. ## [Unreleased] +### Changed + +- `wash` profile cache now reloads mid-session when the profile file changes, + so adaptive defaults pick up new writes without a CLI restart. A broken + per-repo profile falls through to the global profile instead of masking it. + ### Fixed - `relaywash__GhPR`: `comments` op resolves `owner/repo` from the git remote diff --git a/crates/wash/src/profile.rs b/crates/wash/src/profile.rs index aac4d7a..48c8717 100644 --- a/crates/wash/src/profile.rs +++ b/crates/wash/src/profile.rs @@ -14,8 +14,9 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; -use std::path::PathBuf; -use std::sync::OnceLock; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, RwLock}; +use std::time::SystemTime; const DEFAULT_HOME: &str = ".relayburn"; @@ -75,31 +76,80 @@ pub struct HookSettings { pub edit_batching_nudge: Option, } -/// Process-cached active profile. Loaded lazily on first access. -fn cached() -> &'static OnceLock { - static C: OnceLock = OnceLock::new(); +/// Cache entry: the loaded `Profile` plus the cache key that produced it. The key is +/// the full priority-ordered list of candidate paths we consult and each candidate's +/// mtime at load time. Tracking every candidate (not just the chosen one) means +/// state changes anywhere in the chain — a previously-missing higher-priority file +/// appearing, a broken file getting rewritten — invalidate the cache. +#[derive(Clone)] +struct CacheEntry { + profile: Arc, + candidates: Vec, + mtimes: Vec>, +} + +/// Process-level mtime-watched cache. The MCP server lives for the whole session, so a +/// profile written mid-session (e.g., by the future `wash learn aggregate` aggregator) +/// must be picked up without restart. We pay one `metadata()` syscall per candidate +/// per access and only re-parse JSON when something changes. +fn cached() -> &'static RwLock> { + static C: RwLock> = RwLock::new(None); &C } -/// Resolve the active profile for the current process. Reads `RELAYWASH_PROFILE_PATH` -/// if set (test/override seam), otherwise derives the per-repo path from CWD's git -/// remote, otherwise the global profile, otherwise empty defaults. -pub fn get() -> &'static Profile { - cached().get_or_init(load_active) +/// Resolve the active profile for the current process. Walks the candidate list in +/// priority order — `RELAYWASH_PROFILE_PATH` if set, else per-repo, else global — and +/// takes the first file that *successfully loads*. A file that exists but fails to +/// parse is skipped so a broken per-repo profile does not silently mask a valid global +/// one. The empty default is returned when nothing loads. +/// +/// We cache against the full candidate list plus each candidate's current mtime, so any +/// mtime change (including missing → present transitions) invalidates the cache. +pub fn get() -> Arc { + let candidates = candidate_paths(); + let mtimes: Vec> = candidates.iter().map(|p| mtime_of(p)).collect(); + + { + let guard = cached().read().expect("profile cache poisoned"); + if let Some(entry) = guard.as_ref() { + if entry.candidates == candidates && entry.mtimes == mtimes { + return entry.profile.clone(); + } + } + } + + let profile = Arc::new( + candidates + .iter() + .find_map(|p| load_from(p)) + .unwrap_or_default(), + ); + + let new_entry = CacheEntry { + profile: profile.clone(), + candidates, + mtimes, + }; + *cached().write().expect("profile cache poisoned") = Some(new_entry); + profile } -fn load_active() -> Profile { +/// Build the prioritized list of candidate profile paths. The env override short- +/// circuits the fallback chain so tests and explicit users can target one specific file. +fn candidate_paths() -> Vec { if let Ok(path) = std::env::var("RELAYWASH_PROFILE_PATH") { - return load_from(std::path::Path::new(&path)).unwrap_or_default(); + return vec![PathBuf::from(path)]; } let home = ledger_home(); let key = current_repo_key(); - let per_repo = home.join("profiles").join(format!("{key}.json")); - if let Some(p) = load_from(&per_repo) { - return p; - } - let global = home.join("profiles").join("_global.json"); - load_from(&global).unwrap_or_default() + vec![ + home.join("profiles").join(format!("{key}.json")), + home.join("profiles").join("_global.json"), + ] +} + +fn mtime_of(path: &Path) -> Option { + std::fs::metadata(path).ok()?.modified().ok() } fn load_from(path: &std::path::Path) -> Option { @@ -180,8 +230,34 @@ pub fn pick_fields<'a>(src: &'a Value, allow: &[&str]) -> Value { #[cfg(test)] mod tests { use super::*; + use std::sync::Mutex; use tempfile::TempDir; + /// `RELAYWASH_PROFILE_PATH`, `RELAYBURN_HOME`, and `cached()` are all process-wide, + /// so tests that exercise `get()` must run serially to avoid clobbering each other. + static GET_SERIAL: Mutex<()> = Mutex::new(()); + + /// Clear the process-level cache so each `get()` test starts from a clean slate. + fn reset_cache() { + *cached().write().unwrap() = None; + } + + /// RAII helper: clears the env vars `get()` reads and the cache on drop, so a test + /// that panics mid-flight cannot leak global state into a later test sharing + /// `GET_SERIAL`. Used at the top of every test that mutates these globals. + struct GetTestCleanup; + impl Drop for GetTestCleanup { + fn drop(&mut self) { + // SAFETY: `GET_SERIAL` is held for the duration of the test, so no other + // thread is touching these env vars while we clear them. + unsafe { + std::env::remove_var("RELAYWASH_PROFILE_PATH"); + std::env::remove_var("RELAYBURN_HOME"); + } + reset_cache(); + } + } + #[test] fn default_profile_loads_when_file_missing() { let p: Profile = serde_json::from_str("{}").unwrap(); @@ -231,4 +307,126 @@ mod tests { assert_eq!(fnv1a(b"hello"), fnv1a(b"hello")); assert_ne!(fnv1a(b"hello"), fnv1a(b"world")); } + + /// Issue #24: a profile rewritten mid-session must be re-read on the next `get()`, + /// not stuck on whatever was cached at process start. + #[test] + fn get_reloads_when_profile_file_mtime_changes() { + let _g = GET_SERIAL.lock().unwrap_or_else(|e| e.into_inner()); + let _cleanup = GetTestCleanup; + reset_cache(); + + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("p.json"); + + // v1 + std::fs::write( + &path, + r#"{"version":1,"tools":{"search":{"maxResults":10}}}"#, + ) + .unwrap(); + // Backdate so the v2 write is guaranteed to advance mtime even on + // coarse-resolution filesystems. + let past = SystemTime::now() - std::time::Duration::from_secs(2); + set_mtime(&path, past); + + // SAFETY: tests under `GET_SERIAL` are serialized; no other thread touches this + // env var while the guard is held. + unsafe { + std::env::set_var("RELAYWASH_PROFILE_PATH", &path); + } + let p1 = get(); + assert_eq!(p1.tools.search.max_results, Some(10)); + + // v2 — overwrite and bump mtime to "now". + std::fs::write( + &path, + r#"{"version":1,"tools":{"search":{"maxResults":777}}}"#, + ) + .unwrap(); + set_mtime(&path, SystemTime::now()); + + let p2 = get(); + assert_eq!( + p2.tools.search.max_results, + Some(777), + "profile cache failed to reload after mtime advance" + ); + } + + /// Cheapness check: two `get()` calls with no filesystem change return the same + /// underlying `Arc`, proving we didn't re-parse JSON on the second call. + #[test] + fn get_returns_cached_instance_when_unchanged() { + let _g = GET_SERIAL.lock().unwrap_or_else(|e| e.into_inner()); + let _cleanup = GetTestCleanup; + reset_cache(); + + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("p.json"); + std::fs::write( + &path, + r#"{"version":1,"tools":{"search":{"maxResults":42}}}"#, + ) + .unwrap(); + + unsafe { + std::env::set_var("RELAYWASH_PROFILE_PATH", &path); + } + let a = get(); + let b = get(); + assert!( + Arc::ptr_eq(&a, &b), + "second get() should reuse the cached Arc when mtime is unchanged" + ); + assert_eq!(a.tools.search.max_results, Some(42)); + } + + /// Regression: a per-repo profile that exists but fails to parse must not mask the + /// global profile. The previous resolve-then-load split short-circuited on + /// `exists()` and silently returned the empty default when the per-repo file was + /// malformed; now the loader walks the candidate list and skips files that fail to + /// parse. + #[test] + fn get_falls_back_to_global_when_per_repo_is_unreadable() { + let _g = GET_SERIAL.lock().unwrap_or_else(|e| e.into_inner()); + let _cleanup = GetTestCleanup; + reset_cache(); + + let tmp = TempDir::new().unwrap(); + let profiles = tmp.path().join("profiles"); + std::fs::create_dir_all(&profiles).unwrap(); + + // SAFETY: `GET_SERIAL` serializes env-var access across tests. + unsafe { + std::env::set_var("RELAYBURN_HOME", tmp.path()); + // Make sure no leftover override from another test interferes. + std::env::remove_var("RELAYWASH_PROFILE_PATH"); + } + + // Resolve the per-repo key the same way `get()` will, so we can plant the + // malformed file at exactly the path that gets stat'd. + let key = current_repo_key(); + std::fs::write(profiles.join(format!("{key}.json")), "{ not valid json").unwrap(); + std::fs::write( + profiles.join("_global.json"), + r#"{"version":1,"tools":{"search":{"maxResults":999}}}"#, + ) + .unwrap(); + + let p = get(); + assert_eq!( + p.tools.search.max_results, + Some(999), + "broken per-repo profile should fall through to the global profile", + ); + } + + /// Helper: set a file's mtime. We avoid the `filetime` crate dep by writing the + /// timestamp via the platform `utimensat` shim through `std::fs::File::set_modified` + /// (stable since 1.75). + fn set_mtime(path: &std::path::Path, t: SystemTime) { + let f = std::fs::OpenOptions::new().write(true).open(path).unwrap(); + f.set_modified(t).unwrap(); + } } diff --git a/crates/wash/src/tools/read.rs b/crates/wash/src/tools/read.rs index 15352c8..1b31db4 100644 --- a/crates/wash/src/tools/read.rs +++ b/crates/wash/src/tools/read.rs @@ -113,7 +113,8 @@ fn run(args: &Value, ctx: &ToolContext) -> Result { } // signatures mode (default) - let prof = &profile::get().tools.read; + let active_profile = profile::get(); + let prof = &active_profile.tools.read; let small_file_lines = prof.small_file_lines.unwrap_or(DEFAULT_SMALL_FILE_LINES); let lines: Vec<&str> = text.split('\n').collect(); if lines.len() <= small_file_lines { diff --git a/crates/wash/src/tools/search.rs b/crates/wash/src/tools/search.rs index 4bd3722..03f5931 100644 --- a/crates/wash/src/tools/search.rs +++ b/crates/wash/src/tools/search.rs @@ -48,7 +48,8 @@ fn run(args: &Value) -> Result { // Profile-aware defaults: when the agent omits an arg, fall back to the active // per-repo profile if it has a value, else the static default. The tool *schema* // never changes (cache safety — see src/profile.rs). - let prof = &profile::get().tools.search; + let active_profile = profile::get(); + let prof = &active_profile.tools.search; let max_results = args .get("maxResults") .and_then(|v| v.as_u64())