From 74bc613d943632cba0a64d1f46d7691d221ba80a Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Tue, 11 Aug 2026 21:04:21 -0600 Subject: [PATCH] fix(rag): address Copilot review findings on the driver abstraction Five review comments, all real: - hybrid_search ran its vector and keyword legs sequentially after the provider refactor; main ran them under tokio::join!. Restores the concurrency while keeping the degrade-on-error keyword behaviour, so a remote provider no longer pays two serial round trips per query. - inject_rag_secrets derived a vault secret name by trimming braces, which leaves a literal key untouched. A RAG holding a plaintext api_key therefore looked the secret up by its own value and printed it to stderr on failure. Parsing is now strict and a non-placeholder is skipped with a warning that names no credential. - validate() now refuses a driver_config.api_key that is not a {{NAME}} placeholder, so a plaintext key cannot reach the RAG YAML at all. - Rag::create's catch-all arm treated any unrecognised driver as yaml. A typo built a yaml store, paid to embed the corpus, persisted the bad driver and only failed on the next run. Unknown drivers now fail immediately. - The qdrant arm's error was written for a developer; it now tells the user that only attached collections are readable and points at .rag attach. --- src/rag/mod.rs | 137 +++++++++++++++++++++++++++++++++++++++++---- src/sandbox/mod.rs | 20 +++++-- 2 files changed, 142 insertions(+), 15 deletions(-) diff --git a/src/rag/mod.rs b/src/rag/mod.rs index ac1ba52..8522e05 100644 --- a/src/rag/mod.rs +++ b/src/rag/mod.rs @@ -592,13 +592,23 @@ impl Rag { (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 +1179,12 @@ impl Rag { top_k: usize, rerank_model: Option<&str>, ) -> Result> { - 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 = - vector_search_results.into_iter().map(|(v, _)| v).collect(); - - let keyword_search_results: Vec<(DocumentId, f32)> = + // The two legs run CONCURRENTLY. Both can be network round trips on a + // remote provider (embedding the query, then the vector search; a native + // keyword search), so awaiting them in sequence roughly doubles the + // latency of every hybrid query. The local BM25 branch is synchronous and + // simply runs inline inside the future. + let keyword_leg = async { if self.provider.has_native_keyword_search() { self.provider .keyword_search(query, top_k) @@ -1185,7 +1195,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 = + vector_search_results.into_iter().map(|(v, _)| v).collect(); + debug!("keyword_search_results: {keyword_search_results:?}",); let keyword_search_ids: Vec = keyword_search_results.into_iter().map(|(v, _)| v).collect(); @@ -1568,6 +1587,23 @@ impl RagData { } } + // An api_key must be a `{{NAME}}` reference, never the credential itself. + // Two reasons, both load-bearing: a literal key here gets committed to the + // RAG YAML in plaintext, and sandbox provisioning parses this value back + // out to learn which vault secret to bind, so a literal one silently + // provisions nothing (and used to be echoed to stderr on failure). + if let Some(api_key) = self.driver_config.get("api_key") + && placeholder_secret_name(api_key).is_none() + { + bail!( + "driver_config.api_key must be a secret placeholder of the form \ + '{{{{NAME}}}}', not a literal key. Store the credential with \ + `coyote --add-secret ` and reference it by name; a literal \ + key would be written to this RAG's YAML in plaintext and cannot \ + be provisioned into the sandbox." + ); + } + match (self.driver.as_str(), self.attached) { ("yaml", false) => Ok(()), ("duckdb", false) => Ok(()), @@ -2177,6 +2213,21 @@ fn resolve_driver_config( }) } +/// The secret NAME inside a `{{NAME}}` placeholder, or `None` for anything else. +/// +/// Deliberately strict, and shared with sandbox provisioning so both agree on +/// what a placeholder is. A RAG's `driver_config.api_key` is supposed to hold a +/// placeholder, never a credential, but nothing stops a hand-edited or older +/// config from holding the literal key. Consumers report failures *by name*, so +/// treating a literal value as a name leaks the credential into stderr and logs. +pub(crate) fn placeholder_secret_name(value: &str) -> Option<&str> { + let inner = value.trim().strip_prefix("{{")?.strip_suffix("}}")?.trim(); + if inner.is_empty() || inner.contains(['{', '}']) { + return None; + } + Some(inner) +} + /// Interpolation core, taking the resolver as an argument so it can be exercised /// without a vault. Mirrors `interpolate_secrets` / `interpolate_secrets_with`. fn resolve_driver_config_with( @@ -3239,6 +3290,72 @@ vectors: {} assert!(data.validate().is_ok()); } + /// A literal credential in `driver_config.api_key` is refused outright: it + /// would be persisted to the RAG YAML in plaintext, and sandbox + /// provisioning parses this field expecting a placeholder. + #[test] + fn ragdata_validate_rejects_a_literal_api_key() { + let mut data = RagData::new( + "m".into(), + 1024, + 50, + None, + 5, + None, + GraphRagConfig::default(), + ); + data.driver = "qdrant".to_string(); + data.attached = true; + data.driver_config + .insert("api_key".to_string(), "sk-a-real-looking-key".to_string()); + + let err = data.validate().unwrap_err().to_string(); + + assert!(err.contains("must be a secret placeholder"), "got: {err}"); + assert!( + !err.contains("sk-a-real-looking-key"), + "the error must never echo the credential back: {err}" + ); + } + + #[test] + fn ragdata_validate_accepts_a_placeholder_api_key() { + let mut data = RagData::new( + "m".into(), + 1024, + 50, + None, + 5, + None, + GraphRagConfig::default(), + ); + data.driver = "qdrant".to_string(); + data.attached = true; + data.driver_config + .insert("api_key".to_string(), "{{QDRANT_KEY}}".to_string()); + + assert!(data.validate().is_ok()); + } + + /// The parser is the single thing standing between a hand-edited literal key + /// and a "could not load secret ''" line in the user's terminal. + #[test] + fn placeholder_secret_name_accepts_only_well_formed_placeholders() { + assert_eq!(placeholder_secret_name("{{NAME}}"), Some("NAME")); + assert_eq!(placeholder_secret_name(" {{ NAME }} "), Some("NAME")); + + // Every one of these used to survive the old trim_matches unchanged and + // then be used as a secret name. + assert_eq!(placeholder_secret_name("sk-literal-key"), None); + assert_eq!(placeholder_secret_name(""), None); + assert_eq!(placeholder_secret_name("{{}}"), None); + assert_eq!(placeholder_secret_name("{{ }}"), None); + assert_eq!(placeholder_secret_name("{{A}}{{B}}"), None); + assert_eq!(placeholder_secret_name("prefix{{NAME}}"), None); + assert_eq!(placeholder_secret_name("{{NAME"), None); + assert_eq!(placeholder_secret_name("NAME}}"), None); + } + #[test] fn ragdata_validate_rejects_zero_top_k_from_a_truncated_yaml() { let yaml = " diff --git a/src/sandbox/mod.rs b/src/sandbox/mod.rs index 94c9889..e97fcb1 100644 --- a/src/sandbox/mod.rs +++ b/src/sandbox/mod.rs @@ -19,7 +19,7 @@ use crate::config::AppConfig; use crate::config::Config; use crate::config::VAULT_DATA_FILE_NAME; use crate::config::paths; -use crate::rag::RagData; +use crate::rag::{RagData, placeholder_secret_name}; use crate::sandbox::mcp_credentials::MCP_MIXIN_NAME; use crate::sandbox::mixins::DiscoveredMixin; use crate::utils::run_command_with_output; @@ -344,10 +344,20 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet) -> Result<()> if service_id.is_empty() || registered.contains(&service_id) { continue; } - let secret_name = placeholder - .trim_start_matches("{{") - .trim_end_matches("}}") - .trim(); + // A literal key must NOT be mistaken for a secret NAME. The trims that + // used to stand here leave a non-placeholder value completely untouched, + // so the vault lookup below would run with the credential as the "name" + // and the warning would then print that credential to stderr. + let Some(secret_name) = placeholder_secret_name(placeholder) else { + eprintln!( + "Warning: RAG '{stem}' has a driver_config.api_key that is not a \ + secret placeholder, so no credential can be provisioned to the \ + sandbox and queries to this RAG will fail inside it. Store the \ + key with `coyote --add-secret `, then set api_key to the \ + matching placeholder in the RAG YAML." + ); + continue; + }; match vault.get_secret(secret_name, false) { Ok(secret_value) => {