Dark-Alex-17/coyote
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix(rag): address Copilot review findings on the driver abstraction

Browse Source 
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.
This commit is contained in:
Dark-Alex-17
2026-08-11 21:04:21 -06:00
parent 6d0a5550fe
commit 74bc613d94
2 changed files with 142 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/rag/mod.rs
+127 -10
View File
@@ -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<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)> =
// 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<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();
@@ -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 <NAME>` 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<F>(
@@ -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 '<the key>'" 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 = "
src/sandbox/mod.rs
+15 -5
View File
@@ -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<String>) -> 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 <NAME>`, then set api_key to the \
matching placeholder in the RAG YAML."
);
continue;
};
match vault.get_secret(secret_name, false) {
Ok(secret_value) => {