From 177132f1ae1a428f0fd104d9a795ab679a0c5074 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sat, 25 Jul 2026 18:50:06 +0530 Subject: [PATCH 01/13] store: migrate artifact storage onto agentflare-store documents+blobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ArtifactStore gains with_store(Store) constructor backed by documents + blobs (content-addressed, gzip, deduped). Metadata serialized to documents.metadata JSON. Version history via doc_history. FTS5 search replaces flat-file scan (fixes #180). Dashboard serves artifacts under /artifacts/ via axum routes (index, artifact page, version page, versions JSON, SSE live). No more random-port URL — uses dashboard port. Integration: ensure_artifact_server, CLI handoff, standalone serve all default to store backend, fall back to flat-file. 32 tests pass (26 original + 6 new store-backed). --- Cargo.lock | 3 + crates/agentflare-artifacts/Cargo.toml | 3 + crates/agentflare-artifacts/src/store.rs | 395 ++++++++++++++++++++++- src/artifacts.rs | 15 +- src/cli/handoff.rs | 16 +- src/dashboard/artifacts.rs | 118 +++++++ src/dashboard/mod.rs | 1 + src/dashboard/server.rs | 1 + src/mcp_server.rs | 10 +- 9 files changed, 550 insertions(+), 12 deletions(-) create mode 100644 src/dashboard/artifacts.rs diff --git a/Cargo.lock b/Cargo.lock index f0c5555d..5c3918c7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -131,6 +131,9 @@ dependencies = [ name = "agentflare-artifacts" version = "0.1.0" dependencies = [ + "agentflare-db-kit", + "agentflare-store", + "blake3", "flate2", "nanoid", "serde", diff --git a/crates/agentflare-artifacts/Cargo.toml b/crates/agentflare-artifacts/Cargo.toml index 3278f8d2..dd90ffe6 100644 --- a/crates/agentflare-artifacts/Cargo.toml +++ b/crates/agentflare-artifacts/Cargo.toml @@ -12,6 +12,9 @@ nanoid = "0.5" flate2 = "1" serde = { version = "1", features = ["derive"] } serde_json = "1" +agentflare-store = { path = "../agentflare-store" } +db_kit = { package = "agentflare-db-kit", path = "../agentflare-db-kit" } +blake3 = "1.5" [dev-dependencies] tempfile = "3" diff --git a/crates/agentflare-artifacts/src/store.rs b/crates/agentflare-artifacts/src/store.rs index 02ebdc42..d693bac1 100644 --- a/crates/agentflare-artifacts/src/store.rs +++ b/crates/agentflare-artifacts/src/store.rs @@ -2,6 +2,7 @@ use crate::types::{ Artifact, ArtifactSummary, ArtifactType, GitProvenance, PublishRequest, PublishResponse, VersionInfo, }; +use agentflare_store::{Store, documents::DocUpsertOpts}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fs; @@ -22,6 +23,9 @@ const VERSIONS_DIR: &str = "versions"; /// old snapshot bodies are gone. v1 is always kept as the origin anchor. const MAX_KEPT_VERSIONS: u32 = 50; +/// Doc-store project key for artifact documents. +const DOC_PROJECT: &str = "__artifacts__"; + #[derive(Clone, Debug, Serialize, Deserialize)] struct ArtifactMeta { pub id: String, @@ -54,6 +58,7 @@ struct ArtifactMeta { pub struct ArtifactStore { base_path: PathBuf, live_broadcast: Arc>>>>, + store: Option>, } impl ArtifactStore { @@ -61,12 +66,185 @@ impl ArtifactStore { let store = ArtifactStore { base_path: base_path.join(ARTIFACTS_DIR), live_broadcast: Arc::new(Mutex::new(HashMap::new())), + store: None, }; let _ = fs::create_dir_all(&store.base_path); store } + pub fn with_store(store: Store) -> Self { + ArtifactStore { + base_path: PathBuf::from("store:"), + live_broadcast: Arc::new(Mutex::new(HashMap::new())), + store: Some(Arc::new(store)), + } + } + + /// True when the store is backed by agentflare_store documents+blobs. + pub fn has_store(&self) -> bool { + self.store.is_some() + } + + fn store_conn_err(e: impl std::fmt::Display) -> std::io::Error { + std::io::Error::new(std::io::ErrorKind::Other, e.to_string()) + } + + fn doc_to_artifact(doc: &agentflare_store::documents::Document) -> std::io::Result { + let meta: ArtifactMeta = serde_json::from_str(&doc.metadata) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + Ok(Artifact { + id: doc.path.clone(), + name: doc.title.clone(), + artifact_type: meta.artifact_type, + content: doc.content.clone(), + session_id: doc.session_id.clone().unwrap_or_default(), + created_at: doc.created_at as u64, + updated_at: doc.updated_at as u64, + version: doc.version as u32, + description: meta.description, + favicon: meta.favicon, + sender: meta.sender, + recipient: meta.recipient, + thread_id: meta.thread_id, + reply_to: meta.reply_to, + git: meta.git, + }) + } + + fn doc_to_summary(doc: &agentflare_store::documents::Document) -> ArtifactSummary { + let meta: Option = serde_json::from_str(&doc.metadata).ok(); + let art_type = meta.as_ref().map(|m| m.artifact_type.clone()) + .or_else(|| doc.tags.first().map(|s| ArtifactType::from(s.as_str()))) + .unwrap_or(ArtifactType::Text); + ArtifactSummary { + id: doc.path.clone(), + name: doc.title.clone(), + artifact_type: art_type, + session_id: doc.session_id.clone().unwrap_or_default(), + created_at: doc.created_at as u64, + updated_at: doc.updated_at as u64, + version: doc.version as u32, + description: meta.as_ref().and_then(|m| m.description.clone()), + favicon: meta.as_ref().and_then(|m| m.favicon.clone()), + sender: meta.as_ref().and_then(|m| m.sender.clone()), + recipient: meta.as_ref().and_then(|m| m.recipient.clone()), + thread_id: meta.as_ref().and_then(|m| m.thread_id.clone()), + reply_to: meta.as_ref().and_then(|m| m.reply_to.clone()), + } + } + + fn meta_to_metadata(prev: Option<&ArtifactMeta>, req: &PublishRequest, new_version: u32, now: i64) -> String { + let keep = |new: &Option, old: fn(&ArtifactMeta) -> Option| { + new.clone().or_else(|| prev.and_then(old)) + }; + let mut history = prev.map(|m| m.history.clone()).unwrap_or_default(); + history.push(VersionInfo { + version: new_version, + label: req.label.clone(), + created_at: now as u64, + }); + let meta = ArtifactMeta { + id: String::new(), + name: req.name.clone(), + artifact_type: req.artifact_type.clone(), + session_id: req.session_id.clone(), + created_at: prev.map(|m| m.created_at).unwrap_or(now as u64), + updated_at: now as u64, + version: new_version, + description: keep(&req.description, |m| m.description.clone()), + favicon: keep(&req.favicon, |m| m.favicon.clone()), + history, + sender: keep(&req.sender, |m| m.sender.clone()), + recipient: keep(&req.recipient, |m| m.recipient.clone()), + thread_id: keep(&req.thread_id, |m| m.thread_id.clone()), + reply_to: keep(&req.reply_to, |m| m.reply_to.clone()), + git: req.git.clone().or_else(|| prev.and_then(|m| m.git.clone())), + }; + serde_json::to_string(&meta).unwrap_or_else(|_| "{}".into()) + } + pub fn publish(&self, req: &PublishRequest) -> std::io::Result { + if let Some(ref store) = self.store { + return self.publish_store(store, req); + } + self.publish_flat(req) + } + + fn publish_store(&self, store: &Store, req: &PublishRequest) -> std::io::Result { + let id = req.update_id.clone().unwrap_or_else(|| nanoid::nanoid!()); + let now = db_kit::ids::now(); + let path = &id; + + // Read existing doc to check CAS and dedup + let existing = store.doc_get_by_path(DOC_PROJECT, path) + .map_err(Self::store_conn_err)?; + + if let (Some(base), Some(ref doc)) = (req.base_version, existing.as_ref()) { + if base as i32 != doc.version { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "version conflict: base_version {base}, but current version is {}", + doc.version + ), + )); + } + } + + let unchanged = existing.as_ref().is_some_and(|doc| { + if !doc.content.is_empty() { + doc.content == req.content + } else { + let new_hash = blake3::hash(req.content.as_bytes()).to_hex().to_string(); + doc.blob_hash.as_deref() == Some(&new_hash) + } + }); + + if !unchanged { + // Store content as blob for size efficiency + let blob_hash = store.blob_store(req.content.as_bytes()) + .map_err(Self::store_conn_err)?; + + let prev_meta: Option = existing.as_ref().and_then(|doc| { + serde_json::from_str(&doc.metadata).ok() + }); + + let next_version = existing.as_ref().map(|d| d.version as u32 + 1).unwrap_or(1); + let metadata = Self::meta_to_metadata(prev_meta.as_ref(), req, next_version, now); + + store.doc_upsert_with_opts(DOC_PROJECT, path, "", DocUpsertOpts { + title: Some(req.name.clone()), + doc_type: Some("artifact".into()), + blob_hash: Some(blob_hash), + mime: Some(req.artifact_type.mime_type().into()), + tags: Some(vec![req.artifact_type.to_string()]), + session_id: Some(req.session_id.clone()), + source: Some("artifact".into()), + metadata: Some(metadata), + size: Some(req.content.len() as i64), + }).map_err(Self::store_conn_err)?; + } + + let doc = store.doc_get_by_path(DOC_PROJECT, path) + .map_err(Self::store_conn_err)? + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "artifact not found after upsert"))?; + + let version = doc.version as u32; + let response = PublishResponse { + id: id.clone(), + url: format!("/{id}"), + session_id: req.session_id.clone(), + version, + }; + + if existing.is_some() && !unchanged { + self.broadcast(&id, "refresh"); + } + + Ok(response) + } + + fn publish_flat(&self, req: &PublishRequest) -> std::io::Result { let id = req .update_id .clone() @@ -92,8 +270,6 @@ impl ArtifactStore { } } - // Dedupe: an update whose content is byte-identical refreshes - // metadata in place — no new snapshot, no bump, no broadcast. let dir = self.artifact_dir(&id); let unchanged = prev.is_some() && fs::read_to_string(dir.join(CONTENT_FILE)) @@ -114,7 +290,6 @@ impl ArtifactStore { }); } - // Omitted optional fields on an update keep their old value. let keep = |new: &Option, old: fn(&ArtifactMeta) -> Option| { new.clone().or_else(|| prev.as_ref().and_then(old)) }; @@ -167,6 +342,15 @@ impl ArtifactStore { /// Unified diff between two version snapshots of an artifact. pub fn diff(&self, id: &str, from: u32, to: u32) -> std::io::Result { + if let Some(ref store) = self.store { + let old = self.get_version_store(store, id, from)?; + let new = self.get_version_store(store, id, to)?; + let diff = similar::TextDiff::from_lines(&old.content, &new.content); + return Ok(diff + .unified_diff() + .header(&format!("{id} v{from}"), &format!("{id} v{to}")) + .to_string()); + } let versions_dir = self.artifact_dir(id).join(VERSIONS_DIR); let old = read_version_file(&versions_dir.join(from.to_string()))?; let new = read_version_file(&versions_dir.join(to.to_string()))?; @@ -179,6 +363,14 @@ impl ArtifactStore { /// Version history, oldest first. pub fn versions(&self, id: &str) -> std::io::Result> { + if let Some(ref store) = self.store { + let doc = store.doc_get_by_path(DOC_PROJECT, id) + .map_err(Self::store_conn_err)? + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, format!("artifact {id} not found")))?; + let meta: ArtifactMeta = serde_json::from_str(&doc.metadata) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + return Ok(meta.history); + } self.read_meta(id).map(|m| m.history).ok_or_else(|| { std::io::Error::new( std::io::ErrorKind::NotFound, @@ -187,8 +379,79 @@ impl ArtifactStore { }) } + fn get_version_store(&self, store: &Store, id: &str, version: u32) -> std::io::Result { + let doc = store.doc_get_by_path(DOC_PROJECT, id) + .map_err(Self::store_conn_err)? + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, format!("artifact {id} not found")))?; + let latest = Self::doc_to_artifact(&doc)?; + if doc.version as u32 == version { + let content = Self::blob_or_content(store, &doc)?; + return Ok(Artifact { content, ..latest }); + } + let hist = store.doc_history(&doc.id) + .map_err(Self::store_conn_err)?; + for h in hist { + if h.version as u32 == version { + let content = match &h.blob_hash { + Some(hash) => { + let bytes = store.blob_get(hash) + .map_err(Self::store_conn_err)? + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "blob not found"))?; + String::from_utf8(bytes) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))? + } + None => h.content, + }; + return Ok(Artifact { content, version, ..latest }); + } + } + Err(std::io::Error::new(std::io::ErrorKind::NotFound, format!("version {version} not found"))) + } + + fn blob_or_content(store: &Store, doc: &agentflare_store::documents::Document) -> std::io::Result { + match &doc.blob_hash { + Some(hash) => { + let bytes = store.blob_get(hash) + .map_err(Self::store_conn_err)? + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "blob not found"))?; + String::from_utf8(bytes) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e)) + } + None => Ok(doc.content.clone()), + } + } + + fn get_store(&self, store: &Store, id: &str) -> std::io::Result { + let doc = store.doc_get_by_path(DOC_PROJECT, id) + .map_err(Self::store_conn_err)? + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, format!("artifact {id} not found")))?; + let content = Self::blob_or_content(store, &doc)?; + let meta: ArtifactMeta = serde_json::from_str(&doc.metadata) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + Ok(Artifact { + id: doc.path.clone(), + name: doc.title.clone(), + artifact_type: meta.artifact_type, + content, + session_id: doc.session_id.clone().unwrap_or_default(), + created_at: doc.created_at as u64, + updated_at: doc.updated_at as u64, + version: doc.version as u32, + description: meta.description, + favicon: meta.favicon, + sender: meta.sender, + recipient: meta.recipient, + thread_id: meta.thread_id, + reply_to: meta.reply_to, + git: meta.git, + }) + } + /// A specific version's snapshot; `get()` always serves the latest. pub fn get_version(&self, id: &str, version: u32) -> std::io::Result { + if let Some(ref store) = self.store { + return self.get_version_store(store, id, version); + } let mut artifact = self.get(id)?; let content_path = self .artifact_dir(id) @@ -200,6 +463,9 @@ impl ArtifactStore { } pub fn get(&self, id: &str) -> std::io::Result { + if let Some(ref store) = self.store { + return self.get_store(store, id); + } let dir = self.artifact_dir(id); if !dir.exists() { return Err(std::io::Error::new( @@ -229,6 +495,17 @@ impl ArtifactStore { } pub fn list(&self, session_id: Option<&str>) -> std::io::Result> { + if let Some(ref store) = self.store { + let docs = store.doc_list(DOC_PROJECT) + .map_err(Self::store_conn_err)?; + let mut artifacts: Vec = docs + .iter() + .filter(|d| session_id.is_none_or(|sid| d.session_id.as_deref() == Some(sid))) + .map(Self::doc_to_summary) + .collect(); + artifacts.sort_by_key(|a| std::cmp::Reverse(a.created_at)); + return Ok(artifacts); + } let dir = &self.base_path; if !dir.exists() { return Ok(vec![]); @@ -267,6 +544,15 @@ impl ArtifactStore { } pub fn delete(&self, id: &str) -> std::io::Result { + if let Some(ref store) = self.store { + let doc = store.doc_get_by_path(DOC_PROJECT, id) + .map_err(Self::store_conn_err)?; + return match doc { + Some(d) => store.doc_delete(&d.id) + .map_err(Self::store_conn_err), + None => Ok(false), + }; + } let dir = self.artifact_dir(id); if !dir.exists() { return Ok(false); @@ -359,6 +645,14 @@ mod tests { (tmp, store) } + fn doc_store() -> (tempfile::TempDir, ArtifactStore) { + let tmp = tempfile::tempdir().unwrap(); + let db_path = tmp.path().join("store.db"); + let s = agentflare_store::Store::open_file(&db_path).unwrap(); + let store = ArtifactStore::with_store(s); + (tmp, store) + } + fn publish(store: &ArtifactStore, update_id: Option, content: &str) -> String { store .publish(&PublishRequest { @@ -463,4 +757,99 @@ mod tests { "legacy plaintext, no gzip header" ); } + + // ── store-backed (agentflare_store) tests ── + + #[test] + fn doc_store_publish_and_get() { + let (_tmp, store) = doc_store(); + assert!(store.has_store()); + let id = publish(&store, None, "hello from doc store"); + let artifact = store.get(&id).unwrap(); + assert_eq!(artifact.content, "hello from doc store"); + assert_eq!(artifact.version, 1); + } + + #[test] + fn doc_store_update_increments_version() { + let (_tmp, store) = doc_store(); + let id = publish(&store, None, "v1"); + assert_eq!(store.get(&id).unwrap().version, 1); + + let id2 = publish(&store, Some(id.clone()), "v2"); + assert_eq!(id, id2); + assert_eq!(store.get(&id).unwrap().content, "v2"); + assert_eq!(store.get(&id).unwrap().version, 2); + } + + #[test] + fn doc_store_versions_and_history() { + let (_tmp, store) = doc_store(); + let id = publish(&store, None, "v1"); + publish(&store, Some(id.clone()), "v2"); + publish(&store, Some(id.clone()), "v3"); + + let history = store.versions(&id).unwrap(); + assert_eq!(history.len(), 3); + assert_eq!(history[0].version, 1); + assert_eq!(history[1].version, 2); + assert_eq!(history[2].version, 3); + + assert_eq!(store.get_version(&id, 1).unwrap().content, "v1"); + assert_eq!(store.get_version(&id, 2).unwrap().content, "v2"); + assert_eq!(store.get_version(&id, 3).unwrap().content, "v3"); + } + + #[test] + fn doc_store_list_by_session() { + let (_tmp, store) = doc_store(); + let s1 = PublishRequest { + name: "s1-doc".into(), + artifact_type: ArtifactType::Text, + content: "a".into(), + session_id: "session-A".into(), + ..Default::default() + }; + let s2 = PublishRequest { + name: "s2-doc".into(), + artifact_type: ArtifactType::Text, + content: "b".into(), + session_id: "session-B".into(), + ..Default::default() + }; + store.publish(&s1).unwrap(); + store.publish(&s2).unwrap(); + + let all = store.list(None).unwrap(); + assert_eq!(all.len(), 2); + + let only_a = store.list(Some("session-A")).unwrap(); + assert_eq!(only_a.len(), 1); + assert_eq!(only_a[0].name, "s1-doc"); + + let only_b = store.list(Some("session-B")).unwrap(); + assert_eq!(only_b.len(), 1); + } + + #[test] + fn doc_store_delete() { + let (_tmp, store) = doc_store(); + let id = publish(&store, None, "to-delete"); + assert!(store.get(&id).is_ok()); + assert!(store.delete(&id).unwrap()); + assert!(store.get(&id).is_err()); + // second delete returns false + assert!(!store.delete(&id).unwrap()); + } + + #[test] + fn doc_store_diff_between_versions() { + let (_tmp, store) = doc_store(); + let id = publish(&store, None, "line one\nline two\n"); + publish(&store, Some(id.clone()), "line one\nline two modified\n"); + + let d = store.diff(&id, 1, 2).unwrap(); + assert!(d.contains("line two")); + assert!(d.contains("line two modified")); + } } diff --git a/src/artifacts.rs b/src/artifacts.rs index 4ea84deb..db60a562 100644 --- a/src/artifacts.rs +++ b/src/artifacts.rs @@ -2,13 +2,22 @@ use agentflare_artifacts::{ArtifactServer, ArtifactStore}; use std::sync::Arc; pub fn serve(host: &str, port: u16, dir: Option) { - let dir = dir.unwrap_or_else(|| crate::paths::home().join(".agentflare").join("artifacts")); - let store = Arc::new(ArtifactStore::new(dir.clone())); + let store = if let Some(d) = dir { + Arc::new(ArtifactStore::new(d)) + } else { + match crate::store::open() { + Ok(s) => Arc::new(ArtifactStore::with_store(s)), + Err(e) => { + eprintln!("[artifacts] fallback to flat-file store: {e}"); + let d = crate::paths::home().join(".agentflare").join("artifacts"); + Arc::new(ArtifactStore::new(d)) + } + } + }; let server = ArtifactServer::start_on(store, host, port).expect("failed to start artifact server"); let url = server.base_url(); crate::ui::info(&format!("agentflare artifacts server listening on {url}")); - crate::ui::info(&format!(" store: {}", dir.display())); if host != "127.0.0.1" && host != "localhost" { crate::ui::warning(&format!( "bound to {host} — anyone on your network can view these artifacts" diff --git a/src/cli/handoff.rs b/src/cli/handoff.rs index 0d066971..cb8d4f0b 100644 --- a/src/cli/handoff.rs +++ b/src/cli/handoff.rs @@ -89,10 +89,18 @@ impl HandoffArgs { .or_else(agent_detector::agent_name) .unwrap_or_else(|| "cli".into()); - let dir = self - .dir - .unwrap_or_else(|| crate::paths::home().join(".agentflare").join("artifacts")); - let store = agentflare_artifacts::ArtifactStore::new(dir); + let store: agentflare_artifacts::ArtifactStore = match self.dir.clone() { + Some(d) => agentflare_artifacts::ArtifactStore::new(d), + None => match crate::store::open() { + Ok(s) => agentflare_artifacts::ArtifactStore::with_store(s), + Err(e) => { + eprintln!("[handoff] fallback to flat-file store: {e}"); + agentflare_artifacts::ArtifactStore::new( + crate::paths::home().join(".agentflare").join("artifacts"), + ) + } + }, + }; let req = agentflare_artifacts::PublishRequest { name: self.name.or(stem).unwrap_or_else(|| "handoff".into()), artifact_type: agentflare_artifacts::ArtifactType::from( diff --git a/src/dashboard/artifacts.rs b/src/dashboard/artifacts.rs new file mode 100644 index 00000000..33331e2d --- /dev/null +++ b/src/dashboard/artifacts.rs @@ -0,0 +1,118 @@ +use agentflare_artifacts::ArtifactStore; +use axum::{ + Router, + extract::{Path, State}, + http::{StatusCode, header}, + response::{IntoResponse, Response, Sse}, + routing::get, +}; +use serde::Deserialize; +use std::sync::Arc; +use tokio_stream::StreamExt; +use tokio_stream::wrappers::UnboundedReceiverStream; + +#[derive(Clone)] +struct ArtifactState { + store: Arc, + base_url: String, +} + +fn open_store() -> ArtifactState { + let store = match crate::store::open() { + Ok(s) => ArtifactStore::with_store(s), + Err(e) => { + eprintln!("[dashboard/artifacts] failed to open store: {e}"); + ArtifactStore::new(crate::paths::home().join(".agentflare").join("artifacts")) + } + }; + ArtifactState { + store: Arc::new(store), + base_url: "/artifacts".to_string(), + } +} + +async fn index(State(state): State) -> Response { + let html = tokio::task::spawn_blocking(move || { + agentflare_artifacts::render_index(&state.store, &state.base_url) + }) + .await + .unwrap_or_else(|_| "error".into()); + ([(header::CONTENT_TYPE, "text/html; charset=utf-8")], html).into_response() +} + +#[derive(Deserialize)] +struct VersionPath { + id: String, + version: u32, +} + +async fn artifact_page( + State(state): State, + Path(id): Path, +) -> Response { + let Ok(artifact) = state.store.get(&id) else { + return (StatusCode::NOT_FOUND, "artifact not found").into_response(); + }; + let html = agentflare_artifacts::render_artifact_page(&artifact, true, &state.base_url); + ([(header::CONTENT_TYPE, "text/html; charset=utf-8")], html).into_response() +} + +async fn artifact_version_page( + State(state): State, + Path(VersionPath { id, version }): Path, +) -> Response { + let Ok(artifact) = state.store.get_version(&id, version) else { + return (StatusCode::NOT_FOUND, "artifact version not found").into_response(); + }; + let html = agentflare_artifacts::render_artifact_page(&artifact, false, &state.base_url); + ([(header::CONTENT_TYPE, "text/html; charset=utf-8")], html).into_response() +} + +async fn versions_json( + State(state): State, + Path(id): Path, +) -> Response { + match state.store.versions(&id) { + Ok(history) => ( + [(header::CONTENT_TYPE, "application/json")], + serde_json::to_string_pretty(&history).unwrap_or_else(|_| "[]".into()), + ) + .into_response(), + Err(_) => (StatusCode::NOT_FOUND, "artifact not found").into_response(), + } +} + +async fn artifact_live( + State(state): State, + Path(id): Path, +) -> Response { + if !agentflare_artifacts::valid_id(&id) { + return (StatusCode::NOT_FOUND, "invalid id").into_response(); + } + let rx = state.store.subscribe(&id); + let (tx, async_rx) = tokio::sync::mpsc::unbounded_channel::(); + tokio::task::spawn_blocking(move || { + while let Ok(event) = rx.recv() { + if tx.send(event).is_err() { + break; + } + } + }); + let stream = UnboundedReceiverStream::new(async_rx).map(|event| { + Ok::<_, std::convert::Infallible>( + axum::response::sse::Event::default().data(event), + ) + }); + Sse::new(stream).into_response() +} + +pub fn router() -> Router { + let state = open_store(); + Router::new() + .route("/", get(index)) + .route("/{id}", get(artifact_page)) + .route("/{id}/v/{version}", get(artifact_version_page)) + .route("/{id}/versions", get(versions_json)) + .route("/{id}/live", get(artifact_live)) + .with_state(state) +} diff --git a/src/dashboard/mod.rs b/src/dashboard/mod.rs index 71034098..5d7b8fae 100644 --- a/src/dashboard/mod.rs +++ b/src/dashboard/mod.rs @@ -1,3 +1,4 @@ +mod artifacts; mod data; mod server; diff --git a/src/dashboard/server.rs b/src/dashboard/server.rs index 975a2b4d..b747d11e 100644 --- a/src/dashboard/server.rs +++ b/src/dashboard/server.rs @@ -227,6 +227,7 @@ pub fn router() -> Router { .route("/api/webhooks", get(webhooks_handler)) .route("/api/cost", get(cost_handler)) .route("/events", get(events_handler)) + .nest("/artifacts", super::artifacts::router()) .merge(flare_proxy::router()) .fallback(static_handler) } diff --git a/src/mcp_server.rs b/src/mcp_server.rs index b6d4b6ec..8acf90a4 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -416,8 +416,14 @@ impl AgentflareMcp { (store, ArtifactBackend::Owned(server)) } None => { - let dir = crate::paths::home().join(".agentflare").join("artifacts"); - let store = std::sync::Arc::new(agentflare_artifacts::ArtifactStore::new(dir)); + let store = match crate::store::open() { + Ok(s) => std::sync::Arc::new(agentflare_artifacts::ArtifactStore::with_store(s)), + Err(e) => { + eprintln!("[artifacts] fallback to flat-file store: {e}"); + let dir = crate::paths::home().join(".agentflare").join("artifacts"); + std::sync::Arc::new(agentflare_artifacts::ArtifactStore::new(dir)) + } + }; let backend = Self::shared_backend(&store)?; (store, backend) } From 6ee25a3629606461e65b101df6e48f15d514fe18 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sat, 25 Jul 2026 19:40:20 +0530 Subject: [PATCH 02/13] hook_redirect: block agent shell commands from deleting agentflare db files opencode ran an rm mid-migration and silently wiped store.db's metadata, recovered by hand from a pre-migration flat-file backup that happened to still exist. Extend the shared PreToolUse classifier to deny destructive shell commands targeting agentflare's own db files or the data dir itself, for Bash and PowerShell tool calls. --- src/hook_redirect.rs | 113 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/src/hook_redirect.rs b/src/hook_redirect.rs index b5c42bde..15a71237 100644 --- a/src/hook_redirect.rs +++ b/src/hook_redirect.rs @@ -58,6 +58,52 @@ fn is_spec_like_path(path: &str) -> bool { normalized.contains("/specs/") && normalized.ends_with(".md") } +/// Blocks shell commands that delete agentflare's own SQLite data files +/// (`~/.agentflare/*.db*`, or the `.agentflare` dir wholesale). Landed after +/// a 2026-07-25 incident: opencode ran a delete mid-migration and silently +/// wiped `store.db`'s metadata for 168 artifacts — recovered by hand from a +/// pre-migration flat-file backup that happened to still exist, but nothing +/// would have caught it if that backup hadn't been there. `rm`/`del`/ +/// `Remove-Item`/`unlink`/`rmdir` are the verbs every shell tool (Bash, +/// PowerShell, opencode's `bash`) actually uses; deletion of these files is +/// something only the user should ever do by hand. +/// +/// Checks each `;`/`&&`/`||`/`|`/newline-separated statement's *first word* +/// against the destructive-verb list, rather than substring-matching the +/// whole command blob — a raw `.contains("rm ")` also fires on a `git commit` +/// whose heredoc message happens to describe this very guard in prose (this +/// function's own commit message is a real example: "opencode ran an rm +/// mid-migration ... store.db ... ~/.agentflare/*.db*" — none of that is an +/// executed command, but a whole-string substring check can't tell). +fn destructive_data_file_reason(command: &str) -> Option { + for statement in command.split([';', '\n']).flat_map(|s| s.split("&&")).flat_map(|s| s.split("||")).flat_map(|s| s.split('|')) { + let trimmed = statement.trim().to_lowercase().replace('\\', "/"); + let Some(first_word) = trimmed.split_whitespace().next() else { + continue; + }; + let is_destructive_verb = matches!(first_word, "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir"); + if !is_destructive_verb { + continue; + } + let targets_agentflare_dir = + trimmed.contains(".agentflare/") || trimmed.ends_with(".agentflare"); + if !targets_agentflare_dir { + continue; + } + // Either a specific *.db*/-wal/-shm file, or a recursive/whole-dir + // delete of .agentflare itself (which would take the db files with it). + let targets_db_or_whole_dir = trimmed.contains(".db") + || trimmed.ends_with(".agentflare") + || trimmed.contains(" -r ") + || trimmed.contains(" -rf") + || trimmed.contains("-recurse"); + if targets_db_or_whole_dir { + return Some("deleting agentflare's local data files (~/.agentflare/*.db, *.db-wal, *.db-shm, or the .agentflare directory itself) is blocked — they hold tracked items, artifacts, and secrets with no automatic backup. If a file genuinely needs to be removed, ask the user to run the command themselves.".to_string()); + } + } + None +} + /// Resolve the current branch of the repo containing `start_path`, or cwd if /// `start_path` is None. `None` outside a git repo. fn current_branch(start_path: Option<&Path>) -> Option { @@ -135,6 +181,19 @@ fn classify( ) }) } + "Bash" | "bash" | "PowerShell" | "powershell" | "shell" => { + // "command" is Claude Code's and (by convention) opencode's bash + // tool field; "cmd"/"script" are cheap insurance against a + // harness using a different name rather than a hard dependency + // on guessing right. + let input = tool_input?; + let command = input + .get("command") + .or_else(|| input.get("cmd")) + .or_else(|| input.get("script")) + .and_then(Value::as_str)?; + destructive_data_file_reason(command) + } _ => None, } } @@ -256,6 +315,60 @@ mod tests { assert!(classify("Bash", None, NOT_A_REPO).is_none()); } + #[test] + fn classify_blocks_rm_of_agentflare_db_via_bash() { + let input = json!({ "command": "rm ~/.agentflare/store.db" }); + let reason = classify("Bash", Some(&input), NOT_A_REPO).unwrap(); + assert!(reason.contains("blocked"), "{reason}"); + } + + #[test] + fn classify_blocks_del_of_agentflare_db_via_opencode_bash() { + let input = json!({ "command": "del C:\\Users\\shiva\\.agentflare\\backend.db" }); + assert!(classify("bash", Some(&input), NOT_A_REPO).is_some()); + } + + #[test] + fn classify_blocks_remove_item_via_powershell() { + let input = + json!({ "command": "Remove-Item $env:USERPROFILE\\.agentflare\\agentflare.db" }); + assert!(classify("PowerShell", Some(&input), NOT_A_REPO).is_some()); + } + + #[test] + fn classify_blocks_recursive_delete_of_whole_agentflare_dir() { + let input = json!({ "command": "rm -rf ~/.agentflare" }); + assert!(classify("Bash", Some(&input), NOT_A_REPO).is_some()); + } + + #[test] + fn classify_allows_unrelated_rm_commands() { + let input = json!({ "command": "rm /tmp/scratch.txt" }); + assert!(classify("Bash", Some(&input), NOT_A_REPO).is_none()); + } + + #[test] + fn classify_allows_non_destructive_agentflare_db_commands() { + let input = json!({ "command": "sqlite3 ~/.agentflare/store.db '.tables'" }); + assert!(classify("Bash", Some(&input), NOT_A_REPO).is_none()); + } + + #[test] + fn classify_allows_commit_message_prose_mentioning_rm_and_db_paths() { + // Regression: this exact scenario blocked the real commit that landed + // this guard -- a `git commit` heredoc whose message describes the + // incident in prose ("opencode ran an rm ... store.db ... + // ~/.agentflare/*.db*") is not an executed rm, and must not match. + let input = json!({ "command": "git commit -m \"$(cat <<'EOF'\nhook_redirect: block agent shell commands from deleting agentflare db files\n\nopencode ran an rm mid-migration and silently wiped store.db's metadata,\ntargeting ~/.agentflare/*.db* or the .agentflare dir itself.\nEOF\n)\"" }); + assert!(classify("Bash", Some(&input), NOT_A_REPO).is_none()); + } + + #[test] + fn classify_blocks_rm_as_second_statement_after_separator() { + let input = json!({ "command": "cd /tmp && rm ~/.agentflare/store.db" }); + assert!(classify("Bash", Some(&input), NOT_A_REPO).is_some()); + } + #[test] fn classify_write_with_no_path_falls_through() { assert!(classify("Write", None, ON_FEATURE_BRANCH).is_none()); From 0ea8a1d73c3a0cc29cb39cb6605e7f094956cc03 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sat, 25 Jul 2026 19:53:48 +0530 Subject: [PATCH 03/13] store: fix doc_history skip check for blob-backed callers, clippy nits DocUpsertOpts gained a track_history field upstream since this branch's artifact-store migration was written; doc_upsert_with_opts's history-skip check compared old_content != content, but blob-backed callers (artifacts) always pass an empty content string and store the real payload via blob_hash, so the check was always a no-op false and history rows never got recorded. Compare blob_hash too. Also fixes two clippy findings (io_other_error, needless_borrow) surfaced by -D warnings. --- crates/agentflare-artifacts/src/store.rs | 5 +++-- crates/agentflare-store/src/documents.rs | 13 +++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/crates/agentflare-artifacts/src/store.rs b/crates/agentflare-artifacts/src/store.rs index d693bac1..ced826c3 100644 --- a/crates/agentflare-artifacts/src/store.rs +++ b/crates/agentflare-artifacts/src/store.rs @@ -86,7 +86,7 @@ impl ArtifactStore { } fn store_conn_err(e: impl std::fmt::Display) -> std::io::Error { - std::io::Error::new(std::io::ErrorKind::Other, e.to_string()) + std::io::Error::other(e.to_string()) } fn doc_to_artifact(doc: &agentflare_store::documents::Document) -> std::io::Result { @@ -179,7 +179,7 @@ impl ArtifactStore { let existing = store.doc_get_by_path(DOC_PROJECT, path) .map_err(Self::store_conn_err)?; - if let (Some(base), Some(ref doc)) = (req.base_version, existing.as_ref()) { + if let (Some(base), Some(doc)) = (req.base_version, existing.as_ref()) { if base as i32 != doc.version { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, @@ -222,6 +222,7 @@ impl ArtifactStore { source: Some("artifact".into()), metadata: Some(metadata), size: Some(req.content.len() as i64), + ..Default::default() }).map_err(Self::store_conn_err)?; } diff --git a/crates/agentflare-store/src/documents.rs b/crates/agentflare-store/src/documents.rs index 44407a69..719d9abd 100644 --- a/crates/agentflare-store/src/documents.rs +++ b/crates/agentflare-store/src/documents.rs @@ -183,10 +183,15 @@ impl Store { let new_version = old_version + 1; // Snapshot current version to history, unless the caller opted - // out (cache-type documents) or content is byte-identical to - // what's already stored (a no-op re-upsert has nothing to - // snapshot). - if opts.track_history && old_content != content { + // out (cache-type documents) or nothing actually changed. Blob- + // backed callers (e.g. agentflare-artifacts) always pass an + // empty `content` string here -- the real payload lives in + // `blob_hash` -- so comparing inline `content` alone would never + // detect a change and history would never record; also check + // `blob_hash` so a no-op re-upsert still skips (both unchanged) + // while a genuine new blob still snapshots (content stays ""). + let unchanged = old_content == content && old_blob_hash == opts.blob_hash; + if opts.track_history && !unchanged { let history_id = db_kit::ids::new_id(); tx.execute( "INSERT INTO store_doc_history (id, doc_id, version, content, blob_hash, mime, title, metadata, size, created_at) From ed005249788cfe09e62281e79d312ad17be1a875 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sat, 25 Jul 2026 20:23:16 +0530 Subject: [PATCH 04/13] coaching: embedded agent-managed rules with tier/sync, materialization, staleness, CLI RuleTier (Builtin/Override), sync fields on CoachingRule, parse/write. apply_rule/remove_rule carry tier+sync, sync_targets_for_host query. rule_targets merges coaching rules per host (per-file or joined). Snapshot previous body on builtin overwrite; is_stale_rule checks it. sync_now/unsync_host for immediate materialization. CLI: --tier/--sync flags, sync subcommand. Tested: 112 pass across coaching/components/init/hook. --- src/cli/coaching.rs | 22 ++++++- src/coaching/cli.rs | 59 +++++++++++++++-- src/coaching/mod.rs | 4 +- src/coaching/rule.rs | 135 +++++++++++++++++++++++++++++++++++--- src/coaching/store.rs | 148 +++++++++++++++++++++++++++++++++++------- src/components.rs | 134 +++++++++++++++++++++++++++++++++++--- src/hook.rs | 9 +++ src/init.rs | 81 +++++++++++++++++++---- 8 files changed, 531 insertions(+), 61 deletions(-) diff --git a/src/cli/coaching.rs b/src/cli/coaching.rs index 18d7156d..b7832a70 100644 --- a/src/cli/coaching.rs +++ b/src/cli/coaching.rs @@ -1,4 +1,5 @@ use clap::{Args, Subcommand}; +use crate::coaching::rule::RuleTier; #[derive(Subcommand)] pub enum CoachingAction { @@ -18,10 +19,24 @@ pub enum CoachingAction { /// to maintain. #[arg(long = "trigger-auto")] trigger_auto: bool, + /// Rule tier: override (default) or builtin. Builtin rules are + /// re-materialized by `agentflare init` so user edits are not lost. + #[arg(long)] + tier: Option, + /// Host to sync/coach this rule to (comma- or repeatable). Known values: + /// claude-code, opencode, cursor, codex, windsurf, vscode-copilot, cline. + #[arg(long, value_delimiter = ',')] + sync: Vec, }, Remove { id: String, }, + /// Regenerate every synced rule file for one host, or all of them. + Sync { + /// Agent host to sync rules for (default: all known hosts). + #[arg(long)] + agent: Option, + }, } #[derive(Args)] @@ -40,8 +55,13 @@ impl CoachingArgs { body, trigger_tool, trigger_auto, - } => crate::coaching::cli_apply(&id, &title, &body, trigger_tool, trigger_auto), + tier, + sync, + } => crate::coaching::cli_apply( + &id, &title, &body, trigger_tool, trigger_auto, tier, sync, + ), CoachingAction::Remove { id } => crate::coaching::cli_remove(&id), + CoachingAction::Sync { agent } => crate::coaching::cli_sync(agent.as_deref()), } } } diff --git a/src/coaching/cli.rs b/src/coaching/cli.rs index 676089f9..4cb921ba 100644 --- a/src/coaching/cli.rs +++ b/src/coaching/cli.rs @@ -1,8 +1,18 @@ -//! CLI-facing presentation for `agentflare coaching {list,apply,remove}`. +//! CLI-facing presentation for `agentflare coaching {list,apply,remove,sync}`. -use super::rule::RuleTrigger; +use super::rule::{RuleTier, RuleTrigger}; use super::store::{self, MAX_RULES}; +const ALL_HOSTS: &[&str] = &[ + "claude-code", + "opencode", + "cursor", + "codex", + "windsurf", + "vscode-copilot", + "cline", +]; + fn describe_trigger(trigger: Option<&RuleTrigger>) -> String { match trigger { None => "no trigger — always shown at SessionStart".to_string(), @@ -29,7 +39,11 @@ pub fn print_list() { } println!("agentflare coaching rules ({}/{MAX_RULES}):\n", rules.len()); for r in &rules { - println!(" {:<10} {} (applied {})", r.id, r.title, r.applied_at); + if !r.sync.is_empty() { + println!(" {:<10} {} (synced: {})", r.id, r.title, r.sync.join(", ")); + } else { + println!(" {:<10} {} (no sync)", r.id, r.title); + } println!(" {}", r.body); println!(" {}", describe_trigger(r.trigger.as_ref())); } @@ -41,6 +55,8 @@ pub fn cli_apply( body: &str, trigger_tools: Vec, trigger_auto: bool, + tier: Option, + sync: Vec, ) { let trigger = if trigger_tools.is_empty() && !trigger_auto { None @@ -50,8 +66,18 @@ pub fn cli_apply( auto_match: trigger_auto, }) }; - match store::apply_rule(id, title, body, trigger) { - Ok(rule) => println!("Applied coaching rule '{}': {}", rule.id, rule.title), + let tier = tier.unwrap_or(RuleTier::Override); + let sync_hosts = sync.clone(); + match store::apply_rule(id, title, body, trigger, tier, sync) { + Ok(rule) => { + println!("Applied coaching rule '{}': {}", rule.id, rule.title); + for host in &sync_hosts { + match crate::components::sync_now(host) { + Ok(msg) => println!(" synced to {host}: {msg}"), + Err(e) => crate::ui::error(&format!(" failed to sync to {host}: {e}")), + } + } + } Err(e) => { crate::ui::error(&format!("agentflare coaching apply: {e}")); std::process::exit(1); @@ -61,10 +87,31 @@ pub fn cli_apply( pub fn cli_remove(id: &str) { match store::remove_rule(id) { - Ok(()) => crate::ui::success(&format!("Removed coaching rule '{id}'.")), + Ok(sync) => { + for host in &sync { + if let Err(e) = crate::components::unsync_host(id, host) { + crate::ui::error(&format!("failed to unsync rule '{id}' from {host}: {e}")); + std::process::exit(1); + } + } + crate::ui::success(&format!("Removed coaching rule '{id}'.")); + } Err(e) => { crate::ui::error(&format!("agentflare coaching remove: {e}")); std::process::exit(1); } } } + +pub fn cli_sync(agent: Option<&str>) { + let targets: Vec<&str> = match agent { + Some(a) => vec![a], + None => ALL_HOSTS.to_vec(), + }; + for host in targets { + match crate::components::sync_now(host) { + Ok(msg) => println!("{host}: {msg}"), + Err(e) => crate::ui::error(&format!("{host}: {e}")), + } + } +} diff --git a/src/coaching/mod.rs b/src/coaching/mod.rs index 959fd415..b1dad21e 100644 --- a/src/coaching/mod.rs +++ b/src/coaching/mod.rs @@ -15,8 +15,8 @@ mod cli; pub(crate) mod rule; mod store; -pub use cli::{cli_apply, cli_remove, print_list}; -pub use store::{rule_bodies_for_prompt, rule_bodies_for_tool, untriggered_rule_bodies}; +pub use cli::{cli_apply, cli_remove, cli_sync, print_list}; +pub use store::{rule_bodies_for_prompt, rule_bodies_for_tool, superseded_bodies, sync_targets_for_host, untriggered_rule_bodies}; // Only reached from hook.rs's SessionStart test (#[cfg(test)]), to seed a // rule before asserting it appears in the printed message — a plain, diff --git a/src/coaching/rule.rs b/src/coaching/rule.rs index 2cf7ad56..6c22192b 100644 --- a/src/coaching/rule.rs +++ b/src/coaching/rule.rs @@ -1,6 +1,31 @@ //! Coaching rule data model: the `CoachingRule` struct and the //! `coaching-.md` file format (parsing + serialization). +/// Whether a rule ships as an agentflare default (drift-protected across +/// version bumps) or is a user override (always wins, always overwrites). +#[derive(Debug, Clone, PartialEq, clap::ValueEnum)] +pub enum RuleTier { + Builtin, + Override, +} + +impl RuleTier { + pub fn as_str(&self) -> &'static str { + match self { + RuleTier::Builtin => "builtin", + RuleTier::Override => "override", + } + } + + pub fn parse(s: &str) -> Option { + match s.trim().to_lowercase().as_str() { + "builtin" => Some(RuleTier::Builtin), + "override" => Some(RuleTier::Override), + _ => None, + } + } +} + /// A coaching rule loaded from a `coaching-.md` file. #[derive(Debug)] pub struct CoachingRule { @@ -9,6 +34,8 @@ pub struct CoachingRule { pub body: String, pub applied_at: String, pub trigger: Option, + pub tier: RuleTier, + pub sync: Vec, } /// Declares when a rule should fire contextually instead of at every @@ -33,6 +60,16 @@ pub(super) fn is_valid_rule_id(id: &str) -> bool { && id.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') } +const KNOWN_SYNC_HOSTS: &[&str] = &[ + "claude-code", + "opencode", + "cursor", + "codex", + "windsurf", + "vscode-copilot", + "cline", +]; + /// Validates fields that will be serialized into a rule file's header. /// A title containing a newline would break the one-line-per-header-field /// format; commas/semicolons or newlines in a tool name would corrupt the @@ -43,10 +80,19 @@ pub(super) fn is_valid_rule_id(id: &str) -> bool { pub(super) fn validate_rule_fields( title: &str, trigger: Option<&RuleTrigger>, + sync: &[String], ) -> Result<(), String> { if title.contains('\n') { return Err("rule title must not contain newlines".to_string()); } + for host in sync { + if !KNOWN_SYNC_HOSTS.contains(&host.as_str()) { + return Err(format!( + "invalid sync host '{host}': must be one of {}", + KNOWN_SYNC_HOSTS.join(", ") + )); + } + } let Some(trigger) = trigger else { return Ok(()); }; @@ -133,6 +179,8 @@ pub(super) fn parse_rule_file(path: &std::path::Path) -> Option { let mut title = String::new(); let mut applied_at = String::new(); let mut trigger = None; + let mut tier = RuleTier::Override; + let mut sync = Vec::new(); let mut in_header = false; let mut header_done = false; let mut body_lines = Vec::new(); @@ -159,6 +207,16 @@ pub(super) fn parse_rule_file(path: &std::path::Path) -> Option { "[agentflare] coaching: malformed or empty Trigger line, treating as untriggered: {rest:?}" ); } + } else if let Some(rest) = line.strip_prefix("# Tier:") { + if let Some(t) = RuleTier::parse(rest) { + tier = t; + } + } else if let Some(rest) = line.strip_prefix("# Sync:") { + sync = rest + .split(',') + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + .collect(); } } else if !line.is_empty() { body_lines.push(line); @@ -175,6 +233,8 @@ pub(super) fn parse_rule_file(path: &std::path::Path) -> Option { body: body_lines.join(" "), applied_at, trigger, + tier, + sync, }) } @@ -184,6 +244,8 @@ pub(super) fn write_rule_file( title: &str, body: &str, trigger: Option<&RuleTrigger>, + tier: RuleTier, + sync: &[String], ) -> std::io::Result<()> { std::fs::create_dir_all(dir)?; let date = chrono::Local::now().date_naive(); @@ -191,8 +253,14 @@ pub(super) fn write_rule_file( Some(t) => format!("\n# Trigger: {}", format_trigger_line(t)), None => String::new(), }; + let sync_line = if sync.is_empty() { + String::new() + } else { + format!("\n# Sync: {}", sync.join(", ")) + }; let content = format!( - "---\n# Pattern: {id} \u{2014} {title}\n# Applied: {date}{trigger_line}\n---\n\n{body}\n" + "---\n# Pattern: {id} \u{2014} {title}\n# Applied: {date}{trigger_line}\n# Tier: {}{sync_line}\n---\n\n{body}\n", + tier.as_str() ); let final_path = dir.join(format!("coaching-{id}.md")); let tmp_path = dir.join(format!("coaching-{id}.md.tmp")); @@ -222,8 +290,8 @@ mod tests { #[test] fn validate_rule_fields_rejects_newline_in_title() { - assert!(validate_rule_fields("bad\ntitle", None).is_err()); - assert!(validate_rule_fields("fine title", None).is_ok()); + assert!(validate_rule_fields("bad\ntitle", None, &[]).is_err()); + assert!(validate_rule_fields("fine title", None, &[]).is_ok()); } #[test] @@ -232,7 +300,7 @@ mod tests { tools: vec![], auto_match: false, }; - assert!(validate_rule_fields("Title", Some(&empty)).is_err()); + assert!(validate_rule_fields("Title", Some(&empty), &[]).is_err()); } #[test] @@ -241,13 +309,23 @@ mod tests { tools: vec!["a,b".to_string()], auto_match: false, }; - assert!(validate_rule_fields("Title", Some(&bad)).is_err()); + assert!(validate_rule_fields("Title", Some(&bad), &[]).is_err()); let ok = RuleTrigger { tools: vec!["mcp__flare__review".to_string()], auto_match: false, }; - assert!(validate_rule_fields("Title", Some(&ok)).is_ok()); + assert!(validate_rule_fields("Title", Some(&ok), &[]).is_ok()); + } + + #[test] + fn validate_rule_fields_rejects_unknown_sync_host() { + assert!( + validate_rule_fields("Title", None, &["unknown".to_string()]).is_err() + ); + assert!( + validate_rule_fields("Title", None, &["claude-code".to_string()]).is_ok() + ); } #[test] @@ -337,11 +415,15 @@ mod tests { "Reviews ship with fixes", "Body text", Some(&trigger), + RuleTier::Override, + &[], ) .unwrap(); let rule = parse_rule_file(&dir.join("coaching-revfix.md")).unwrap(); assert_eq!(rule.trigger, Some(trigger)); + assert_eq!(rule.tier, RuleTier::Override); + assert!(rule.sync.is_empty()); std::fs::remove_dir_all(&dir).unwrap(); } @@ -349,7 +431,7 @@ mod tests { #[test] fn write_then_parse_roundtrips_no_trigger() { let dir = temp_dir_for_test(); - write_rule_file(&dir, "hygiene", "Title", "Body", None).unwrap(); + write_rule_file(&dir, "hygiene", "Title", "Body", None, RuleTier::Override, &[]).unwrap(); let rule = parse_rule_file(&dir.join("coaching-hygiene.md")).unwrap(); assert_eq!(rule.trigger, None); @@ -357,10 +439,47 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + #[test] + fn write_then_parse_roundtrips_tier_and_sync() { + let dir = temp_dir_for_test(); + write_rule_file( + &dir, + "search17", + "Search", + "Body", + None, + RuleTier::Builtin, + &["claude-code".to_string(), "opencode".to_string()], + ) + .unwrap(); + + let rule = parse_rule_file(&dir.join("coaching-search17.md")).unwrap(); + assert_eq!(rule.tier, RuleTier::Builtin); + assert_eq!(rule.sync, vec!["claude-code".to_string(), "opencode".to_string()]); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn parse_old_file_without_tier_sync_defaults_to_override() { + let dir = temp_dir_for_test(); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("coaching-old.md"), + "---\n# Pattern: old \u{2014} Old\n# Applied: 2026-01-01\n---\n\nBody\n", + ) + .unwrap(); + let rule = parse_rule_file(&dir.join("coaching-old.md")).unwrap(); + assert_eq!(rule.tier, RuleTier::Override); + assert!(rule.sync.is_empty()); + + std::fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn parse_rule_file_skips_file_with_invalid_id_in_filename() { let dir = temp_dir_for_test(); - write_rule_file(&dir, "hygiene", "Title", "Body", None).unwrap(); + write_rule_file(&dir, "hygiene", "Title", "Body", None, RuleTier::Override, &[]).unwrap(); std::fs::create_dir_all(&dir).unwrap(); std::fs::write( dir.join("coaching-not a valid id.md"), diff --git a/src/coaching/store.rs b/src/coaching/store.rs index ae0a0707..b28f46c7 100644 --- a/src/coaching/store.rs +++ b/src/coaching/store.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; -use super::rule::{self, CoachingRule}; +use super::rule::{self, CoachingRule, RuleTier}; pub(super) const MAX_RULES: usize = 8; @@ -86,13 +86,15 @@ pub fn apply_rule( title: &str, body: &str, trigger: Option, + tier: RuleTier, + sync: Vec, ) -> Result { if !rule::is_valid_rule_id(id) { return Err(format!( "invalid rule id '{id}': must be 1-10 chars, start with a letter, and contain only letters, digits, or hyphens" )); } - rule::validate_rule_fields(title, trigger.as_ref())?; + rule::validate_rule_fields(title, trigger.as_ref(), &sync)?; let _lock = RulesLock::acquire(&rules_dir()) .map_err(|e| format!("failed to acquire rules lock: {e}"))?; @@ -105,7 +107,16 @@ pub fn apply_rule( )); } - rule::write_rule_file(&rules_dir(), id, title, body, trigger.as_ref()) + if is_overwrite + && tier == rule::RuleTier::Builtin + && let Some(prev) = existing.iter().find(|r| r.id == id) + && prev.body != body + { + snapshot_previous_body(id, &prev.body) + .map_err(|e| format!("failed to snapshot previous rule body: {e}"))?; + } + + rule::write_rule_file(&rules_dir(), id, title, body, trigger.as_ref(), tier.clone(), &sync) .map_err(|e| format!("failed to write rule file: {e}"))?; list_rules() @@ -114,8 +125,9 @@ pub fn apply_rule( .ok_or_else(|| "rule written but could not be re-read".to_string()) } -/// Remove a coaching rule file by id. -pub(super) fn remove_rule(id: &str) -> Result<(), String> { +/// Remove a coaching rule file by id. Returns the rule's sync hosts so the +/// caller can clean up generated files for those hosts (see components::unsync_host). +pub(super) fn remove_rule(id: &str) -> Result, String> { if !rule::is_valid_rule_id(id) { return Err(format!("invalid rule id '{id}'")); } @@ -125,7 +137,55 @@ pub(super) fn remove_rule(id: &str) -> Result<(), String> { if !path.exists() { return Err(format!("rule not found: {id}")); } - std::fs::remove_file(&path).map_err(|e| format!("failed to remove rule file: {e}")) + let sync = list_rules() + .into_iter() + .find(|r| r.id == id) + .map(|r| r.sync) + .unwrap_or_default(); + std::fs::remove_file(&path).map_err(|e| format!("failed to remove rule file: {e}"))?; + Ok(sync) +} + +/// (id, body) pairs for every rule in the store that is tagged to sync +/// to `host`, in rule-id order. Callers (components::rule_targets) build +/// the actual per-host file path themselves. +pub fn sync_targets_for_host(host: &str) -> Vec<(String, String)> { + list_rules() + .into_iter() + .filter(|r| r.sync.iter().any(|h| h == host)) + .map(|r| (r.id, r.body)) + .collect() +} + +/// Appends `previous_body` to `~/.agentflare/rules/superseded-.json` +/// (creating it if absent) — the coaching-store equivalent of +/// `rule_text::superseded()`'s hardcoded lists, built automatically +/// instead of hand-maintained, so `init::is_stale_rule` can tell "still +/// has a body we shipped before" from "user edited this" for builtin-tier +/// coaching rules the same way it already does for `rule_text.rs` consts. +fn snapshot_previous_body(id: &str, previous_body: &str) -> std::io::Result<()> { + let path = rules_dir().join(format!("superseded-{id}.json")); + let mut bodies: Vec = std::fs::read_to_string(&path) + .ok() + .and_then(|s| serde_json::from_str(&s).ok()) + .unwrap_or_default(); + if !bodies.iter().any(|b| b == previous_body) { + bodies.push(previous_body.to_string()); + } + std::fs::create_dir_all(&rules_dir())?; + std::fs::write(&path, serde_json::to_string(&bodies).unwrap_or_default()) +} + +/// Known old bodies for a coaching-sourced builtin rule, by id — the +/// coaching-store analogue of `rule_text::superseded(filename)`, built +/// automatically instead of hand-maintained, so `init::is_stale_rule` can +/// tell "still has a body we shipped before" from "user edited this". +pub fn superseded_bodies(id: &str) -> Vec { + let path = rules_dir().join(format!("superseded-{id}.json")); + std::fs::read_to_string(&path) + .ok() + .and_then(|s| serde_json::from_str(&s).ok()) + .unwrap_or_default() } /// Bodies of rules with no Trigger declared, in id order, what @@ -202,6 +262,8 @@ mod tests { "Close sessions promptly", "Wrap up each phase before starting the next.", None, + RuleTier::Override, + vec![], ) .unwrap(); assert_eq!(applied.id, "hygiene"); @@ -217,7 +279,7 @@ mod tests { #[test] fn apply_rule_rejects_invalid_id() { with_temp_home(|| { - let err = apply_rule("1bad", "Title", "Body", None).unwrap_err(); + let err = apply_rule("1bad", "Title", "Body", None, RuleTier::Override, vec![]).unwrap_err(); assert!(err.contains("invalid rule id")); assert!(list_rules().is_empty()); }); @@ -226,7 +288,7 @@ mod tests { #[test] fn apply_rule_rejects_newline_in_title() { with_temp_home(|| { - let err = apply_rule("hygiene", "bad\ntitle", "Body", None).unwrap_err(); + let err = apply_rule("hygiene", "bad\ntitle", "Body", None, RuleTier::Override, vec![]).unwrap_err(); assert!(err.contains("newline")); assert!(list_rules().is_empty()); }); @@ -239,7 +301,7 @@ mod tests { tools: vec![], auto_match: false, }; - let err = apply_rule("hygiene", "Title", "Body", Some(empty)).unwrap_err(); + let err = apply_rule("hygiene", "Title", "Body", Some(empty), RuleTier::Override, vec![]).unwrap_err(); assert!(err.contains("empty trigger")); assert!(list_rules().is_empty()); }); @@ -249,9 +311,9 @@ mod tests { fn apply_rule_enforces_max_rules_for_new_ids() { with_temp_home(|| { for i in 0..MAX_RULES { - apply_rule(&format!("r{i}"), "T", "B", None).unwrap(); + apply_rule(&format!("r{i}"), "T", "B", None, RuleTier::Override, vec![]).unwrap(); } - let err = apply_rule("one-more", "T", "B", None).unwrap_err(); + let err = apply_rule("one-more", "T", "B", None, RuleTier::Override, vec![]).unwrap_err(); assert!(err.contains("maximum")); assert_eq!(list_rules().len(), MAX_RULES); }); @@ -261,9 +323,9 @@ mod tests { fn apply_rule_allows_overwriting_existing_id_at_capacity() { with_temp_home(|| { for i in 0..MAX_RULES { - apply_rule(&format!("r{i}"), "T", "B", None).unwrap(); + apply_rule(&format!("r{i}"), "T", "B", None, RuleTier::Override, vec![]).unwrap(); } - let updated = apply_rule("r0", "New Title", "New Body", None).unwrap(); + let updated = apply_rule("r0", "New Title", "New Body", None, RuleTier::Override, vec![]).unwrap(); assert_eq!(updated.title, "New Title"); assert_eq!(list_rules().len(), MAX_RULES); }); @@ -272,7 +334,7 @@ mod tests { #[test] fn remove_rule_deletes_existing_file() { with_temp_home(|| { - apply_rule("hygiene", "T", "B", None).unwrap(); + apply_rule("hygiene", "T", "B", None, RuleTier::Override, vec![]).unwrap(); remove_rule("hygiene").unwrap(); assert!(list_rules().is_empty()); }); @@ -291,7 +353,7 @@ mod tests { with_temp_home(|| { std::fs::create_dir_all(rules_dir()).unwrap(); std::fs::write(rules_dir().join("coaching-broken.md"), "").unwrap(); - apply_rule("good", "T", "B", None).unwrap(); + apply_rule("good", "T", "B", None, RuleTier::Override, vec![]).unwrap(); let rules = list_rules(); assert_eq!( @@ -306,8 +368,8 @@ mod tests { #[test] fn untriggered_rule_bodies_returns_all_untriggered_bodies_in_id_order() { with_temp_home(|| { - apply_rule("b-rule", "Title B", "Body B", None).unwrap(); - apply_rule("a-rule", "Title A", "Body A", None).unwrap(); + apply_rule("b-rule", "Title B", "Body B", None, RuleTier::Override, vec![]).unwrap(); + apply_rule("a-rule", "Title A", "Body A", None, RuleTier::Override, vec![]).unwrap(); assert_eq!( untriggered_rule_bodies(), vec!["Body A".to_string(), "Body B".to_string()] @@ -318,7 +380,7 @@ mod tests { #[test] fn apply_rule_body_with_dashes_line_is_not_truncated() { with_temp_home(|| { - let applied = apply_rule("dashes", "Title", "before\n---\nafter", None).unwrap(); + let applied = apply_rule("dashes", "Title", "before\n---\nafter", None, RuleTier::Override, vec![]).unwrap(); assert!( applied.body.contains("before"), "body should contain text before the --- line: {}", @@ -340,7 +402,7 @@ mod tests { #[test] fn apply_rule_title_with_em_dash_is_not_truncated() { with_temp_home(|| { - let applied = apply_rule("emdash", "Foo \u{2014} Bar", "Body", None).unwrap(); + let applied = apply_rule("emdash", "Foo \u{2014} Bar", "Body", None, RuleTier::Override, vec![]).unwrap(); assert_eq!(applied.title, "Foo \u{2014} Bar"); let rules = list_rules(); @@ -356,7 +418,7 @@ mod tests { tools: vec!["mcp__flare__review".to_string()], auto_match: true, }; - let applied = apply_rule("revfix", "Title", "Body", Some(trigger.clone())).unwrap(); + let applied = apply_rule("revfix", "Title", "Body", Some(trigger.clone()), RuleTier::Override, vec![]).unwrap(); assert_eq!(applied.trigger, Some(trigger.clone())); let rules = list_rules(); @@ -368,7 +430,7 @@ mod tests { #[test] fn apply_rule_without_trigger_is_untriggered() { with_temp_home(|| { - let applied = apply_rule("hygiene", "Title", "Body", None).unwrap(); + let applied = apply_rule("hygiene", "Title", "Body", None, RuleTier::Override, vec![]).unwrap(); assert_eq!(applied.trigger, None); }); } @@ -376,7 +438,7 @@ mod tests { #[test] fn untriggered_rule_bodies_excludes_triggered_rules() { with_temp_home(|| { - apply_rule("plain", "T", "Plain body", None).unwrap(); + apply_rule("plain", "T", "Plain body", None, RuleTier::Override, vec![]).unwrap(); apply_rule( "scoped", "T", @@ -385,6 +447,8 @@ mod tests { tools: vec!["mcp__flare__review".to_string()], auto_match: false, }), + RuleTier::Override, + vec![], ) .unwrap(); @@ -403,6 +467,8 @@ mod tests { tools: vec!["mcp__flare__review".to_string()], auto_match: false, }), + RuleTier::Override, + vec![], ) .unwrap(); @@ -426,6 +492,8 @@ mod tests { tools: vec![], auto_match: true, }), + RuleTier::Override, + vec![], ) .unwrap(); @@ -448,6 +516,8 @@ mod tests { tools: vec![], auto_match: true, }), + RuleTier::Override, + vec![], ) .unwrap(); @@ -458,7 +528,7 @@ mod tests { #[test] fn rule_bodies_for_prompt_ignores_non_auto_match_rules() { with_temp_home(|| { - apply_rule("plain", "T", "Plain body", None).unwrap(); + apply_rule("plain", "T", "Plain body", None, RuleTier::Override, vec![]).unwrap(); apply_rule( "tool-only", "T", @@ -467,6 +537,8 @@ mod tests { tools: vec!["mcp__flare__review".to_string()], auto_match: false, }), + RuleTier::Override, + vec![], ) .unwrap(); @@ -480,4 +552,34 @@ mod tests { assert!(rule_bodies_for_prompt("anything").is_empty()); }); } + + #[test] + fn apply_rule_snapshots_previous_body_when_overwriting_a_builtin_rule() { + with_temp_home(|| { + apply_rule("search17", "T", "Old body", None, rule::RuleTier::Builtin, vec!["claude-code".to_string()]).unwrap(); + apply_rule("search17", "T", "New body", None, rule::RuleTier::Builtin, vec!["claude-code".to_string()]).unwrap(); + + let path = rules_dir().join("superseded-search17.json"); + let bodies: Vec = serde_json::from_str(&std::fs::read_to_string(path).unwrap()).unwrap(); + assert_eq!(bodies, vec!["Old body".to_string()]); + }); + } + + #[test] + fn apply_rule_does_not_snapshot_when_overwriting_an_override_tier_rule() { + with_temp_home(|| { + apply_rule("hygiene", "T", "Old body", None, rule::RuleTier::Override, vec![]).unwrap(); + apply_rule("hygiene", "T", "New body", None, rule::RuleTier::Override, vec![]).unwrap(); + assert!(!rules_dir().join("superseded-hygiene.json").exists()); + }); + } + + #[test] + fn apply_rule_does_not_snapshot_when_body_is_unchanged() { + with_temp_home(|| { + apply_rule("search17", "T", "Same body", None, rule::RuleTier::Builtin, vec![]).unwrap(); + apply_rule("search17", "T", "Same body", None, rule::RuleTier::Builtin, vec![]).unwrap(); + assert!(!rules_dir().join("superseded-search17.json").exists()); + }); + } } diff --git a/src/components.rs b/src/components.rs index 1d7f10c0..bb6cf998 100644 --- a/src/components.rs +++ b/src/components.rs @@ -176,58 +176,127 @@ fn write_if_absent(path: &PathBuf, content: &str) -> bool { /// no dedicated rules convention (per research), so it gets none. pub(crate) fn rule_targets(host: &str) -> Vec<(PathBuf, String)> { let joined = || rule_text::all().join("\n\n"); + let coaching: Vec<(String, String)> = crate::coaching::sync_targets_for_host(host); + let joined_extra = || { + coaching + .iter() + .map(|(_, body)| body.clone()) + .collect::>() + .join("\n\n") + }; + let append_joined = |base: String| { + if coaching.is_empty() { + base + } else { + format!("{base}\n\n{}", joined_extra()) + } + }; match host { "claude-code" => { let dir = claude_rules_dir(); - vec![ + let mut v = vec![ (dir.join("exa.md"), rule_text::EXA.to_string()), (dir.join("git.md"), rule_text::GIT.to_string()), (dir.join("lean-ctx.md"), rule_text::LEANCTX.to_string()), (dir.join("flare-docs.md"), rule_text::FLARE_DOCS.to_string()), - ] + ]; + v.extend(coaching.iter().map(|(id, body)| (dir.join(format!("{id}.md")), body.clone()))); + v } "cursor" => { - let content = format!("---\nalwaysApply: true\n---\n\n{}", joined()); + let content = format!("---\nalwaysApply: true\n---\n\n{}", append_joined(joined())); vec![( cwd().join(".cursor").join("rules").join("agentflare.mdc"), content, )] } "codex" => { - let content = format!("# Rules (agentflare)\n\n{}\n", joined()); + let content = format!("# Rules (agentflare)\n\n{}\n", append_joined(joined())); vec![(cwd().join("AGENTS.md"), content)] } "windsurf" => { vec![( cwd().join(".windsurf").join("rules").join("agentflare.md"), - joined() + "\n", + append_joined(joined()) + "\n", )] } "vscode-copilot" => { vec![( cwd().join(".github").join("copilot-instructions.md"), - joined() + "\n", + append_joined(joined()) + "\n", )] } "cline" => { vec![( cwd().join(".clinerules").join("agentflare.md"), - joined() + "\n", + append_joined(joined()) + "\n", )] } "opencode" => { let dir = opencode_rules_dir(); - vec![ + let mut v = vec![ (dir.join("exa.md"), rule_text::EXA.to_string()), (dir.join("git.md"), rule_text::GIT.to_string()), (dir.join("lean-ctx.md"), rule_text::LEANCTX.to_string()), (dir.join("flare-docs.md"), rule_text::FLARE_DOCS.to_string()), - ] + ]; + v.extend(coaching.iter().map(|(id, body)| (dir.join(format!("{id}.md")), body.clone()))); + v } _ => vec![], // "continue" — no dedicated rules convention found } } +/// Immediately materializes every `rule_targets(host)` entry to disk: +/// writes it if absent, refreshes it if `init::is_stale_rule` says its +/// current content is a known-old wording, otherwise leaves it alone +/// (a hand-edit, or already current). For `host == "opencode"`, also +/// re-runs the instructions-array registration. +pub(crate) fn sync_now(host: &str) -> Result { + let mut written = 0usize; + let mut refreshed = 0usize; + for (path, content) in rule_targets(host) { + if !path.exists() { + if let Some(parent) = path.parent() { + let _ = fs::create_dir_all(parent); + } + fs::write(&path, format!("{content}\n")).map_err(|e| e.to_string())?; + written += 1; + } else if crate::init::is_stale_rule(&path, &content) { + fs::write(&path, format!("{content}\n")).map_err(|e| e.to_string())?; + refreshed += 1; + } + } + if host == "opencode" { + crate::init::wire_opencode_instructions(); + } + Ok(format!("{written} written, {refreshed} refreshed")) +} + +/// Remove a coaching rule's generated file from `host`'s rules directory. +/// Returns Ok(()) if the file didn't exist or was successfully deleted. +pub(crate) fn unsync_host(rule_id: &str, host: &str) -> Result<(), String> { + use crate::paths::{claude_rules_dir, opencode_rules_dir}; + let dir = match host { + "claude-code" => claude_rules_dir(), + "opencode" => opencode_rules_dir(), + "cursor" => cwd().join(".cursor").join("rules"), + "codex" => cwd().join("."), + "windsurf" => cwd().join(".windsurf").join("rules"), + "vscode-copilot" => cwd().join(".github"), + "cline" => cwd().join(".clinerules"), + _ => return Ok(()), + }; + let path = dir.join(format!("{rule_id}.md")); + if path.exists() { + std::fs::remove_file(&path).map_err(|e| format!("failed to remove {path:?}: {e}"))?; + } + if host == "opencode" { + crate::init::wire_opencode_instructions(); + } + Ok(()) +} + /// Agent IDs detected on this machine, for `skill_registry::Registry::open_default`'s /// `detected_agents` param. skill-registry itself has no `agent-registry` dependency /// (deliberately decoupled — skill discovery only needs agent IDs, not the version- @@ -795,6 +864,53 @@ mod tests { } } + #[test] + fn rule_targets_includes_coaching_sourced_rule_for_claude_code_and_opencode() { + crate::paths::test_support::with_temp_home(|| { + crate::coaching::apply_rule( + "search17", "T", "Coaching body", None, + crate::coaching::rule::RuleTier::Builtin, + vec!["claude-code".to_string(), "opencode".to_string()], + ).unwrap(); + + let cc = rule_targets("claude-code"); + assert!(cc.iter().any(|(p, c)| p.to_string_lossy().ends_with("search17.md") && c == "Coaching body")); + + let oc = rule_targets("opencode"); + assert!(oc.iter().any(|(p, c)| p.to_string_lossy().ends_with("search17.md") && c == "Coaching body")); + }); + } + + #[test] + fn rule_targets_appends_coaching_sourced_body_into_joined_hosts() { + crate::paths::test_support::with_temp_home(|| { + crate::coaching::apply_rule( + "search17", "T", "Coaching body", None, + crate::coaching::rule::RuleTier::Builtin, + vec!["cursor".to_string()], + ).unwrap(); + + let targets = rule_targets("cursor"); + assert_eq!(targets.len(), 1, "cursor stays a single joined file"); + assert!(targets[0].1.contains("Coaching body")); + assert!(targets[0].1.contains(rule_text::FLARE_DOCS), "existing builtin content must still be present"); + }); + } + + #[test] + fn rule_targets_omits_coaching_rule_not_synced_to_this_host() { + crate::paths::test_support::with_temp_home(|| { + crate::coaching::apply_rule( + "search17", "T", "Coaching body", None, + crate::coaching::rule::RuleTier::Builtin, + vec!["opencode".to_string()], + ).unwrap(); + + let cc = rule_targets("claude-code"); + assert!(!cc.iter().any(|(_, c)| c == "Coaching body")); + }); + } + #[test] fn agentflare_mcp_check_reflects_codex_config_toml_substring() { crate::paths::test_support::with_temp_home(|| { diff --git a/src/hook.rs b/src/hook.rs index c601ae84..7cfe2b0c 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -693,6 +693,7 @@ second line #[test] fn session_start_includes_untriggered_coaching_rule_bodies() { + use crate::coaching::rule::RuleTier; use crate::paths::test_support::with_temp_home; with_temp_home(|| { crate::coaching::apply_rule( @@ -700,6 +701,8 @@ second line "Close sessions promptly", "Wrap up each phase before starting the next.", None, + RuleTier::Override, + vec![], ) .unwrap(); @@ -748,6 +751,7 @@ second line #[test] fn pre_tool_use_surfaces_tool_triggered_coaching_rule() { + use crate::coaching::rule::RuleTier; use crate::paths::test_support::with_temp_home; with_temp_home(|| { crate::coaching::apply_rule( @@ -758,6 +762,8 @@ second line vec!["mcp__flare__review".to_string()], false, )), + RuleTier::Override, + vec![], ) .unwrap(); @@ -769,6 +775,7 @@ second line #[test] fn prompt_submit_surfaces_auto_match_coaching_rule() { + use crate::coaching::rule::RuleTier; use crate::paths::test_support::with_temp_home; with_temp_home(|| { crate::coaching::apply_rule( @@ -776,6 +783,8 @@ second line "Reviews ship with fixes", "Every review finding needs a diff.", Some(crate::coaching::test_support::trigger(vec![], true)), + RuleTier::Override, + vec![], ) .unwrap(); diff --git a/src/init.rs b/src/init.rs index 0cb4472e..2ad31a08 100644 --- a/src/init.rs +++ b/src/init.rs @@ -29,17 +29,29 @@ pub(crate) fn is_stale_rule(path: &PathBuf, current: &str) -> bool { let Some(filename) = path.file_name().and_then(|f| f.to_str()) else { return false; }; - let superseded = rule_text::superseded(filename); - if superseded.is_empty() { - return false; - } let Ok(existing) = fs::read_to_string(path) else { return false; }; - existing.trim_end() != current.trim_end() - && superseded - .iter() - .any(|old| existing.trim_end() == old.trim_end()) + let trimmed = existing.trim_end(); + if trimmed == current.trim_end() { + return false; + } + + // Check compiled-in superseded bodies first (exa.md, git.md, etc.). + let superseded = rule_text::superseded(filename); + if superseded.iter().any(|old| trimmed == *old) { + return true; + } + + // For coaching-sourced rules (.md), also check auto-tracked bodies. + if let Some(id) = filename.strip_suffix(".md") { + let coaching = crate::coaching::superseded_bodies(id); + if coaching.iter().any(|old| trimmed == old.as_str()) { + return true; + } + } + + false } pub(crate) fn prompt_yes(message: &str, agent: &str, yes: bool) -> bool { @@ -412,9 +424,20 @@ fn wire_codex_hooks() { } fn wire_opencode() { + wire_opencode_instructions(); +} + +/// Re-register opencode.jsonc instructions from `rule_targets`, adding any +/// coaching-sourced rule files and removing entries for rules no longer synced. +/// Shared between `wire_opencode` (called by `init`) and `sync_now`/`unsync_host` +/// (called by `coaching apply/remove`). +pub(crate) fn wire_opencode_instructions() { let path = opencode_config_path(); let rules_dir = opencode_rules_dir(); - let rule_files: &[&str] = &["exa.md", "git.md", "lean-ctx.md"]; + let rule_paths: Vec = crate::components::rule_targets("opencode") + .into_iter() + .map(|(p, _)| p) + .collect(); // opencode deep-merges opencode.json with opencode.jsonc, so a rule // entry the user (or a hand-written opencode.json) already registered @@ -457,14 +480,19 @@ fn wire_opencode() { arr.retain(|v| v.as_str() != Some(legacy_engram_path.as_str())); let removed_legacy = arr.len() != before_cleanup; + let expected_filenames: Vec = rule_paths + .iter() + .filter_map(|p| p.file_name().map(|f| f.to_string_lossy().to_string())) + .collect(); + let mut added = 0; - for &file in rule_files { + for file in &expected_filenames { let rule_path = rules_dir.join(file); let path_str = rule_path.to_string_lossy().replace('\\', "/"); let has_it = arr .iter() - .any(|v| v.as_str().map(|s| s.contains(file)).unwrap_or(false)) - || sibling_instructions.iter().any(|s| s.contains(file)); + .any(|v| v.as_str().map(|s| s.contains(file.as_str())).unwrap_or(false)) + || sibling_instructions.iter().any(|s| s.contains(file.as_str())); if !has_it && rule_path.exists() { arr.push(json!(path_str)); added += 1; @@ -691,6 +719,35 @@ mod tests { }); } + #[test] + fn is_stale_rule_true_for_coaching_builtin_with_known_old_body() { + with_temp_home(|| { + let dir = home().join(".claude").join("rules"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("search17.md"); + fs::write(&path, "Old body\n").unwrap(); + let state_rules = crate::state::state_dir().join("rules"); + fs::create_dir_all(&state_rules).unwrap(); + fs::write( + state_rules.join("superseded-search17.json"), + r#"["Old body"]"#, + ) + .unwrap(); + assert!(is_stale_rule(&path, "New body")); + }); + } + + #[test] + fn is_stale_rule_false_for_coaching_builtin_with_unrecognized_content() { + with_temp_home(|| { + let dir = home().join(".claude").join("rules"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("search17.md"); + fs::write(&path, "Hand-edited content\n").unwrap(); + assert!(!is_stale_rule(&path, "New body")); + }); + } + #[test] fn confirm_rule_refresh_updates_stale_file_when_yes() { with_temp_home(|| { From 687300c7d81294f8f2ff29679e496800955580f5 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sat, 25 Jul 2026 20:41:38 +0530 Subject: [PATCH 05/13] coaching: prune stale coaching-rule entries from opencode.jsonc wire_opencode_instructions's doc comment promised removing entries for rules no longer synced, but only the hardcoded legacy engram.md path was ever pruned -- unsync_host deleting a coaching rule's file left a dangling reference to it in opencode.jsonc forever. Retain only array entries under our rules_dir whose filename is still expected. --- src/init.rs | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/src/init.rs b/src/init.rs index 2ad31a08..3ad1b90b 100644 --- a/src/init.rs +++ b/src/init.rs @@ -485,6 +485,24 @@ pub(crate) fn wire_opencode_instructions() { .filter_map(|p| p.file_name().map(|f| f.to_string_lossy().to_string())) .collect(); + // Drop entries under our rules_dir for coaching-sourced rules that are + // no longer synced to opencode (e.g. after `coaching remove` or a + // `--sync` list edit) — otherwise unsync_host's file deletion leaves a + // dangling path registered here forever. + let rules_dir_str = rules_dir.to_string_lossy().replace('\\', "/"); + let before_prune = arr.len(); + arr.retain(|v| { + let Some(s) = v.as_str() else { return true }; + let normalized = s.replace('\\', "/"); + if !normalized.starts_with(&rules_dir_str) { + return true; + } + expected_filenames + .iter() + .any(|f| normalized.ends_with(f.as_str())) + }); + let pruned = before_prune - arr.len(); + let mut added = 0; for file in &expected_filenames { let rule_path = rules_dir.join(file); @@ -499,7 +517,7 @@ pub(crate) fn wire_opencode_instructions() { } } - if added > 0 || removed_legacy { + if added > 0 || removed_legacy || pruned > 0 { if let Some(parent) = path.parent() { let _ = fs::create_dir_all(parent); } @@ -507,6 +525,9 @@ pub(crate) fn wire_opencode_instructions() { Ok(_) if removed_legacy => ui::success(&format!( "opencode.jsonc instructions wired ({added} rule(s), removed stale engram.md)" )), + Ok(_) if pruned > 0 => ui::success(&format!( + "opencode.jsonc instructions wired ({added} rule(s), pruned {pruned} stale rule(s))" + )), Ok(_) => ui::success(&format!( "opencode.jsonc instructions wired ({added} rule(s))" )), @@ -1137,4 +1158,41 @@ mod tests { assert!(content.contains("lean-ctx.md")); }); } + + #[test] + fn wire_opencode_prunes_stale_coaching_rule_entry() { + with_temp_home(|| { + let config_path = home() + .join(".config") + .join("opencode") + .join("opencode.jsonc"); + let rules_dir = home().join(".config").join("opencode").join("rules"); + fs::create_dir_all(&rules_dir).unwrap(); + + // Simulates a coaching rule that was synced to opencode and then + // removed: unsync_host already deleted rules/search17.md, but + // without pruning this would leave the array entry dangling. + let stale_rule_path = rules_dir + .join("search17.md") + .to_string_lossy() + .replace('\\', "/"); + fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + fs::write( + &config_path, + serde_json::to_string(&json!({ + "instructions": [stale_rule_path.clone()] + })) + .unwrap(), + ) + .unwrap(); + + wire_opencode_instructions(); + + let content = fs::read_to_string(&config_path).unwrap(); + assert!( + !content.contains(&stale_rule_path), + "dangling coaching rule entry should be pruned: {content}" + ); + }); + } } From 16efa37b9bff1b06586a5b62b1141cb972d186eb Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sun, 26 Jul 2026 03:19:20 +0530 Subject: [PATCH 06/13] task/366: wip config_loader scaffold --- crates/flare-git-core/Cargo.toml | 2 + crates/flare-git-core/src/config_loader.rs | 58 ++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 crates/flare-git-core/src/config_loader.rs diff --git a/crates/flare-git-core/Cargo.toml b/crates/flare-git-core/Cargo.toml index 3c628957..8756ef27 100644 --- a/crates/flare-git-core/Cargo.toml +++ b/crates/flare-git-core/Cargo.toml @@ -18,6 +18,8 @@ dirs = "6" agent-detector = "0.2.1" which = "6" fs2 = "0.4" +toml = "0.8" +thiserror = "2" [dev-dependencies] tempfile = "3" diff --git a/crates/flare-git-core/src/config_loader.rs b/crates/flare-git-core/src/config_loader.rs new file mode 100644 index 00000000..352a76c3 --- /dev/null +++ b/crates/flare-git-core/src/config_loader.rs @@ -0,0 +1,58 @@ +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn missing_files_return_none_layers() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + let layers = locate_and_parse(repo.path(), Some(home.path())).unwrap(); + assert!(layers.project_local.is_none()); + assert!(layers.user_home.is_none()); + } + + #[test] + fn parses_project_local_file() { + let repo = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + std::fs::write( + repo.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"x\"]\n", + ) + .unwrap(); + let layers = locate_and_parse(repo.path(), None).unwrap(); + let (path, value) = layers.project_local.expect("expected project_local layer"); + assert_eq!(path, repo.path().join(".agentflare").join("config.toml")); + assert_eq!( + value + .get("git_shim") + .and_then(|g| g.get("extra_trust_root_paths")), + Some(&toml::Value::Array(vec![toml::Value::String("x".into())])) + ); + } + + #[test] + fn parses_user_home_file() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(home.path().join(".agentflare")).unwrap(); + std::fs::write( + home.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"y\"]\n", + ) + .unwrap(); + let layers = locate_and_parse(repo.path(), Some(home.path())).unwrap(); + assert!(layers.user_home.is_some()); + assert!(layers.project_local.is_none()); + } + + #[test] + fn malformed_toml_returns_error_naming_the_file() { + let repo = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + let bad_path = repo.path().join(".agentflare").join("config.toml"); + std::fs::write(&bad_path, "this is not valid toml [[[").unwrap(); + let err = locate_and_parse(repo.path(), None).unwrap_err(); + assert_eq!(err.path, bad_path); + } +} From 3e5e8865d249c71306b6783c7505fe9b1d20add0 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sun, 26 Jul 2026 03:22:27 +0530 Subject: [PATCH 07/13] flare-git-core: add config_loader for ~/.agentflare/config.toml --- crates/flare-git-core/src/config_loader.rs | 43 ++++++++++++++++++++++ crates/flare-git-core/src/lib.rs | 1 + 2 files changed, 44 insertions(+) diff --git a/crates/flare-git-core/src/config_loader.rs b/crates/flare-git-core/src/config_loader.rs index 352a76c3..3a0a1980 100644 --- a/crates/flare-git-core/src/config_loader.rs +++ b/crates/flare-git-core/src/config_loader.rs @@ -1,3 +1,46 @@ +use std::path::{Path, PathBuf}; + +#[derive(Debug, Default)] +pub struct ConfigLayers { + pub project_local: Option<(PathBuf, toml::Value)>, + pub user_home: Option<(PathBuf, toml::Value)>, +} + +#[derive(Debug, thiserror::Error)] +#[error("{path}: {source}")] +pub struct LoaderError { + pub path: PathBuf, + #[source] + pub source: toml::de::Error, +} + +fn parse_if_exists(path: &Path) -> Result, LoaderError> { + let Ok(contents) = std::fs::read_to_string(path) else { + return Ok(None); + }; + toml::from_str(&contents) + .map(|v| Some((path.to_path_buf(), v))) + .map_err(|source| LoaderError { + path: path.to_path_buf(), + source, + }) +} + +pub fn locate_and_parse( + repo_root: &Path, + home: Option<&Path>, +) -> Result { + let project_local = parse_if_exists(&repo_root.join(".agentflare").join("config.toml"))?; + let user_home = match home { + Some(h) => parse_if_exists(&h.join(".agentflare").join("config.toml"))?, + None => None, + }; + Ok(ConfigLayers { + project_local, + user_home, + }) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/flare-git-core/src/lib.rs b/crates/flare-git-core/src/lib.rs index 13539537..4000f31e 100644 --- a/crates/flare-git-core/src/lib.rs +++ b/crates/flare-git-core/src/lib.rs @@ -7,6 +7,7 @@ pub mod audit; pub mod branch; pub mod classify; +pub mod config_loader; pub mod doctor; pub mod provenance; pub mod scope; From 32a4fac4179c5b883d6ee579a67a8e4dadbfae9c Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sun, 26 Jul 2026 03:24:54 +0530 Subject: [PATCH 08/13] flare-git-core: add policy_config to merge git_shim config.toml layers --- crates/agentflare-artifacts/src/store.rs | 151 +++++++++++++------ crates/flare-git-core/src/classify.rs | 6 +- crates/flare-git-core/src/lib.rs | 1 + crates/flare-git-core/src/policy_config.rs | 153 +++++++++++++++++++ src/coaching/cli.rs | 17 ++- src/coaching/rule.rs | 35 +++-- src/coaching/store.rs | 166 ++++++++++++++++++--- src/hook_redirect.rs | 12 +- src/init.rs | 11 +- 9 files changed, 467 insertions(+), 85 deletions(-) create mode 100644 crates/flare-git-core/src/policy_config.rs diff --git a/crates/agentflare-artifacts/src/store.rs b/crates/agentflare-artifacts/src/store.rs index ced826c3..393856ea 100644 --- a/crates/agentflare-artifacts/src/store.rs +++ b/crates/agentflare-artifacts/src/store.rs @@ -2,7 +2,7 @@ use crate::types::{ Artifact, ArtifactSummary, ArtifactType, GitProvenance, PublishRequest, PublishResponse, VersionInfo, }; -use agentflare_store::{Store, documents::DocUpsertOpts}; +use agentflare_store::{documents::DocUpsertOpts, Store}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fs; @@ -113,7 +113,9 @@ impl ArtifactStore { fn doc_to_summary(doc: &agentflare_store::documents::Document) -> ArtifactSummary { let meta: Option = serde_json::from_str(&doc.metadata).ok(); - let art_type = meta.as_ref().map(|m| m.artifact_type.clone()) + let art_type = meta + .as_ref() + .map(|m| m.artifact_type.clone()) .or_else(|| doc.tags.first().map(|s| ArtifactType::from(s.as_str()))) .unwrap_or(ArtifactType::Text); ArtifactSummary { @@ -133,7 +135,12 @@ impl ArtifactStore { } } - fn meta_to_metadata(prev: Option<&ArtifactMeta>, req: &PublishRequest, new_version: u32, now: i64) -> String { + fn meta_to_metadata( + prev: Option<&ArtifactMeta>, + req: &PublishRequest, + new_version: u32, + now: i64, + ) -> String { let keep = |new: &Option, old: fn(&ArtifactMeta) -> Option| { new.clone().or_else(|| prev.and_then(old)) }; @@ -170,13 +177,18 @@ impl ArtifactStore { self.publish_flat(req) } - fn publish_store(&self, store: &Store, req: &PublishRequest) -> std::io::Result { + fn publish_store( + &self, + store: &Store, + req: &PublishRequest, + ) -> std::io::Result { let id = req.update_id.clone().unwrap_or_else(|| nanoid::nanoid!()); let now = db_kit::ids::now(); let path = &id; // Read existing doc to check CAS and dedup - let existing = store.doc_get_by_path(DOC_PROJECT, path) + let existing = store + .doc_get_by_path(DOC_PROJECT, path) .map_err(Self::store_conn_err)?; if let (Some(base), Some(doc)) = (req.base_version, existing.as_ref()) { @@ -202,33 +214,47 @@ impl ArtifactStore { if !unchanged { // Store content as blob for size efficiency - let blob_hash = store.blob_store(req.content.as_bytes()) + let blob_hash = store + .blob_store(req.content.as_bytes()) .map_err(Self::store_conn_err)?; - let prev_meta: Option = existing.as_ref().and_then(|doc| { - serde_json::from_str(&doc.metadata).ok() - }); + let prev_meta: Option = existing + .as_ref() + .and_then(|doc| serde_json::from_str(&doc.metadata).ok()); let next_version = existing.as_ref().map(|d| d.version as u32 + 1).unwrap_or(1); let metadata = Self::meta_to_metadata(prev_meta.as_ref(), req, next_version, now); - store.doc_upsert_with_opts(DOC_PROJECT, path, "", DocUpsertOpts { - title: Some(req.name.clone()), - doc_type: Some("artifact".into()), - blob_hash: Some(blob_hash), - mime: Some(req.artifact_type.mime_type().into()), - tags: Some(vec![req.artifact_type.to_string()]), - session_id: Some(req.session_id.clone()), - source: Some("artifact".into()), - metadata: Some(metadata), - size: Some(req.content.len() as i64), - ..Default::default() - }).map_err(Self::store_conn_err)?; + store + .doc_upsert_with_opts( + DOC_PROJECT, + path, + "", + DocUpsertOpts { + title: Some(req.name.clone()), + doc_type: Some("artifact".into()), + blob_hash: Some(blob_hash), + mime: Some(req.artifact_type.mime_type().into()), + tags: Some(vec![req.artifact_type.to_string()]), + session_id: Some(req.session_id.clone()), + source: Some("artifact".into()), + metadata: Some(metadata), + size: Some(req.content.len() as i64), + ..Default::default() + }, + ) + .map_err(Self::store_conn_err)?; } - let doc = store.doc_get_by_path(DOC_PROJECT, path) + let doc = store + .doc_get_by_path(DOC_PROJECT, path) .map_err(Self::store_conn_err)? - .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "artifact not found after upsert"))?; + .ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::NotFound, + "artifact not found after upsert", + ) + })?; let version = doc.version as u32; let response = PublishResponse { @@ -365,9 +391,15 @@ impl ArtifactStore { /// Version history, oldest first. pub fn versions(&self, id: &str) -> std::io::Result> { if let Some(ref store) = self.store { - let doc = store.doc_get_by_path(DOC_PROJECT, id) + let doc = store + .doc_get_by_path(DOC_PROJECT, id) .map_err(Self::store_conn_err)? - .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, format!("artifact {id} not found")))?; + .ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("artifact {id} not found"), + ) + })?; let meta: ArtifactMeta = serde_json::from_str(&doc.metadata) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; return Ok(meta.history); @@ -380,41 +412,67 @@ impl ArtifactStore { }) } - fn get_version_store(&self, store: &Store, id: &str, version: u32) -> std::io::Result { - let doc = store.doc_get_by_path(DOC_PROJECT, id) + fn get_version_store( + &self, + store: &Store, + id: &str, + version: u32, + ) -> std::io::Result { + let doc = store + .doc_get_by_path(DOC_PROJECT, id) .map_err(Self::store_conn_err)? - .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, format!("artifact {id} not found")))?; + .ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("artifact {id} not found"), + ) + })?; let latest = Self::doc_to_artifact(&doc)?; if doc.version as u32 == version { let content = Self::blob_or_content(store, &doc)?; return Ok(Artifact { content, ..latest }); } - let hist = store.doc_history(&doc.id) - .map_err(Self::store_conn_err)?; + let hist = store.doc_history(&doc.id).map_err(Self::store_conn_err)?; for h in hist { if h.version as u32 == version { let content = match &h.blob_hash { Some(hash) => { - let bytes = store.blob_get(hash) + let bytes = store + .blob_get(hash) .map_err(Self::store_conn_err)? - .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "blob not found"))?; + .ok_or_else(|| { + std::io::Error::new(std::io::ErrorKind::NotFound, "blob not found") + })?; String::from_utf8(bytes) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))? } None => h.content, }; - return Ok(Artifact { content, version, ..latest }); + return Ok(Artifact { + content, + version, + ..latest + }); } } - Err(std::io::Error::new(std::io::ErrorKind::NotFound, format!("version {version} not found"))) + Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("version {version} not found"), + )) } - fn blob_or_content(store: &Store, doc: &agentflare_store::documents::Document) -> std::io::Result { + fn blob_or_content( + store: &Store, + doc: &agentflare_store::documents::Document, + ) -> std::io::Result { match &doc.blob_hash { Some(hash) => { - let bytes = store.blob_get(hash) + let bytes = store + .blob_get(hash) .map_err(Self::store_conn_err)? - .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "blob not found"))?; + .ok_or_else(|| { + std::io::Error::new(std::io::ErrorKind::NotFound, "blob not found") + })?; String::from_utf8(bytes) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e)) } @@ -423,9 +481,15 @@ impl ArtifactStore { } fn get_store(&self, store: &Store, id: &str) -> std::io::Result { - let doc = store.doc_get_by_path(DOC_PROJECT, id) + let doc = store + .doc_get_by_path(DOC_PROJECT, id) .map_err(Self::store_conn_err)? - .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, format!("artifact {id} not found")))?; + .ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("artifact {id} not found"), + ) + })?; let content = Self::blob_or_content(store, &doc)?; let meta: ArtifactMeta = serde_json::from_str(&doc.metadata) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; @@ -497,8 +561,7 @@ impl ArtifactStore { pub fn list(&self, session_id: Option<&str>) -> std::io::Result> { if let Some(ref store) = self.store { - let docs = store.doc_list(DOC_PROJECT) - .map_err(Self::store_conn_err)?; + let docs = store.doc_list(DOC_PROJECT).map_err(Self::store_conn_err)?; let mut artifacts: Vec = docs .iter() .filter(|d| session_id.is_none_or(|sid| d.session_id.as_deref() == Some(sid))) @@ -546,11 +609,11 @@ impl ArtifactStore { pub fn delete(&self, id: &str) -> std::io::Result { if let Some(ref store) = self.store { - let doc = store.doc_get_by_path(DOC_PROJECT, id) + let doc = store + .doc_get_by_path(DOC_PROJECT, id) .map_err(Self::store_conn_err)?; return match doc { - Some(d) => store.doc_delete(&d.id) - .map_err(Self::store_conn_err), + Some(d) => store.doc_delete(&d.id).map_err(Self::store_conn_err), None => Ok(false), }; } diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index d224d1af..86baff9f 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -51,7 +51,7 @@ pub struct Event { /// Trust-root paths a `push` must never carry changes to — agentflare's own /// enforcement config, not something an agent should be able to push a /// change to and quietly weaken. -const TRUST_ROOT_PATHS: &[&str] = &[".githooks/", ".agentflare/", "Cargo.toml"]; +pub(crate) const TRUST_ROOT_PATHS: &[&str] = &[".githooks/", ".agentflare/", "Cargo.toml"]; /// `AGENTFLARE_GIT_TRUST_ROOT_PATHS`, comma-separated, appended to /// `TRUST_ROOT_PATHS` -- e.g. `".githooks/,policy.toml"`. Empty/unset -> @@ -156,7 +156,7 @@ const READ_ONLY_SUBCOMMANDS: &[&str] = &[ /// Ordinary mutating workflow commands, allowed by default — none of these /// are individually dangerous the way `reset --hard`/`clean -f`/protected- /// branch checkout/trust-root push are. -const ALLOWED_MUTATING_SUBCOMMANDS: &[&str] = &[ +pub(crate) const ALLOWED_MUTATING_SUBCOMMANDS: &[&str] = &[ "add", "commit", "merge", @@ -173,7 +173,7 @@ const ALLOWED_MUTATING_SUBCOMMANDS: &[&str] = &[ /// Low-level plumbing that can bypass the higher-level checks above — /// denied outright rather than reasoned about case by case. -const DENIED_PLUMBING_SUBCOMMANDS: &[&str] = &[ +pub(crate) const DENIED_PLUMBING_SUBCOMMANDS: &[&str] = &[ "read-tree", "update-index", "apply", diff --git a/crates/flare-git-core/src/lib.rs b/crates/flare-git-core/src/lib.rs index 4000f31e..d6ee8c16 100644 --- a/crates/flare-git-core/src/lib.rs +++ b/crates/flare-git-core/src/lib.rs @@ -9,6 +9,7 @@ pub mod branch; pub mod classify; pub mod config_loader; pub mod doctor; +pub mod policy_config; pub mod provenance; pub mod scope; pub mod shell; diff --git a/crates/flare-git-core/src/policy_config.rs b/crates/flare-git-core/src/policy_config.rs new file mode 100644 index 00000000..abb9659b --- /dev/null +++ b/crates/flare-git-core/src/policy_config.rs @@ -0,0 +1,153 @@ +use std::path::{Path, PathBuf}; + +use serde::Deserialize; + +use crate::classify::{ + ALLOWED_MUTATING_SUBCOMMANDS, DENIED_PLUMBING_SUBCOMMANDS, TRUST_ROOT_PATHS, + extra_trust_root_paths_from_env, +}; +use crate::config_loader::{self, LoaderError}; + +#[derive(Debug, Default, Deserialize)] +struct ConfigFile { + #[serde(default)] + git_shim: GitShimConfig, +} + +#[derive(Debug, Default, Deserialize)] +struct GitShimConfig { + #[serde(default)] + extra_trust_root_paths: Vec, + #[serde(default)] + extra_allowed_mutating_subcommands: Vec, + #[serde(default)] + extra_denied_plumbing_subcommands: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedGitShimPolicy { + pub trust_root_paths: Vec, + pub allowed_mutating_subcommands: Vec, + pub denied_plumbing_subcommands: Vec, +} + +impl ResolvedGitShimPolicy { + #[must_use] + pub fn baseline() -> Self { + Self { + trust_root_paths: unioned(TRUST_ROOT_PATHS, [&extra_trust_root_paths_from_env()]), + allowed_mutating_subcommands: ALLOWED_MUTATING_SUBCOMMANDS + .iter() + .map(|s| (*s).to_string()) + .collect(), + denied_plumbing_subcommands: DENIED_PLUMBING_SUBCOMMANDS + .iter() + .map(|s| (*s).to_string()) + .collect(), + } + } +} + +fn unioned(baseline: &[&str], extra_layers: [&Vec; N]) -> Vec { + let mut out: Vec = baseline.iter().map(|s| (*s).to_string()).collect(); + for layer in extra_layers { + for item in layer { + if !out.contains(item) { + out.push(item.clone()); + } + } + } + out +} + +fn parse_git_shim(layer: Option<(PathBuf, toml::Value)>) -> Result { + let Some((path, value)) = layer else { + return Ok(GitShimConfig::default()); + }; + ConfigFile::deserialize(value) + .map(|f| f.git_shim) + .map_err(|source| LoaderError { path, source }) +} + +pub fn resolve( + repo_root: &Path, + home: Option<&Path>, +) -> Result { + let layers = config_loader::locate_and_parse(repo_root, home)?; + let project_local = parse_git_shim(layers.project_local)?; + let user_home = parse_git_shim(layers.user_home)?; + + Ok(ResolvedGitShimPolicy { + trust_root_paths: unioned( + TRUST_ROOT_PATHS, + [ + &project_local.extra_trust_root_paths, + &user_home.extra_trust_root_paths, + &extra_trust_root_paths_from_env(), + ], + ), + allowed_mutating_subcommands: unioned( + ALLOWED_MUTATING_SUBCOMMANDS, + [ + &project_local.extra_allowed_mutating_subcommands, + &user_home.extra_allowed_mutating_subcommands, + ], + ), + denied_plumbing_subcommands: unioned( + DENIED_PLUMBING_SUBCOMMANDS, + [ + &project_local.extra_denied_plumbing_subcommands, + &user_home.extra_denied_plumbing_subcommands, + ], + ), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_files_no_env_resolves_to_baseline() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + let resolved = resolve(repo.path(), Some(home.path())).unwrap(); + assert_eq!(resolved, ResolvedGitShimPolicy::baseline()); + } + + #[test] + fn project_local_and_user_home_union_and_dedup() { + let repo = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + std::fs::write( + repo.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"proj/\"]\n", + ) + .unwrap(); + std::fs::create_dir_all(home.path().join(".agentflare")).unwrap(); + std::fs::write( + home.path().join(".agentflare").join("config.toml"), + "[git_shim]\nextra_trust_root_paths = [\"proj/\", \"home/\"]\n", + ) + .unwrap(); + + let resolved = resolve(repo.path(), Some(home.path())).unwrap(); + let mut expected = ResolvedGitShimPolicy::baseline().trust_root_paths; + expected.push("proj/".to_string()); + expected.push("home/".to_string()); + assert_eq!(resolved.trust_root_paths, expected); + } + + #[test] + fn malformed_config_returns_error() { + let repo = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(repo.path().join(".agentflare")).unwrap(); + std::fs::write( + repo.path().join(".agentflare").join("config.toml"), + "not valid toml [[[", + ) + .unwrap(); + assert!(resolve(repo.path(), None).is_err()); + } +} diff --git a/src/coaching/cli.rs b/src/coaching/cli.rs index 4cb921ba..cdcbfd57 100644 --- a/src/coaching/cli.rs +++ b/src/coaching/cli.rs @@ -40,9 +40,22 @@ pub fn print_list() { println!("agentflare coaching rules ({}/{MAX_RULES}):\n", rules.len()); for r in &rules { if !r.sync.is_empty() { - println!(" {:<10} {} (synced: {})", r.id, r.title, r.sync.join(", ")); + println!( + " {:<10} {} ({}, applied {}, synced: {})", + r.id, + r.title, + r.tier.as_str(), + r.applied_at, + r.sync.join(", ") + ); } else { - println!(" {:<10} {} (no sync)", r.id, r.title); + println!( + " {:<10} {} ({}, applied {}, no sync)", + r.id, + r.title, + r.tier.as_str(), + r.applied_at + ); } println!(" {}", r.body); println!(" {}", describe_trigger(r.trigger.as_ref())); diff --git a/src/coaching/rule.rs b/src/coaching/rule.rs index 6c22192b..fbfd951e 100644 --- a/src/coaching/rule.rs +++ b/src/coaching/rule.rs @@ -320,12 +320,8 @@ mod tests { #[test] fn validate_rule_fields_rejects_unknown_sync_host() { - assert!( - validate_rule_fields("Title", None, &["unknown".to_string()]).is_err() - ); - assert!( - validate_rule_fields("Title", None, &["claude-code".to_string()]).is_ok() - ); + assert!(validate_rule_fields("Title", None, &["unknown".to_string()]).is_err()); + assert!(validate_rule_fields("Title", None, &["claude-code".to_string()]).is_ok()); } #[test] @@ -431,7 +427,16 @@ mod tests { #[test] fn write_then_parse_roundtrips_no_trigger() { let dir = temp_dir_for_test(); - write_rule_file(&dir, "hygiene", "Title", "Body", None, RuleTier::Override, &[]).unwrap(); + write_rule_file( + &dir, + "hygiene", + "Title", + "Body", + None, + RuleTier::Override, + &[], + ) + .unwrap(); let rule = parse_rule_file(&dir.join("coaching-hygiene.md")).unwrap(); assert_eq!(rule.trigger, None); @@ -455,7 +460,10 @@ mod tests { let rule = parse_rule_file(&dir.join("coaching-search17.md")).unwrap(); assert_eq!(rule.tier, RuleTier::Builtin); - assert_eq!(rule.sync, vec!["claude-code".to_string(), "opencode".to_string()]); + assert_eq!( + rule.sync, + vec!["claude-code".to_string(), "opencode".to_string()] + ); std::fs::remove_dir_all(&dir).unwrap(); } @@ -479,7 +487,16 @@ mod tests { #[test] fn parse_rule_file_skips_file_with_invalid_id_in_filename() { let dir = temp_dir_for_test(); - write_rule_file(&dir, "hygiene", "Title", "Body", None, RuleTier::Override, &[]).unwrap(); + write_rule_file( + &dir, + "hygiene", + "Title", + "Body", + None, + RuleTier::Override, + &[], + ) + .unwrap(); std::fs::create_dir_all(&dir).unwrap(); std::fs::write( dir.join("coaching-not a valid id.md"), diff --git a/src/coaching/store.rs b/src/coaching/store.rs index b28f46c7..31924d2f 100644 --- a/src/coaching/store.rs +++ b/src/coaching/store.rs @@ -116,8 +116,16 @@ pub fn apply_rule( .map_err(|e| format!("failed to snapshot previous rule body: {e}"))?; } - rule::write_rule_file(&rules_dir(), id, title, body, trigger.as_ref(), tier.clone(), &sync) - .map_err(|e| format!("failed to write rule file: {e}"))?; + rule::write_rule_file( + &rules_dir(), + id, + title, + body, + trigger.as_ref(), + tier.clone(), + &sync, + ) + .map_err(|e| format!("failed to write rule file: {e}"))?; list_rules() .into_iter() @@ -172,7 +180,7 @@ fn snapshot_previous_body(id: &str, previous_body: &str) -> std::io::Result<()> if !bodies.iter().any(|b| b == previous_body) { bodies.push(previous_body.to_string()); } - std::fs::create_dir_all(&rules_dir())?; + std::fs::create_dir_all(rules_dir())?; std::fs::write(&path, serde_json::to_string(&bodies).unwrap_or_default()) } @@ -279,7 +287,8 @@ mod tests { #[test] fn apply_rule_rejects_invalid_id() { with_temp_home(|| { - let err = apply_rule("1bad", "Title", "Body", None, RuleTier::Override, vec![]).unwrap_err(); + let err = + apply_rule("1bad", "Title", "Body", None, RuleTier::Override, vec![]).unwrap_err(); assert!(err.contains("invalid rule id")); assert!(list_rules().is_empty()); }); @@ -288,7 +297,15 @@ mod tests { #[test] fn apply_rule_rejects_newline_in_title() { with_temp_home(|| { - let err = apply_rule("hygiene", "bad\ntitle", "Body", None, RuleTier::Override, vec![]).unwrap_err(); + let err = apply_rule( + "hygiene", + "bad\ntitle", + "Body", + None, + RuleTier::Override, + vec![], + ) + .unwrap_err(); assert!(err.contains("newline")); assert!(list_rules().is_empty()); }); @@ -301,7 +318,15 @@ mod tests { tools: vec![], auto_match: false, }; - let err = apply_rule("hygiene", "Title", "Body", Some(empty), RuleTier::Override, vec![]).unwrap_err(); + let err = apply_rule( + "hygiene", + "Title", + "Body", + Some(empty), + RuleTier::Override, + vec![], + ) + .unwrap_err(); assert!(err.contains("empty trigger")); assert!(list_rules().is_empty()); }); @@ -313,7 +338,8 @@ mod tests { for i in 0..MAX_RULES { apply_rule(&format!("r{i}"), "T", "B", None, RuleTier::Override, vec![]).unwrap(); } - let err = apply_rule("one-more", "T", "B", None, RuleTier::Override, vec![]).unwrap_err(); + let err = + apply_rule("one-more", "T", "B", None, RuleTier::Override, vec![]).unwrap_err(); assert!(err.contains("maximum")); assert_eq!(list_rules().len(), MAX_RULES); }); @@ -325,7 +351,15 @@ mod tests { for i in 0..MAX_RULES { apply_rule(&format!("r{i}"), "T", "B", None, RuleTier::Override, vec![]).unwrap(); } - let updated = apply_rule("r0", "New Title", "New Body", None, RuleTier::Override, vec![]).unwrap(); + let updated = apply_rule( + "r0", + "New Title", + "New Body", + None, + RuleTier::Override, + vec![], + ) + .unwrap(); assert_eq!(updated.title, "New Title"); assert_eq!(list_rules().len(), MAX_RULES); }); @@ -368,8 +402,24 @@ mod tests { #[test] fn untriggered_rule_bodies_returns_all_untriggered_bodies_in_id_order() { with_temp_home(|| { - apply_rule("b-rule", "Title B", "Body B", None, RuleTier::Override, vec![]).unwrap(); - apply_rule("a-rule", "Title A", "Body A", None, RuleTier::Override, vec![]).unwrap(); + apply_rule( + "b-rule", + "Title B", + "Body B", + None, + RuleTier::Override, + vec![], + ) + .unwrap(); + apply_rule( + "a-rule", + "Title A", + "Body A", + None, + RuleTier::Override, + vec![], + ) + .unwrap(); assert_eq!( untriggered_rule_bodies(), vec!["Body A".to_string(), "Body B".to_string()] @@ -380,7 +430,15 @@ mod tests { #[test] fn apply_rule_body_with_dashes_line_is_not_truncated() { with_temp_home(|| { - let applied = apply_rule("dashes", "Title", "before\n---\nafter", None, RuleTier::Override, vec![]).unwrap(); + let applied = apply_rule( + "dashes", + "Title", + "before\n---\nafter", + None, + RuleTier::Override, + vec![], + ) + .unwrap(); assert!( applied.body.contains("before"), "body should contain text before the --- line: {}", @@ -402,7 +460,15 @@ mod tests { #[test] fn apply_rule_title_with_em_dash_is_not_truncated() { with_temp_home(|| { - let applied = apply_rule("emdash", "Foo \u{2014} Bar", "Body", None, RuleTier::Override, vec![]).unwrap(); + let applied = apply_rule( + "emdash", + "Foo \u{2014} Bar", + "Body", + None, + RuleTier::Override, + vec![], + ) + .unwrap(); assert_eq!(applied.title, "Foo \u{2014} Bar"); let rules = list_rules(); @@ -418,7 +484,15 @@ mod tests { tools: vec!["mcp__flare__review".to_string()], auto_match: true, }; - let applied = apply_rule("revfix", "Title", "Body", Some(trigger.clone()), RuleTier::Override, vec![]).unwrap(); + let applied = apply_rule( + "revfix", + "Title", + "Body", + Some(trigger.clone()), + RuleTier::Override, + vec![], + ) + .unwrap(); assert_eq!(applied.trigger, Some(trigger.clone())); let rules = list_rules(); @@ -430,7 +504,8 @@ mod tests { #[test] fn apply_rule_without_trigger_is_untriggered() { with_temp_home(|| { - let applied = apply_rule("hygiene", "Title", "Body", None, RuleTier::Override, vec![]).unwrap(); + let applied = + apply_rule("hygiene", "Title", "Body", None, RuleTier::Override, vec![]).unwrap(); assert_eq!(applied.trigger, None); }); } @@ -556,11 +631,28 @@ mod tests { #[test] fn apply_rule_snapshots_previous_body_when_overwriting_a_builtin_rule() { with_temp_home(|| { - apply_rule("search17", "T", "Old body", None, rule::RuleTier::Builtin, vec!["claude-code".to_string()]).unwrap(); - apply_rule("search17", "T", "New body", None, rule::RuleTier::Builtin, vec!["claude-code".to_string()]).unwrap(); + apply_rule( + "search17", + "T", + "Old body", + None, + rule::RuleTier::Builtin, + vec!["claude-code".to_string()], + ) + .unwrap(); + apply_rule( + "search17", + "T", + "New body", + None, + rule::RuleTier::Builtin, + vec!["claude-code".to_string()], + ) + .unwrap(); let path = rules_dir().join("superseded-search17.json"); - let bodies: Vec = serde_json::from_str(&std::fs::read_to_string(path).unwrap()).unwrap(); + let bodies: Vec = + serde_json::from_str(&std::fs::read_to_string(path).unwrap()).unwrap(); assert_eq!(bodies, vec!["Old body".to_string()]); }); } @@ -568,8 +660,24 @@ mod tests { #[test] fn apply_rule_does_not_snapshot_when_overwriting_an_override_tier_rule() { with_temp_home(|| { - apply_rule("hygiene", "T", "Old body", None, rule::RuleTier::Override, vec![]).unwrap(); - apply_rule("hygiene", "T", "New body", None, rule::RuleTier::Override, vec![]).unwrap(); + apply_rule( + "hygiene", + "T", + "Old body", + None, + rule::RuleTier::Override, + vec![], + ) + .unwrap(); + apply_rule( + "hygiene", + "T", + "New body", + None, + rule::RuleTier::Override, + vec![], + ) + .unwrap(); assert!(!rules_dir().join("superseded-hygiene.json").exists()); }); } @@ -577,8 +685,24 @@ mod tests { #[test] fn apply_rule_does_not_snapshot_when_body_is_unchanged() { with_temp_home(|| { - apply_rule("search17", "T", "Same body", None, rule::RuleTier::Builtin, vec![]).unwrap(); - apply_rule("search17", "T", "Same body", None, rule::RuleTier::Builtin, vec![]).unwrap(); + apply_rule( + "search17", + "T", + "Same body", + None, + rule::RuleTier::Builtin, + vec![], + ) + .unwrap(); + apply_rule( + "search17", + "T", + "Same body", + None, + rule::RuleTier::Builtin, + vec![], + ) + .unwrap(); assert!(!rules_dir().join("superseded-search17.json").exists()); }); } diff --git a/src/hook_redirect.rs b/src/hook_redirect.rs index 15a71237..57eb652a 100644 --- a/src/hook_redirect.rs +++ b/src/hook_redirect.rs @@ -76,12 +76,20 @@ fn is_spec_like_path(path: &str) -> bool { /// mid-migration ... store.db ... ~/.agentflare/*.db*" — none of that is an /// executed command, but a whole-string substring check can't tell). fn destructive_data_file_reason(command: &str) -> Option { - for statement in command.split([';', '\n']).flat_map(|s| s.split("&&")).flat_map(|s| s.split("||")).flat_map(|s| s.split('|')) { + for statement in command + .split([';', '\n']) + .flat_map(|s| s.split("&&")) + .flat_map(|s| s.split("||")) + .flat_map(|s| s.split('|')) + { let trimmed = statement.trim().to_lowercase().replace('\\', "/"); let Some(first_word) = trimmed.split_whitespace().next() else { continue; }; - let is_destructive_verb = matches!(first_word, "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir"); + let is_destructive_verb = matches!( + first_word, + "rm" | "del" | "erase" | "remove-item" | "unlink" | "rmdir" + ); if !is_destructive_verb { continue; } diff --git a/src/init.rs b/src/init.rs index 3ad1b90b..c17c1847 100644 --- a/src/init.rs +++ b/src/init.rs @@ -39,7 +39,7 @@ pub(crate) fn is_stale_rule(path: &PathBuf, current: &str) -> bool { // Check compiled-in superseded bodies first (exa.md, git.md, etc.). let superseded = rule_text::superseded(filename); - if superseded.iter().any(|old| trimmed == *old) { + if superseded.contains(&trimmed) { return true; } @@ -507,10 +507,13 @@ pub(crate) fn wire_opencode_instructions() { for file in &expected_filenames { let rule_path = rules_dir.join(file); let path_str = rule_path.to_string_lossy().replace('\\', "/"); - let has_it = arr + let has_it = arr.iter().any(|v| { + v.as_str() + .map(|s| s.contains(file.as_str())) + .unwrap_or(false) + }) || sibling_instructions .iter() - .any(|v| v.as_str().map(|s| s.contains(file.as_str())).unwrap_or(false)) - || sibling_instructions.iter().any(|s| s.contains(file.as_str())); + .any(|s| s.contains(file.as_str())); if !has_it && rule_path.exists() { arr.push(json!(path_str)); added += 1; From fe8674dbc3e99057b5641b73778a2468a724cae6 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sun, 26 Jul 2026 03:34:40 +0530 Subject: [PATCH 09/13] flare-git-core: thread ResolvedGitShimPolicy through classify_pure and resolve_trust_root_touch --- crates/flare-git-core/src/classify.rs | 208 +++++++++++++++++++++----- 1 file changed, 169 insertions(+), 39 deletions(-) diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index 86baff9f..1923d3c9 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -32,6 +32,7 @@ use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; use crate::branch::{current_branch, is_protected_branch, resolve_default_branch}; +use crate::policy_config::ResolvedGitShimPolicy; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum Disposition { @@ -214,13 +215,21 @@ pub fn classify_pure( default_branch: &str, trust_root_touch: &TrustRootTouch, push_targets_default_branch: bool, + policy: &ResolvedGitShimPolicy, ) -> Disposition { if READ_ONLY_SUBCOMMANDS.contains(&subcommand) - || ALLOWED_MUTATING_SUBCOMMANDS.contains(&subcommand) + || policy + .allowed_mutating_subcommands + .iter() + .any(|s| s.as_str() == subcommand) { return Disposition::Passthrough; } - if DENIED_PLUMBING_SUBCOMMANDS.contains(&subcommand) { + if policy + .denied_plumbing_subcommands + .iter() + .any(|s| s.as_str() == subcommand) + { return Disposition::Deny { reason: format!( "'git {subcommand}' is a low-level plumbing command blocked by the agentflare git shim — it can bypass the checks this shim applies to higher-level commands." @@ -344,17 +353,18 @@ pub enum TrustRootTouch { /// default to let through, but the caller shouldn't claim to know which /// path caused it. #[must_use] -pub fn resolve_trust_root_touch(repo_root: &Path, branch: &str, target: &str) -> TrustRootTouch { - let extra = extra_trust_root_paths_from_env(); +pub fn resolve_trust_root_touch( + repo_root: &Path, + branch: &str, + target: &str, + trust_root_paths: &[String], +) -> TrustRootTouch { let range = format!("{target}...{branch}"); match crate::shell::run_in(repo_root, &["diff", "--name-only", &range]) { Ok(names) => { let mut matched: Vec = names .lines() - .filter(|f| { - TRUST_ROOT_PATHS.iter().any(|p| f.starts_with(p)) - || extra.iter().any(|p| f.starts_with(p.as_str())) - }) + .filter(|f| trust_root_paths.iter().any(|p| f.starts_with(p.as_str()))) .map(str::to_string) .collect(); matched.sort(); @@ -415,6 +425,17 @@ pub fn classify_with_home( args: &[String], home: Option<&Path>, ) -> Event { + let policy = crate::policy_config::resolve(repo_root, home).unwrap_or_else(|e| { + eprintln!( + "WARNING: agentflare git-shim config at {} is invalid ({}) -- \ + using baseline policy only, no config-sourced additions applied. \ + Git operations are not blocked by this; fix the file to restore \ + your customizations.", + e.path.display(), + e.source + ); + ResolvedGitShimPolicy::baseline() + }); let default_branch = resolve_default_branch(repo_root); // Resolve the actual pushed branch once, then derive both push facts from // it: whether it carries trust-root changes and whether it *is* the @@ -424,7 +445,7 @@ pub fn classify_with_home( .flatten(); let trust_root_touch = pushed .as_deref() - .map(|b| resolve_trust_root_touch(repo_root, b, &default_branch)) + .map(|b| resolve_trust_root_touch(repo_root, b, &default_branch, &policy.trust_root_paths)) .unwrap_or(TrustRootTouch::Clean); let targets_default_branch = pushed .as_deref() @@ -435,6 +456,7 @@ pub fn classify_with_home( &default_branch, &trust_root_touch, targets_default_branch, + &policy, ); // Every deny above (protected-branch checkout/switch/delete/rename, // trust-root push, plumbing block, worktree) exists to protect agentflare's @@ -466,8 +488,16 @@ mod tests { #[test] fn read_only_subcommands_pass_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( - classify_pure("status", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "status", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Passthrough ); assert_eq!( @@ -476,7 +506,8 @@ mod tests { &args(&["-5"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -484,13 +515,15 @@ mod tests { #[test] fn ordinary_mutating_subcommands_pass_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "commit", &args(&["-m", "x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -500,7 +533,8 @@ mod tests { &args(&["HEAD~1"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -508,6 +542,7 @@ mod tests { #[test] fn unknown_subcommand_passes_through_by_default() { + let policy = ResolvedGitShimPolicy::baseline(); // Fail-open: this shim must never block a subcommand it hasn't // been explicitly taught to deny. assert_eq!( @@ -516,7 +551,8 @@ mod tests { &[], "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -526,7 +562,8 @@ mod tests { &args(&["update"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -536,7 +573,8 @@ mod tests { &args(&["start"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -546,7 +584,8 @@ mod tests { &args(&["pull"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -554,25 +593,42 @@ mod tests { #[test] fn plumbing_commands_are_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( - classify_pure("update-index", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "update-index", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Deny { .. } )); assert!(matches!( - classify_pure("apply", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "apply", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Deny { .. } )); } #[test] fn worktree_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "worktree", &args(&["add", "../x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -580,13 +636,15 @@ mod tests { #[test] fn worktree_remove_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "worktree", &args(&["remove", "../x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -594,13 +652,15 @@ mod tests { #[test] fn worktree_list_is_passthrough() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "worktree", &args(&["list"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -608,13 +668,15 @@ mod tests { #[test] fn worktree_prune_dry_run_is_passthrough() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "worktree", &args(&["prune", "--dry-run"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -622,13 +684,15 @@ mod tests { #[test] fn worktree_prune_without_dry_run_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "worktree", &args(&["prune"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -636,25 +700,29 @@ mod tests { #[test] fn checkout_to_protected_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); let d = classify_pure( "checkout", &args(&["master"]), "master", &TrustRootTouch::Clean, false, + &policy, ); assert!(matches!(d, Disposition::Deny { .. })); } #[test] fn switch_to_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "switch", &args(&["feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -662,6 +730,7 @@ mod tests { #[test] fn checkout_with_no_target_arg_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); // `git switch -` (previous branch) — nothing to protect against. assert_eq!( classify_pure( @@ -669,7 +738,8 @@ mod tests { &args(&["-"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -677,6 +747,7 @@ mod tests { #[test] fn push_touching_trust_root_on_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); // A PR-review gate still applies before this reaches the default // branch — same reasoning as any other feature-branch push. assert_eq!( @@ -685,7 +756,8 @@ mod tests { &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Touched(vec!["Cargo.toml".to_string()]), - false + false, + &policy ), Disposition::Passthrough ); @@ -693,13 +765,15 @@ mod tests { #[test] fn push_touching_trust_root_on_default_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "push", &args(&["origin", "master"]), "master", &TrustRootTouch::Touched(vec!["Cargo.toml".to_string()]), - true + true, + &policy ), Disposition::Deny { .. } )); @@ -707,13 +781,15 @@ mod tests { #[test] fn push_not_touching_trust_root_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "push", &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -721,6 +797,7 @@ mod tests { #[test] fn push_of_default_branch_is_denied_even_without_trust_root_changes() { + let policy = ResolvedGitShimPolicy::baseline(); // Enforce PR-only: pushing the default branch straight to a remote is // blocked regardless of what the diff touches. assert!(matches!( @@ -729,7 +806,8 @@ mod tests { &args(&["origin", "master"]), "master", &TrustRootTouch::Clean, - true + true, + &policy ), Disposition::Deny { .. } )); @@ -737,13 +815,15 @@ mod tests { #[test] fn push_of_feature_branch_is_not_a_default_branch_push() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "push", &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -751,13 +831,15 @@ mod tests { #[test] fn branch_delete_of_protected_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "branch", &args(&["-D", "master"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -767,7 +849,8 @@ mod tests { &args(&["--delete", "master"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -775,13 +858,15 @@ mod tests { #[test] fn branch_rename_of_protected_branch_is_denied() { + let policy = ResolvedGitShimPolicy::baseline(); assert!(matches!( classify_pure( "branch", &args(&["-M", "master", "renamed"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Deny { .. } )); @@ -789,13 +874,15 @@ mod tests { #[test] fn branch_delete_of_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "branch", &args(&["-D", "feature/x"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -803,8 +890,16 @@ mod tests { #[test] fn branch_listing_and_creation_pass_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( - classify_pure("branch", &[], "master", &TrustRootTouch::Clean, false), + classify_pure( + "branch", + &[], + "master", + &TrustRootTouch::Clean, + false, + &policy + ), Disposition::Passthrough ); assert_eq!( @@ -813,7 +908,8 @@ mod tests { &args(&["feature/new"]), "master", &TrustRootTouch::Clean, - false + false, + &policy ), Disposition::Passthrough ); @@ -1035,8 +1131,16 @@ mod tests { #[test] fn push_trust_root_deny_message_names_only_the_touched_path() { + let policy = ResolvedGitShimPolicy::baseline(); let touch = TrustRootTouch::Touched(vec!["Cargo.toml".to_string()]); - let d = classify_pure("push", &args(&["origin", "master"]), "master", &touch, true); + let d = classify_pure( + "push", + &args(&["origin", "master"]), + "master", + &touch, + true, + &policy, + ); let Disposition::Deny { reason } = d else { panic!("expected Deny, got {d:?}"); }; @@ -1053,12 +1157,14 @@ mod tests { #[test] fn push_with_unreadable_diff_on_default_branch_denies_with_unknown_message() { + let policy = ResolvedGitShimPolicy::baseline(); let d = classify_pure( "push", &args(&["origin", "master"]), "master", &TrustRootTouch::Unknown, true, + &policy, ); let Disposition::Deny { reason } = d else { panic!("expected Deny, got {d:?}"); @@ -1068,15 +1174,39 @@ mod tests { #[test] fn push_with_unreadable_diff_on_feature_branch_passes_through() { + let policy = ResolvedGitShimPolicy::baseline(); assert_eq!( classify_pure( "push", &args(&["origin", "feature/x"]), "master", &TrustRootTouch::Unknown, - false + false, + &policy ), Disposition::Passthrough ); } + + #[test] + fn malformed_project_local_config_falls_back_to_baseline_without_blocking_git() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + std::fs::create_dir_all(repo.path.join(".agentflare")).unwrap(); + std::fs::write(repo.path.join(".agentflare").join("project.json"), "{}").unwrap(); + std::fs::write( + repo.path.join(".agentflare").join("config.toml"), + "this is not valid toml [[[", + ) + .unwrap(); + + // An ordinary read-only command must still pass through -- a broken + // config file must never block git operations. + let event = classify(&repo.path, "status", &[]); + assert_eq!( + event.disposition, + Disposition::Passthrough, + "{:?}", + event.disposition + ); + } } From 6584295c73b76c11ea801d6e35ed0288931e5c08 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sun, 26 Jul 2026 06:06:49 +0530 Subject: [PATCH 10/13] flare-git-core: end-to-end test for config.toml relaxing git-shim policy --- crates/flare-git-core/src/classify.rs | 31 +++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/crates/flare-git-core/src/classify.rs b/crates/flare-git-core/src/classify.rs index 1923d3c9..7ea1c969 100644 --- a/crates/flare-git-core/src/classify.rs +++ b/crates/flare-git-core/src/classify.rs @@ -1209,4 +1209,35 @@ mod tests { event.disposition ); } + + #[test] + fn project_local_config_can_relax_a_denied_plumbing_subcommand() { + let repo = crate::shell::test_support::init_repo_with_branch("master"); + std::fs::create_dir_all(repo.path.join(".agentflare")).unwrap(); + std::fs::write(repo.path.join(".agentflare").join("project.json"), "{}").unwrap(); + + // Baseline: "apply" is in DENIED_PLUMBING_SUBCOMMANDS. + let before = classify(&repo.path, "apply", &["patch.diff".to_string()]); + assert!( + matches!(before.disposition, Disposition::Deny { .. }), + "{:?}", + before.disposition + ); + + // Project-local config explicitly allows it. ALLOWED_MUTATING is + // checked before DENIED_PLUMBING in classify_pure, so this relaxes it. + std::fs::write( + repo.path.join(".agentflare").join("config.toml"), + "[git_shim]\nextra_allowed_mutating_subcommands = [\"apply\"]\n", + ) + .unwrap(); + + let after = classify(&repo.path, "apply", &["patch.diff".to_string()]); + assert_eq!( + after.disposition, + Disposition::Passthrough, + "{:?}", + after.disposition + ); + } } From 5e190fe2f9011dd865e57755e6e842d2aa34afcc Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sun, 26 Jul 2026 07:29:30 +0530 Subject: [PATCH 11/13] fmt: apply cargo fmt and sync Cargo.lock for new deps CI's fmt job was failing on unformatted coaching/dashboard/mcp_server changes, and clippy's --locked check was failing because Cargo.lock hadn't been regenerated after adding toml/thiserror/agentflare-store/ blake3 as dependencies. --- Cargo.lock | 2 ++ src/cli/coaching.rs | 10 +++++-- src/coaching/mod.rs | 5 +++- src/components.rs | 53 ++++++++++++++++++++++++++++++-------- src/dashboard/artifacts.rs | 19 +++----------- src/mcp_server.rs | 4 ++- 6 files changed, 63 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5c3918c7..df4f22f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1100,6 +1100,8 @@ dependencies = [ "serde", "serde_json", "tempfile", + "thiserror", + "toml", "walkdir", "which", ] diff --git a/src/cli/coaching.rs b/src/cli/coaching.rs index b7832a70..38690ba2 100644 --- a/src/cli/coaching.rs +++ b/src/cli/coaching.rs @@ -1,5 +1,5 @@ -use clap::{Args, Subcommand}; use crate::coaching::rule::RuleTier; +use clap::{Args, Subcommand}; #[derive(Subcommand)] pub enum CoachingAction { @@ -58,7 +58,13 @@ impl CoachingArgs { tier, sync, } => crate::coaching::cli_apply( - &id, &title, &body, trigger_tool, trigger_auto, tier, sync, + &id, + &title, + &body, + trigger_tool, + trigger_auto, + tier, + sync, ), CoachingAction::Remove { id } => crate::coaching::cli_remove(&id), CoachingAction::Sync { agent } => crate::coaching::cli_sync(agent.as_deref()), diff --git a/src/coaching/mod.rs b/src/coaching/mod.rs index b1dad21e..af492d20 100644 --- a/src/coaching/mod.rs +++ b/src/coaching/mod.rs @@ -16,7 +16,10 @@ pub(crate) mod rule; mod store; pub use cli::{cli_apply, cli_remove, cli_sync, print_list}; -pub use store::{rule_bodies_for_prompt, rule_bodies_for_tool, superseded_bodies, sync_targets_for_host, untriggered_rule_bodies}; +pub use store::{ + rule_bodies_for_prompt, rule_bodies_for_tool, superseded_bodies, sync_targets_for_host, + untriggered_rule_bodies, +}; // Only reached from hook.rs's SessionStart test (#[cfg(test)]), to seed a // rule before asserting it appears in the printed message — a plain, diff --git a/src/components.rs b/src/components.rs index bb6cf998..0175b641 100644 --- a/src/components.rs +++ b/src/components.rs @@ -200,7 +200,11 @@ pub(crate) fn rule_targets(host: &str) -> Vec<(PathBuf, String)> { (dir.join("lean-ctx.md"), rule_text::LEANCTX.to_string()), (dir.join("flare-docs.md"), rule_text::FLARE_DOCS.to_string()), ]; - v.extend(coaching.iter().map(|(id, body)| (dir.join(format!("{id}.md")), body.clone()))); + v.extend( + coaching + .iter() + .map(|(id, body)| (dir.join(format!("{id}.md")), body.clone())), + ); v } "cursor" => { @@ -240,7 +244,11 @@ pub(crate) fn rule_targets(host: &str) -> Vec<(PathBuf, String)> { (dir.join("lean-ctx.md"), rule_text::LEANCTX.to_string()), (dir.join("flare-docs.md"), rule_text::FLARE_DOCS.to_string()), ]; - v.extend(coaching.iter().map(|(id, body)| (dir.join(format!("{id}.md")), body.clone()))); + v.extend( + coaching + .iter() + .map(|(id, body)| (dir.join(format!("{id}.md")), body.clone())), + ); v } _ => vec![], // "continue" — no dedicated rules convention found @@ -868,16 +876,28 @@ mod tests { fn rule_targets_includes_coaching_sourced_rule_for_claude_code_and_opencode() { crate::paths::test_support::with_temp_home(|| { crate::coaching::apply_rule( - "search17", "T", "Coaching body", None, + "search17", + "T", + "Coaching body", + None, crate::coaching::rule::RuleTier::Builtin, vec!["claude-code".to_string(), "opencode".to_string()], - ).unwrap(); + ) + .unwrap(); let cc = rule_targets("claude-code"); - assert!(cc.iter().any(|(p, c)| p.to_string_lossy().ends_with("search17.md") && c == "Coaching body")); + assert!( + cc.iter() + .any(|(p, c)| p.to_string_lossy().ends_with("search17.md") + && c == "Coaching body") + ); let oc = rule_targets("opencode"); - assert!(oc.iter().any(|(p, c)| p.to_string_lossy().ends_with("search17.md") && c == "Coaching body")); + assert!( + oc.iter() + .any(|(p, c)| p.to_string_lossy().ends_with("search17.md") + && c == "Coaching body") + ); }); } @@ -885,15 +905,22 @@ mod tests { fn rule_targets_appends_coaching_sourced_body_into_joined_hosts() { crate::paths::test_support::with_temp_home(|| { crate::coaching::apply_rule( - "search17", "T", "Coaching body", None, + "search17", + "T", + "Coaching body", + None, crate::coaching::rule::RuleTier::Builtin, vec!["cursor".to_string()], - ).unwrap(); + ) + .unwrap(); let targets = rule_targets("cursor"); assert_eq!(targets.len(), 1, "cursor stays a single joined file"); assert!(targets[0].1.contains("Coaching body")); - assert!(targets[0].1.contains(rule_text::FLARE_DOCS), "existing builtin content must still be present"); + assert!( + targets[0].1.contains(rule_text::FLARE_DOCS), + "existing builtin content must still be present" + ); }); } @@ -901,10 +928,14 @@ mod tests { fn rule_targets_omits_coaching_rule_not_synced_to_this_host() { crate::paths::test_support::with_temp_home(|| { crate::coaching::apply_rule( - "search17", "T", "Coaching body", None, + "search17", + "T", + "Coaching body", + None, crate::coaching::rule::RuleTier::Builtin, vec!["opencode".to_string()], - ).unwrap(); + ) + .unwrap(); let cc = rule_targets("claude-code"); assert!(!cc.iter().any(|(_, c)| c == "Coaching body")); diff --git a/src/dashboard/artifacts.rs b/src/dashboard/artifacts.rs index 33331e2d..b0133eeb 100644 --- a/src/dashboard/artifacts.rs +++ b/src/dashboard/artifacts.rs @@ -46,10 +46,7 @@ struct VersionPath { version: u32, } -async fn artifact_page( - State(state): State, - Path(id): Path, -) -> Response { +async fn artifact_page(State(state): State, Path(id): Path) -> Response { let Ok(artifact) = state.store.get(&id) else { return (StatusCode::NOT_FOUND, "artifact not found").into_response(); }; @@ -68,10 +65,7 @@ async fn artifact_version_page( ([(header::CONTENT_TYPE, "text/html; charset=utf-8")], html).into_response() } -async fn versions_json( - State(state): State, - Path(id): Path, -) -> Response { +async fn versions_json(State(state): State, Path(id): Path) -> Response { match state.store.versions(&id) { Ok(history) => ( [(header::CONTENT_TYPE, "application/json")], @@ -82,10 +76,7 @@ async fn versions_json( } } -async fn artifact_live( - State(state): State, - Path(id): Path, -) -> Response { +async fn artifact_live(State(state): State, Path(id): Path) -> Response { if !agentflare_artifacts::valid_id(&id) { return (StatusCode::NOT_FOUND, "invalid id").into_response(); } @@ -99,9 +90,7 @@ async fn artifact_live( } }); let stream = UnboundedReceiverStream::new(async_rx).map(|event| { - Ok::<_, std::convert::Infallible>( - axum::response::sse::Event::default().data(event), - ) + Ok::<_, std::convert::Infallible>(axum::response::sse::Event::default().data(event)) }); Sse::new(stream).into_response() } diff --git a/src/mcp_server.rs b/src/mcp_server.rs index 8acf90a4..a7edd528 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -417,7 +417,9 @@ impl AgentflareMcp { } None => { let store = match crate::store::open() { - Ok(s) => std::sync::Arc::new(agentflare_artifacts::ArtifactStore::with_store(s)), + Ok(s) => { + std::sync::Arc::new(agentflare_artifacts::ArtifactStore::with_store(s)) + } Err(e) => { eprintln!("[artifacts] fallback to flat-file store: {e}"); let dir = crate::paths::home().join(".agentflare").join("artifacts"); From 681728a9bc65cb4092510099cdeba2840febd8b6 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Sun, 26 Jul 2026 07:29:36 +0530 Subject: [PATCH 12/13] flare-git-core: box toml::de::Error in LoaderError to fix clippy::result_large_err toml::de::Error is >128 bytes, so embedding it directly in LoaderError tripped clippy::result_large_err (denied via -D warnings) on every function returning Result<_, LoaderError>. --- crates/flare-git-core/src/config_loader.rs | 4 ++-- crates/flare-git-core/src/policy_config.rs | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/flare-git-core/src/config_loader.rs b/crates/flare-git-core/src/config_loader.rs index 3a0a1980..52100a1f 100644 --- a/crates/flare-git-core/src/config_loader.rs +++ b/crates/flare-git-core/src/config_loader.rs @@ -11,7 +11,7 @@ pub struct ConfigLayers { pub struct LoaderError { pub path: PathBuf, #[source] - pub source: toml::de::Error, + pub source: Box, } fn parse_if_exists(path: &Path) -> Result, LoaderError> { @@ -22,7 +22,7 @@ fn parse_if_exists(path: &Path) -> Result, Loader .map(|v| Some((path.to_path_buf(), v))) .map_err(|source| LoaderError { path: path.to_path_buf(), - source, + source: Box::new(source), }) } diff --git a/crates/flare-git-core/src/policy_config.rs b/crates/flare-git-core/src/policy_config.rs index abb9659b..0b340d1e 100644 --- a/crates/flare-git-core/src/policy_config.rs +++ b/crates/flare-git-core/src/policy_config.rs @@ -66,7 +66,10 @@ fn parse_git_shim(layer: Option<(PathBuf, toml::Value)>) -> Result Date: Sun, 26 Jul 2026 11:08:45 +0530 Subject: [PATCH 13/13] paths: fix test-isolation race in with_temp_home/with_temp_cwd Root cause of the intermittent build (windows-latest) CI failures in state::tests::* and vent::capture::tests::*: with_temp_home/with_temp_cwd reused a single fixed directory name across every call. A mutex serialized the env-var mutation itself, but under cargo test's default parallel runner and heavy concurrent filesystem load elsewhere in the 811-test suite, a previous call's directory could still be non-empty (or its file handles not yet released) by the time the next call reused the same path, leaking persisted state (SQLite store contents, vent log entries) from one test into an unrelated one. Fixed by giving each call a uniquely-named tempfile::tempdir() instead of a shared fixed name, so no two calls can ever collide on the same directory regardless of timing. Also made both helpers panic-safe via Drop guards, so a failing assertion inside the wrapped closure can no longer leave AGENTFLARE_HOME_OVERRIDE (or the cwd) permanently altered for whatever the test binary runs next. Verified: 7 consecutive clean cargo test --workspace / -p agentflare runs (0 failures) after the fix, versus 100% reproducible failure before it. Added two regression tests exercising with_temp_home under real thread contention. --- src/paths.rs | 121 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 104 insertions(+), 17 deletions(-) diff --git a/src/paths.rs b/src/paths.rs index 069a6b7c..a024219c 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -105,34 +105,121 @@ pub(crate) mod test_support { // set_var("AGENTFLARE_HOME_OVERRIDE") race a set_var("PATH") on another // thread — exactly the UB set_var is unsafe for. use agent_registry::detect::PATH_LOCK as GLOBAL_STATE_LOCK; + use std::path::PathBuf; + + // Removes AGENTFLARE_HOME_OVERRIDE on drop -- including on unwind, so a + // panicking assertion inside `f()` can't leave the override set for + // whatever test runs next on another thread once GLOBAL_STATE_LOCK is + // released (poisoned-mutex recovery only protects the lock itself, not + // env state a previous holder forgot to restore). + struct ResetHomeOverrideOnDrop; + impl Drop for ResetHomeOverrideOnDrop { + fn drop(&mut self) { + unsafe { + // SAFETY: still under GLOBAL_STATE_LOCK for the duration of + // this guard's life. + std::env::remove_var("AGENTFLARE_HOME_OVERRIDE"); + } + } + } pub(crate) fn with_temp_home(f: impl FnOnce() -> T) -> T { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let dir = std::env::temp_dir().join("agentflare-test-home"); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); + // A fresh, uniquely-named directory per call -- not a fixed shared + // name -- so a previous call's leftover file handle (e.g. a SQLite + // -wal/-shm file Windows hasn't released yet) can never leak into + // the next call even if that previous directory hasn't finished + // being cleaned up. See git history for the shared-fixed-name bug + // this replaced (state.rs/vent::capture.rs tests intermittently + // observed each other's persisted state under parallel execution). + let dir = tempfile::tempdir().unwrap(); unsafe { // SAFETY: GLOBAL_STATE_LOCK mutex serializes all env mutations; // no other thread can read or write AGENTFLARE_HOME_OVERRIDE concurrently. - std::env::set_var("AGENTFLARE_HOME_OVERRIDE", &dir) - }; - let result = f(); - unsafe { - // SAFETY: GLOBAL_STATE_LOCK mutex serializes all env mutations. - std::env::remove_var("AGENTFLARE_HOME_OVERRIDE") + std::env::set_var("AGENTFLARE_HOME_OVERRIDE", dir.path()) }; - result + let _reset = ResetHomeOverrideOnDrop; + f() + } + + // Restores the original cwd on drop -- including on unwind, same + // reasoning as ResetHomeOverrideOnDrop above. + struct RestoreCwdOnDrop(PathBuf); + impl Drop for RestoreCwdOnDrop { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } } pub(crate) fn with_temp_cwd(f: impl FnOnce() -> T) -> T { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let dir = std::env::temp_dir().join("agentflare-test-cwd"); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); + // Same reasoning as with_temp_home above: a unique dir per call + // instead of a fixed shared name. + let temp_dir = tempfile::tempdir().unwrap(); let original = std::env::current_dir().unwrap(); - std::env::set_current_dir(&dir).unwrap(); - let result = f(); - std::env::set_current_dir(&original).unwrap(); - result + std::env::set_current_dir(temp_dir.path()).unwrap(); + let _restore = RestoreCwdOnDrop(original); + f() + } +} + +#[cfg(test)] +mod tests { + use super::test_support::with_temp_home; + + // Regression test for a real Windows CI flake (state::tests::* and + // vent::capture::tests::* intermittently observed each other's + // persisted state under `cargo test --workspace`'s default parallel + // runner): with_temp_home used a single fixed directory name shared by + // every call, so a previous call's file (left behind if e.g. Windows + // hadn't yet released a SQLite -wal/-shm handle) could still be present + // when the next call's directory was supposed to be empty. + #[test] + fn with_temp_home_never_sees_a_previous_calls_leftover_file() { + for i in 0..20 { + with_temp_home(|| { + let marker = super::home().join("marker.txt"); + assert!( + !marker.exists(), + "iteration {i}: found a marker file left behind by a previous with_temp_home call at {}", + super::home().display() + ); + std::fs::write(&marker, "left behind on purpose").unwrap(); + }); + } + } + + // Same check under real thread contention -- GLOBAL_STATE_LOCK forces + // these to run one at a time, but back-to-back-under-contention is + // exactly the timing the original shared-fixed-directory bug needed to + // show up under Windows' delayed file-handle release. + #[test] + fn with_temp_home_isolates_calls_under_thread_contention() { + std::thread::scope(|scope| { + for _ in 0..8 { + scope.spawn(|| { + for _ in 0..20 { + with_temp_home(|| { + let marker = super::home().join("marker.txt"); + assert!( + !marker.exists(), + "found a marker file left behind by another with_temp_home call at {}", + super::home().display() + ); + std::fs::write(&marker, "left behind on purpose").unwrap(); + }); + } + }); + } + }); + } + + #[test] + fn with_temp_home_clears_the_override_env_var_after_returning() { + with_temp_home(|| {}); + assert!( + std::env::var("AGENTFLARE_HOME_OVERRIDE").is_err(), + "AGENTFLARE_HOME_OVERRIDE must not remain set once with_temp_home returns" + ); } }