Merge pull request #14 from Dark-Alex-17/feat/rag-driver-abstraction-v3

feat(rag): pluggable RagProvider abstraction with DuckDB and Qdrant drivers

.gitignore

@@ -5,3 +5,4 @@
 .idea/
 /coyote.iml
 /.idea/
+.coyote

src/config/agent.rs

@@ -4,6 +4,7 @@ use crate::{
     client::Model,
     config::memory,
     function::{Functions, run_llm_function},
+    graph, rag,
 };
 
 use super::rag_cache::RagKey;
@@ -185,7 +186,7 @@ impl Agent {
                             &rag_path_clone,
                             &document_paths,
                             abort,
-                            false,
+                            true,
                         )
                         .await
                     })
@@ -1021,11 +1022,11 @@ async fn init_graph_rags(
             // Graph validation catches this too, but it is skipped when
             // `validate_before_run` is off, so this guard is the load-bearing one.
             if let Some(driver) = &rag_node.driver
-                && let Some(message) = crate::graph::validator::rag_driver_error(driver)
+                && let Some(message) = graph::validator::rag_driver_error(driver)
             {
                 bail!("rag node '{node_id}': {message}");
             }
-            let config = rag_init_config(rag_node);
+            let mut config = rag_init_config(rag_node);
             let fully_specified = config.embedding_model.is_some()
                 && config.chunk_size.is_some()
                 && config.chunk_overlap.is_some();
@@ -1051,6 +1052,10 @@ async fn init_graph_rags(
                          initialized. RAG initialization is required for this agent."
                     );
                 }
+
+                if config.driver.is_none() {
+                    config.driver = Some(rag::select_rag_driver()?);
+                }
             }
 
             let document_paths =

src/config/paths.rs

@@ -437,6 +437,10 @@ pub(crate) fn remove_rag_sidecars(dir: &Path, name: &str) -> Result<()> {
     if duckdb_path.exists() {
         let _ = remove_file(&duckdb_path);
     }
+    let wal_path = dir.join(format!("{name}.duckdb.wal"));
+    if wal_path.exists() {
+        let _ = remove_file(&wal_path);
+    }
     let mixin_path = dir.join(format!("{name}.sbx-mixin.yaml"));
     if mixin_path.exists() {
         remove_file(&mixin_path).with_context(|| {
@@ -894,16 +898,19 @@ mod tests {
     }
 
     #[test]
-    fn remove_rag_sidecars_removes_both() {
+    fn remove_rag_sidecars_removes_duckdb_wal_and_mixin() {
         let root = sidecar_temp_dir("rag-sidecars-both");
         let duckdb = root.join("docs.duckdb");
+        let wal = root.join("docs.duckdb.wal");
         let mixin = root.join("docs.sbx-mixin.yaml");
         fs::write(&duckdb, "db").unwrap();
+        fs::write(&wal, "wal").unwrap();
         fs::write(&mixin, "mixin").unwrap();
 
         remove_rag_sidecars(&root, "docs").unwrap();
 
         assert!(!duckdb.exists(), "the .duckdb sidecar must be removed");
+        assert!(!wal.exists(), "the .duckdb.wal sidecar must be removed");
         assert!(
             !mixin.exists(),
             "the .sbx-mixin.yaml sidecar must be removed"

src/rag/mod.rs

@@ -267,31 +267,10 @@ impl Rag {
         }
         println!("⚙ Initializing RAG...");
         let (embedding_model, chunk_size, chunk_overlap) = Self::create_config(app)?;
-        // Only interactive named-RAG creation offers a driver choice. Temp RAGs and
-        // agent startup pass `false`; an explicit flag is used rather than inferring
-        // from the name because the agent path passes the literal name "rag", which is
-        // indistinguishable from a user creating a RAG genuinely named `rag`.
         let driver = if prompt_for_driver {
-            let options = vec![
-                "yaml   — portable, in-memory HNSW; usable from several Coyote processes at once (default)",
-                "duckdb — persistent on-disk store; vectors and content survive restarts; HNSW approximate search.",
-            ];
-            let sel = Select::new("RAG storage driver:", options)
-                .with_starting_cursor(0)
-                .prompt()?;
-            if sel.starts_with("duckdb") {
-                println!(
-                    "Note: several Coyote processes can query a duckdb RAG at the same time, \
-                     but while one process is ingesting or rebuilding it the others cannot \
-                     read it until that finishes. Changing its driver later means deleting \
-                     and recreating the RAG."
-                );
-                "duckdb"
+            select_rag_driver()?
         } else {
-                "yaml"
-            }
-        } else {
-            "yaml"
+            "yaml".to_string()
         };
         let reranker_model = app.rag_reranker_model.clone();
         let top_k = app.rag_top_k;
@@ -318,7 +297,7 @@ impl Rag {
                 graph_hops: Some(graph_hops),
             },
         );
-        data.driver = driver.to_string();
+        data.driver = driver;
         let mut rag = Self::create(app, name, save_path, data)?;
         let mut paths = doc_paths.to_vec();
         if paths.is_empty() {
@@ -586,19 +565,40 @@ impl Rag {
                 if data.vectors.is_empty() {
                     data.vectors = duck.read_all_vectors()?;
                 }
+                if data.vectors.is_empty() && !data.files.is_empty() {
+                    println!(
+                        "{} RAG '{name}' lists {} indexed file(s), but its vector store \
+                         '{}' holds no vectors, so every search will return nothing. A \
+                         duckdb RAG is two files: bring the .duckdb sidecar along with \
+                         the .yaml, or re-embed with `.rebuild rag`.",
+                        warning_text("WARNING:"),
+                        data.files.len(),
+                        db_path.display()
+                    );
+                }
                 // data.files is always populated for duckdb, so build_bm25() is the only
                 // path; there is no from-DuckDB fallback.
                 let bm25 = data.build_bm25();
                 (Box::new(duck), bm25)
             }
             "qdrant" => bail!(
-                "Qdrant RAGs cannot be constructed via Rag::create(); \
-                 use Rag::attach() or Rag::load_async() instead"
+                "RAG '{name}' uses driver 'qdrant' without `attached: true`. \
+                 Coyote can currently only READ a pre-existing Qdrant \
+                 collection — attach one with `.rag attach`. Writing to a \
+                 Coyote-owned Qdrant collection is not supported yet."
             ),
-            _ => {
+            "yaml" => {
                 let bm25 = data.build_bm25();
                 (Box::new(YamlProvider::from_data(&data)), bm25)
             }
+            // Explicitly NOT a catch-all falling through to yaml. A typo'd driver
+            // used to build a yaml store, pay to embed the whole corpus, persist
+            // the bad driver, and only fail on the NEXT run, leaving the RAG
+            // unusable without hand-editing the YAML.
+            other => bail!(
+                "Unknown RAG driver '{other}' for RAG '{name}'. \
+                 Valid drivers: yaml, duckdb, qdrant."
+            ),
         };
         let node_to_docs = data.knowledge_graph.build_node_to_docs();
         let embedding_model =
@@ -1169,12 +1169,7 @@ impl Rag {
         top_k: usize,
         rerank_model: Option<&str>,
     ) -> Result<Vec<(DocumentId, String)>> {
-        let vector_search_results = self.vector_search(query, top_k, 0.0).await?;
-        debug!("vector_search_results: {vector_search_results:?}",);
-        let vector_search_ids: Vec<DocumentId> =
-            vector_search_results.into_iter().map(|(v, _)| v).collect();
-
-        let keyword_search_results: Vec<(DocumentId, f32)> =
+        let keyword_leg = async {
             if self.provider.has_native_keyword_search() {
                 self.provider
                     .keyword_search(query, top_k)
@@ -1185,7 +1180,16 @@ impl Rag {
                     })
             } else {
                 self.keyword_search(query, top_k, 0.0)
+            }
         };
+        let (vector_search_results, keyword_search_results) =
+            tokio::join!(self.vector_search(query, top_k, 0.0), keyword_leg);
+
+        let vector_search_results = vector_search_results?;
+        debug!("vector_search_results: {vector_search_results:?}",);
+        let vector_search_ids: Vec<DocumentId> =
+            vector_search_results.into_iter().map(|(v, _)| v).collect();
+
         debug!("keyword_search_results: {keyword_search_results:?}",);
         let keyword_search_ids: Vec<DocumentId> =
             keyword_search_results.into_iter().map(|(v, _)| v).collect();
@@ -1842,6 +1846,27 @@ fn select_embedding_model(models: &[&Model]) -> Result<String> {
     Ok(result.value)
 }
 
+pub(crate) fn select_rag_driver() -> Result<String> {
+    let options = vec![
+        "yaml   — portable, in-memory HNSW; usable from several Coyote processes at once (default)",
+        "duckdb — persistent on-disk store; vectors and content survive restarts; HNSW approximate search.",
+    ];
+    let sel = Select::new("RAG storage driver:", options)
+        .with_starting_cursor(0)
+        .prompt()?;
+    if sel.starts_with("duckdb") {
+        println!(
+            "Note: several Coyote processes can query a duckdb RAG at the same time, \
+             but while one process is ingesting or rebuilding it the others cannot \
+             read it until that finishes. Changing its driver later means deleting \
+             and recreating the RAG."
+        );
+        Ok("duckdb".to_string())
+    } else {
+        Ok("yaml".to_string())
+    }
+}
+
 const EXTRACTOR_SKIP: &str = "Skip";
 
 fn select_extractor_model(app: &AppConfig) -> Result<Option<String>> {

src/rag/providers/qdrant.rs

@@ -9,6 +9,7 @@ use reqwest::{Client, Response, StatusCode};
 use serde_json::Value;
 use std::collections::HashMap;
 use std::sync::Arc;
+use url::{Host, Url};
 
 /// Marks a `DocumentId` that stands in for a point id Coyote cannot carry
 /// directly. Qdrant accepts UUID strings as point ids, and that is what
@@ -104,7 +105,7 @@ fn parse_search_hits(
             let score = pt["score"].as_f64()? as f32;
             Some((interner.document_id(&pt["id"])?, score))
         })
-        .filter(|(_, score)| *score > min_score)
+        .filter(|(_, score)| min_score <= 0.0 || *score > min_score)
         .collect())
 }
 
@@ -183,7 +184,22 @@ pub struct QdrantProvider {
 }
 
 impl QdrantProvider {
-    fn make_client(api_key: Option<&str>) -> Result<Client> {
+    fn skips_proxy(base_url: &str) -> bool {
+        let Ok(url) = Url::parse(base_url) else {
+            return false;
+        };
+        match url.host() {
+            Some(Host::Domain(name)) => {
+                name == "localhost" || name.ends_with(".localhost") || name.ends_with(".local")
+            }
+            Some(Host::Ipv4(ip)) => ip.is_loopback() || ip.is_private() || ip.is_link_local(),
+            // No stable is_unique_local, so fc00::/7 is matched directly.
+            Some(Host::Ipv6(ip)) => ip.is_loopback() || ip.segments()[0] & 0xfe00 == 0xfc00,
+            None => false,
+        }
+    }
+
+    fn make_client(base_url: &str, api_key: Option<&str>) -> Result<Client> {
         let mut headers = HeaderMap::new();
         if let Some(key) = api_key {
             let mut value =
@@ -191,10 +207,11 @@ impl QdrantProvider {
             value.set_sensitive(true);
             headers.insert("api-key", value);
         }
-        Client::builder()
-            .default_headers(headers)
-            .build()
-            .context("Failed to build reqwest client")
+        let mut builder = Client::builder().default_headers(headers);
+        if Self::skips_proxy(base_url) {
+            builder = builder.no_proxy();
+        }
+        builder.build().context("Failed to build reqwest client")
     }
 
     pub(crate) fn normalize_base_url(host: &str) -> String {
@@ -219,7 +236,7 @@ impl QdrantProvider {
         api_key: Option<&str>,
     ) -> Result<Value> {
         let base_url = Self::normalize_base_url(host);
-        let client = Self::make_client(api_key)?;
+        let client = Self::make_client(&base_url, api_key)?;
         let resp = client
             .get(format!("{base_url}/collections/{collection}"))
             .send()
@@ -237,7 +254,7 @@ impl QdrantProvider {
 
     pub async fn new(host: &str, collection: &str, api_key: Option<&str>) -> Result<Self> {
         let base_url = Self::normalize_base_url(host);
-        let client = Self::make_client(api_key)?;
+        let client = Self::make_client(&base_url, api_key)?;
         let resp = client
             .get(format!("{base_url}/collections/{collection}"))
             .send()
@@ -260,7 +277,7 @@ impl QdrantProvider {
 
     pub async fn list_collections(host: &str, api_key: Option<&str>) -> Result<Vec<String>> {
         let base_url = Self::normalize_base_url(host);
-        let client = Self::make_client(api_key)?;
+        let client = Self::make_client(&base_url, api_key)?;
         let resp = client
             .get(format!("{base_url}/collections"))
             .send()
@@ -310,7 +327,7 @@ impl QdrantProvider {
         api_key: Option<&str>,
     ) -> Result<Option<String>> {
         let base_url = Self::normalize_base_url(host);
-        let client = Self::make_client(api_key)?;
+        let client = Self::make_client(&base_url, api_key)?;
         let url = format!("{base_url}/collections/{collection}/points/scroll");
         let body = serde_json::json!({ "limit": 1, "with_payload": false });
 
@@ -353,7 +370,8 @@ impl RagProvider for QdrantProvider {
         // `score_threshold` is deliberately NOT sent. It is metric-aware: on Cosine
         // collections 0.0 means "no floor" as expected, but Euclid collections score
         // by negative distance, where 0.0 filters everything out. The attach wizard
-        // does not pin the distance metric, so filter locally instead.
+        // does not pin the distance metric, so filter locally instead; i.e. where a
+        // 0.0 floor is correctly treated as "no floor" (see `parse_search_hits`).
         let body = serde_json::json!({
             "vector": embedding,
             "limit": top_k,
@@ -576,6 +594,73 @@ mod tests {
         assert!(provider.fetch_content(&[]).await.unwrap().is_empty());
     }
 
+    #[test]
+    fn local_and_private_hosts_skip_the_proxy() {
+        for host in [
+            "http://localhost:6333",
+            "http://127.0.0.1:6333",
+            "http://192.168.0.56:6333",
+            "http://10.1.2.3:6333",
+            "http://172.16.4.5:6333",
+            "http://qdrant.local:6333",
+            "http://[::1]:6333",
+        ] {
+            assert!(
+                QdrantProvider::skips_proxy(host),
+                "{host} should not be proxied"
+            );
+        }
+    }
+
+    #[test]
+    fn public_hosts_still_honour_the_environment() {
+        for host in [
+            "https://qdrant.example.com",
+            "http://8.8.8.8:6333",
+            "https://xyz.eu-central.aws.cloud.qdrant.io:6333",
+            "http://172.32.0.1:6333",
+        ] {
+            assert!(
+                !QdrantProvider::skips_proxy(host),
+                "{host} must keep the environment's proxy"
+            );
+        }
+    }
+
+    /// Euclid collections score by NEGATIVE distance, so the 0.0 the caller
+    /// passes must mean "no floor". Filtering on it drops every hit — the exact
+    /// bug that keeps Qdrant's own `score_threshold` off the wire.
+    #[test]
+    fn a_zero_floor_keeps_negative_euclid_scores() {
+        let mut interner = PointIdInterner::default();
+        let search = serde_json::json!({
+            "result": [
+                {"id": 1, "score": -0.12},
+                {"id": 2, "score": -8.5},
+            ]
+        });
+
+        let hits = parse_search_hits(&mut interner, &search, 0.0).unwrap();
+
+        assert_eq!(hits.len(), 2, "a 0.0 floor must not drop negative scores");
+    }
+
+    #[test]
+    fn a_positive_floor_still_filters() {
+        let mut interner = PointIdInterner::default();
+        let search = serde_json::json!({
+            "result": [
+                {"id": 1, "score": 0.9},
+                {"id": 2, "score": 0.2},
+            ]
+        });
+
+        let hits = parse_search_hits(&mut interner, &search, 0.5).unwrap();
+
+        assert_eq!(hits.len(), 1);
+        assert_eq!(hits[0].0, DocumentId(1));
+    }
+
     /// A UUID-keyed collection has to survive the whole `vector_search` →
     /// `fetch_content` round trip, and the fetch must ask Qdrant for the ORIGINAL
     /// string id. Parsing ids with `as_u64()` used to drop these hits inside a

src/sandbox/mod.rs

@@ -337,21 +337,51 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<()>
         if !data.attached {
             continue;
         }
-        let Some(placeholder) = data.driver_config.get("api_key") else {
+        let secret_names = driver_config_secret_names(&data);
+        let Some((primary, extra)) = secret_names.split_first() else {
             continue;
         };
+
         let service_id = mcp_credentials::secret_service_id(&stem);
-        if service_id.is_empty() || registered.contains(&service_id) {
+        if !service_id.is_empty() && !registered.contains(&service_id) {
+            bind_rag_secret(vault, &service_id, primary, &stem)?;
+        }
+
+        for name in extra {
+            let id = mcp_credentials::secret_service_id(name);
+            if !id.is_empty() && !registered.contains(&id) {
+                bind_rag_secret(vault, &id, name, &stem)?;
+            }
+        }
+    }
+
+    Ok(())
+}
+
+fn driver_config_secret_names(data: &RagData) -> Vec<String> {
+    let mut names: Vec<String> = Vec::new();
+    for value in data.driver_config.values() {
+        let trimmed = value.trim();
+        let Ok(Some(caps)) = SECRET_RE.captures(trimmed) else {
+            continue;
+        };
+        if caps.get(0).map(|m| m.as_str()) != Some(trimmed) {
             continue;
         }
-        let secret_name = placeholder
-            .trim_start_matches("{{")
-            .trim_end_matches("}}")
-            .trim();
+        let Some(name) = caps.get(1).map(|m| m.as_str().trim()) else {
+            continue;
+        };
+        if !name.is_empty() && !names.iter().any(|n| n == name) {
+            names.push(name.to_string());
+        }
+    }
+    names
+}
 
+fn bind_rag_secret(vault: &Vault, service_id: &str, secret_name: &str, stem: &str) -> Result<()> {
     match vault.get_secret(secret_name, false) {
         Ok(secret_value) => {
-                sbx_secret_set(&service_id, &secret_value)
+            sbx_secret_set(service_id, &secret_value)
                 .context("Failed to register RAG secret with sbx")?;
         }
         Err(e) => {
@@ -362,8 +392,6 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<()>
             );
         }
     }
-    }
-
     Ok(())
 }
 
@@ -646,6 +674,64 @@ fn chown_agent_recursive(sandbox: &str, path: &str) -> Result<()> {
 #[cfg(test)]
 mod tests {
     use super::*;
+
+    fn rag_with(driver_config: &[(&str, &str)]) -> RagData {
+        let mut data = RagData::new("m".into(), 1024, 50, None, 5, None, Default::default());
+        data.driver = "qdrant".to_string();
+        data.attached = true;
+        for (k, v) in driver_config {
+            data.driver_config.insert(k.to_string(), v.to_string());
+        }
+        data
+    }
+
+    #[test]
+    fn secret_names_are_found_whatever_the_field_is_called() {
+        let data = rag_with(&[
+            ("host", "qdrant.example.com:6333"),
+            ("collection", "docs"),
+            ("token", "{{SOME_TOKEN}}"),
+        ]);
+
+        assert_eq!(driver_config_secret_names(&data), vec!["SOME_TOKEN"]);
+    }
+
+    #[test]
+    fn a_literal_credential_is_not_treated_as_a_secret_name() {
+        let data = rag_with(&[("api_key", "sk-a-real-looking-key")]);
+
+        assert!(driver_config_secret_names(&data).is_empty());
+    }
+
+    #[test]
+    fn plain_values_are_never_mistaken_for_secrets() {
+        let data = rag_with(&[("host", "localhost:6333"), ("collection", "docs")]);
+
+        assert!(driver_config_secret_names(&data).is_empty());
+    }
+
+    #[test]
+    fn a_partial_placeholder_is_not_a_credential() {
+        let data = rag_with(&[("api_key", "Bearer {{KEY}}")]);
+
+        assert!(driver_config_secret_names(&data).is_empty());
+    }
+
+    #[test]
+    fn several_secrets_are_all_found_and_deduped() {
+        let data = rag_with(&[
+            ("api_key", "{{QDRANT_KEY}}"),
+            ("host", "localhost:6333"),
+            ("token", "{{ OTHER_TOKEN }}"),
+            ("fallback_key", "{{QDRANT_KEY}}"),
+        ]);
+
+        assert_eq!(
+            driver_config_secret_names(&data),
+            vec!["QDRANT_KEY", "OTHER_TOKEN"],
+            "order follows driver_config, and a repeat is not registered twice"
+        );
+    }
     use std::time::{SystemTime, UNIX_EPOCH};
 
     #[test]