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

style: Cleaned up some minor styling issues

Browse Source
This commit is contained in:
Dark-Alex-17
2026-08-11 13:04:27 -06:00
parent 3e598065f8
commit ecda258d3a
15 changed files with 331 additions and 567 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/config/paths.rs
+2 -23
View File
@@ -414,11 +414,10 @@ pub fn list_rags() -> Vec<String> {
for entry in rd.flatten() {
let name = entry.file_name();
if let Some(name) = name.to_string_lossy().strip_suffix(".yaml") {
// Sidecars are not RAGs. `.duckdb` files are already excluded by
// the `.yaml` suffix check above; this rejects `<name>.sbx-mixin`.
if is_rag_sidecar_name(name) {
continue;
}
names.push(name.to_string());
}
}
@@ -429,24 +428,10 @@ pub fn list_rags() -> Vec<String> {
}
}
/// True for the sidecar YAML files that must never be listed or deleted as RAGs.
/// `name` is the already-stripped stem (i.e. after `strip_suffix(".yaml")`).
/// Uses `ends_with`, not `contains('.')`, so a RAG legitimately named "v2.docs" is
/// not rejected.
pub(crate) fn is_rag_sidecar_name(name: &str) -> bool {
name.ends_with(".sbx-mixin")
}
/// Remove every sidecar belonging to RAG `name` in `dir`. Missing files are NOT an
/// error. A failure to remove an EXISTING mixin IS an error and must propagate — a
/// silently-orphaned mixin keeps a sandbox network permission alive after the user
/// believes it is gone. The `.duckdb` orphan is only wasted disk, so its removal
/// failure is ignorable; the asymmetry is deliberate.
///
/// Callers must run this BEFORE unlinking the primary `.yaml`. If the YAML goes first
/// and this then fails, the RAG disappears from `list_rags()` — so the user can no
/// longer select it to retry — while its network allow entry keeps being injected
/// into every sandbox launch.
pub(crate) fn remove_rag_sidecars(dir: &Path, name: &str) -> Result<()> {
let duckdb_path = dir.join(format!("{name}.duckdb"));
if duckdb_path.exists() {
@@ -463,6 +448,7 @@ pub(crate) fn remove_rag_sidecars(dir: &Path, name: &str) -> Result<()> {
)
})?;
}
Ok(())
}
@@ -889,8 +875,6 @@ mod tests {
let _ = fs::remove_dir_all(&root);
}
/// Unique temp dir for the sidecar helper tests. These take `dir: &Path` directly,
/// so no env-var mutation and therefore no `#[serial]` is needed.
fn sidecar_temp_dir(label: &str) -> PathBuf {
let unique = time::SystemTime::now()
.duration_since(time::UNIX_EPOCH)
@@ -903,7 +887,6 @@ mod tests {
#[test]
fn is_rag_sidecar_name_accepts_dotted_rag_names() {
// A RAG legitimately named "v2.docs" must not be mistaken for a sidecar.
assert!(!is_rag_sidecar_name("v2.docs"));
assert!(!is_rag_sidecar_name("myrag"));
assert!(is_rag_sidecar_name("myrag.sbx-mixin"));
@@ -940,8 +923,6 @@ mod tests {
let root = sidecar_temp_dir("rag-sidecars-order");
let yaml = root.join("docs.yaml");
fs::write(&yaml, "rag").unwrap();
// A non-empty DIRECTORY at the mixin path makes remove_file fail, standing in
// for any real removal failure (permissions, a busy mount).
let mixin = root.join("docs.sbx-mixin.yaml");
fs::create_dir_all(&mixin).unwrap();
fs::write(mixin.join("blocker"), "x").unwrap();
@@ -952,8 +933,6 @@ mod tests {
.contains("Failed to remove the sandbox mixin"),
"got: {err}"
);
// The whole point of removing sidecars first: the RAG is still on disk, still
// listed, and the deletion is retryable.
assert!(
yaml.exists(),
"the .yaml must survive a sidecar-removal failure so the delete is retryable"
src/config/request_context.rs
+2 -24
View File
@@ -142,11 +142,6 @@ pub struct RequestContext {
pub role: Option<Role>,
pub session: Option<Session>,
pub rag: Option<Arc<Rag>>,
/// The cache key `self.rag` was actually inserted under, carried rather than
/// reconstructed. Reconstruction was the bug: the invalidation sites do not have
/// the information needed to rebuild the key (agent RAGs are inserted under the
/// AGENT's name but `rag.name()` is the constant "rag"), so insert and invalidate
/// silently disagreed. `None` for the temp RAG, which bypasses the cache entirely.
pub rag_key: Option<RagKey>,
pub agent: Option<Agent>,
@@ -4122,14 +4117,10 @@ impl RequestContext {
}
let app = self.app.config.clone();
// Hoisted: `rag_cache` below borrows `self`, so the loader closure cannot
// reach through `self` for the vault. `GlobalVault` is an Arc, so this is cheap.
let vault = self.app.vault.clone();
let rag_cache = self.rag_cache();
let working_mode = self.working_mode;
// The key is returned alongside the Rag rather than assigned inside the match:
// `rag_cache` borrows `self`, so writing `self.rag_key` there is E0506.
let (rag, rag_key): (Arc<Rag>, Option<RagKey>) = match rag {
None => {
let rag_path = self.rag_file(super::TEMP_RAG_NAME);
@@ -4138,7 +4129,6 @@ impl RequestContext {
format!("Failed to cleanup previous '{}' rag", super::TEMP_RAG_NAME)
})?;
}
// The temp RAG is never inserted into the cache, so it has no key.
(
Arc::new(
Rag::init(
@@ -4198,13 +4188,9 @@ impl RequestContext {
let vault = self.app.vault.clone();
let rag = Rag::attach(app, &vault, name, &rag_path).await?;
let rag = Arc::new(rag);
// Populate the cache so a later `.rag <name>` reuses this instance rather
// than re-running the network preflight. Attach is always a global RAG.
let key = RagKey::Named(name.to_string());
self.rag_cache().insert(key.clone(), &rag);
self.rag = Some(rag);
// Carried so invalidation in rebuild_rag()/edit_rag_docs() can find this
// entry; without it a stale Arc would linger in the cache all session.
self.rag_key = Some(key);
Ok(())
}
@@ -4217,7 +4203,7 @@ impl RequestContext {
if rag.is_attached() {
bail!(
"Cannot edit documents on an attached RAG — Coyote does not own its source documents."
"Cannot edit documents on an attached RAG; Coyote does not own its source documents."
);
}
@@ -4270,7 +4256,7 @@ impl RequestContext {
if rag.is_attached() {
bail!(
"Cannot rebuild an attached RAG — Coyote does not own its source documents. \
"Cannot rebuild an attached RAG; Coyote does not own its source documents. \
Re-index from the system that originally created '{}'.",
rag.name()
);
@@ -4716,8 +4702,6 @@ mod tests {
)
.unwrap();
// Stand in for the state `.rag docs` leaves behind: `use_rag` sets `rag` and
// `rag_key` together, so a named key is live when the agent is entered.
ctx.rag_key = Some(RagKey::Named("docs".to_string()));
tokio::runtime::Builder::new_current_thread()
@@ -4730,9 +4714,6 @@ mod tests {
.unwrap();
});
// This agent has no RAG, so `rag` is None and `rag_key` must be None as well.
// Carrying `Named("docs")` across the transition would point `.rebuild rag`
// at an unrelated RAG's cache entry.
assert!(ctx.rag.is_none());
assert_eq!(ctx.rag_key, None);
}
@@ -6122,9 +6103,6 @@ mod tests {
assert!(paths::list_rags().is_empty());
}
/// A `<name>.sbx-mixin.yaml` sidecar must not appear as a phantom RAG in TAB
/// completion or `.list rag`. A RAG whose name legitimately contains a dot must
/// still be listed — the filter uses `ends_with`, not `contains('.')`.
#[test]
#[serial]
fn list_rags_skips_sbx_mixin_sidecars() {
src/rag/mod.rs
+53 -95
View File
@@ -12,9 +12,6 @@ mod splitter;
use self::graph::{KnowledgeGraph, extract_entities};
use self::provider::RagProvider;
// `providers::duckdb_path_from_yaml(path)` is called through the module path in
// `create()`, so `providers` itself must stay in scope — do not collapse it into
// the `use` below.
use self::providers::{DuckDbProvider, QdrantProvider, YamlProvider};
use crate::sandbox::mcp_credentials;
use crate::vault::{Vault, interpolate_secrets};
@@ -65,10 +62,7 @@ pub struct Rag {
name: String,
path: String,
embedding_model: Model,
// Local BM25: keyword search + graph seeding. Always built from `data.files`
// regardless of driver, and kept on `Rag` so the sync `graph_search` can use it.
bm25: SearchEngine<DocumentId>,
// Vector storage + content retrieval.
provider: Box<dyn RagProvider>,
data: RagData,
last_sources: RwLock<Option<String>>,
@@ -126,8 +120,7 @@ pub struct RagInitConfig {
pub extractor_model: Option<String>,
pub extractor_prompt: Option<String>,
pub graph_hops: Option<usize>,
/// `None` -> "yaml". No serde attribute: this struct derives only
/// `Debug, Clone, Default` and is built in Rust, never deserialized.
/// `None` -> "yaml"
pub driver: Option<String>,
}
@@ -367,8 +360,6 @@ impl Rag {
// plaintext.
let data: RagData = serde_yaml::from_str(&raw_content).with_context(err)?;
// Validated before the match so the rule applies to every driver, including
// the sync fallthrough below.
data.validate().with_context(err)?;
match data.driver.as_str() {
@@ -384,7 +375,6 @@ impl Rag {
.context("qdrant driver requires 'collection' in driver_config")?
.clone();
// Resolved out of band and kept in a local; it never enters `data`.
let api_key: Option<String> = match data.driver_config.get("api_key") {
Some(placeholder) => {
let (resolved, _) =
@@ -411,14 +401,10 @@ impl Rag {
last_sources: RwLock::new(None),
})
}
// yaml/duckdb take the sync path. It re-reads and re-parses the file;
// that cost is accepted to keep every existing caller untouched.
_ => Self::load(app, name, path),
}
}
/// Connects to a pre-existing external collection. Coyote is a query-only
/// client here: it never indexes documents into it.
pub async fn attach(
app: &AppConfig,
vault: &Vault,
@@ -435,8 +421,6 @@ impl Rag {
let host = Text::new("Host (e.g. qdrant.company.com:6333):")
.with_validator(required!("This field is required"))
.with_validator(|input: &str| {
// Bracketed IPv6 literals would produce a malformed sandbox
// network allow entry, so refuse them at the prompt.
Ok(if input.contains('[') || input.contains(']') {
Validation::Invalid(
"Bracketed IPv6 literals are not supported; use a hostname.".into(),
@@ -583,7 +567,6 @@ impl Rag {
Ok(rag)
}
/// `mut data` — the duckdb arm rehydrates `data.vectors` from the sidecar.
pub fn create(app: &AppConfig, name: &str, path: &Path, mut data: RagData) -> Result<Self> {
// Deliberately does NOT call rebuild_indexes: both callers construct the Rag
// before any documents are added, so rebuilding empty data would be a no-op.
@@ -603,11 +586,11 @@ impl Rag {
// Guarded on is_empty() so a caller that already has vectors in memory
// is never overwritten by an empty table.
//
// 🔴 `?`, NOT `unwrap_or_default()`. A hydration failure must propagate.
// Degrading to an empty map here loads a RAG that looks healthy, answers
// every query with nothing, and then loses the store permanently on the
// first `.edit rag-docs`. The legitimate "nothing indexed yet" case is
// already Ok(empty) — open() runs CREATE TABLE IF NOT EXISTS — so `?`
// WARNING: `?`, NOT `unwrap_or_default()`. A hydration failure must
// propagate. Degrading to an empty map here loads a RAG that looks healthy,
// answers every query with nothing, and then loses the store permanently
// on the first `.edit rag-docs`. The legitimate "nothing indexed yet" case
// is already Ok(empty) (open() runs CREATE TABLE IF NOT EXISTS) so `?`
// costs a new RAG nothing.
if data.vectors.is_empty() {
data.vectors = duck.read_all_vectors()?;
@@ -622,7 +605,6 @@ impl Rag {
use Rag::attach() or Rag::load_async() instead"
),
_ => {
// "yaml" and any unknown driver — in-memory HNSW.
let bm25 = data.build_bm25();
(Box::new(YamlProvider::from_data(&data)), bm25)
}
@@ -730,7 +712,7 @@ impl Rag {
// `data.files` is empty for an attached RAG; the local index is not the
// source of truth. A static label is honest, an empty list is not.
*self.last_sources.write() =
Some("[attached RAG — source list unavailable]".to_string());
Some("[Using attached RAG. Source list unavailable]".to_string());
return;
}
let mut sources: IndexMap<String, Vec<String>> = IndexMap::new();
@@ -922,6 +904,7 @@ impl Rag {
if self.data.attached {
return format!("- {}", self.data.attached_source_label());
}
let mut seen = IndexSet::new();
for id in ids {
let (file_index, _) = id.split();
@@ -1202,20 +1185,13 @@ impl Rag {
let keyword_search_results: Vec<(DocumentId, f32)> =
if self.provider.has_native_keyword_search() {
// Keyword is ONE of three RRF rankers (vector + keyword + graph);
// its absence is survivable and produces a slightly worse ranking,
// whereas a `?` here turns a provider FTS fault into TOTAL query
// failure — the user gets an error instead of the results the
// vector and graph rankers already retrieved. Degrade, do not
// propagate, and do not silently swap in the local BM25 either:
// that would change the ranking algorithm mid-query.
match self.provider.keyword_search(query, top_k).await {
Ok(v) => v,
Err(e) => {
self.provider
.keyword_search(query, top_k)
.await
.unwrap_or_else(|e| {
warn!("native keyword search failed, dropping the keyword ranker: {e}");
Vec::new()
}
}
})
} else {
self.keyword_search(query, top_k, 0.0)
};
@@ -1231,8 +1207,6 @@ impl Rag {
.concat()
.into_iter()
.collect();
// `ids` is an `IndexSet` here, not the `Vec` of the RRF branch below,
// and `&IndexSet<_>` does not coerce to `&[DocumentId]`.
let ids: Vec<DocumentId> = ids.into_iter().collect();
let fetched = self.provider.fetch_content(&ids).await?;
// Build both vectors from the SAME source in the SAME iteration —
@@ -1276,8 +1250,6 @@ impl Rag {
ids
}
};
// `ids` is the ranked list; `fetch_content` preserves that order per the
// trait's ordering contract, so the result is returned as-is.
let output = self.provider.fetch_content(&ids).await?;
Ok(output)
}
@@ -1308,7 +1280,7 @@ impl Rag {
Ok(merge_vector_results(results))
}
/// Local in-memory BM25 over `data.files` — empty for attached RAGs, which is
/// Local in-memory BM25 over `data.files`. This is empty for attached RAGs, which is
/// correct: they have no local text.
fn keyword_search(&self, query: &str, top_k: usize, min_score: f32) -> Vec<(DocumentId, f32)> {
let results = self.bm25.search(query, top_k);
@@ -1479,9 +1451,6 @@ pub struct RagData {
pub driver: String,
#[serde(default)]
pub attached: bool,
/// Driver-specific connection parameters (qdrant: `host`, `collection`, `api_key`).
/// Secret-bearing values are stored as `{{SECRET_NAME}}` placeholders and resolved
/// out of band at load time, so a resolved credential never reaches disk.
#[serde(default, skip_serializing_if = "IndexMap::is_empty")]
pub driver_config: IndexMap<String, String>,
@@ -1589,6 +1558,7 @@ impl RagData {
no results with no error. Set `top_k:` in the RAG YAML."
);
}
if !self.attached {
if self.chunk_size == 0 {
bail!(
@@ -1597,6 +1567,7 @@ impl RagData {
embedding batches. Set `chunk_size:` in the RAG YAML."
);
}
if self.chunk_overlap >= self.chunk_size {
bail!(
"chunk_overlap ({}) must be strictly less than chunk_size ({}).",
@@ -1605,6 +1576,7 @@ impl RagData {
);
}
}
match (self.driver.as_str(), self.attached) {
("yaml", false) => Ok(()),
("duckdb", false) => Ok(()),
@@ -1626,7 +1598,7 @@ impl RagData {
/// Every (DocumentId, &RagDocument) in the corpus, in `files` order.
///
/// This — NOT `vectors` — is the authoritative document id space. BM25, the
/// This, NOT `vectors`, is the authoritative document id space. BM25, the
/// knowledge graph and content lookup all key off it; `vectors` is a subset,
/// since `add`'s zip truncates whenever fewer embeddings come back than
/// document ids were sent.
@@ -1783,9 +1755,6 @@ fn generate_rag_sbx_mixin(
header_name: &str,
value_format: &str,
) -> Result<()> {
// The client reaches the store through `normalize_base_url`, so deriving the
// allow entry from that same URL keeps the whitelist and the actual dialled
// port from drifting apart.
let base_url = QdrantProvider::normalize_base_url(host);
let Some(allow_entry) = mcp_credentials::allow_entry_for_url(&base_url) else {
eprintln!(
@@ -1793,6 +1762,7 @@ fn generate_rag_sbx_mixin(
grammar, so no sandbox mixin was written for RAG '{service_name}'. \
Queries to this RAG will be blocked inside the sandbox."
);
return Ok(());
};
@@ -1826,12 +1796,11 @@ fn generate_rag_sbx_mixin(
mixin_path.display()
)
})?;
println!("✓ Sandbox mixin: '{}'.", mixin_path.display());
Ok(())
}
/// Bearer credentials are spelled as a `scheme`, everything else as an explicit
/// `header`; the two are mutually exclusive in the inject grammar.
fn rag_inject_rule(
domain: &str,
header_name: &str,
@@ -1856,8 +1825,6 @@ fn rag_inject_rule(
}
}
/// Derives a deterministic env var name from a RAG name:
/// `company-docs` → `COMPANY_DOCS_API_KEY`.
fn rag_env_var_name(rag_name: &str) -> String {
format!(
"{}_API_KEY",
@@ -1865,8 +1832,6 @@ fn rag_env_var_name(rag_name: &str) -> String {
)
}
/// How a driver authenticates its HTTP requests. Qdrant uses a bare `api-key`
/// header rather than `Authorization: Bearer`.
fn driver_auth_header(driver: &str) -> (&'static str, &'static str) {
match driver {
"qdrant" => ("api-key", "%s"),
@@ -2104,8 +2069,6 @@ fn find_hash_skip(
/// so the pool is bounded by `top_k * query_chunks`, and `reciprocal_rank_fusion`
/// truncates to `top_k` itself. Capping here would let whichever query chunk has
/// the strongest absolute scores crowd out every other chunk's hits.
///
/// Free function (not a method) so it is unit-testable without an embeddings client.
fn merge_vector_results(mut results: Vec<(DocumentId, f32)>) -> Vec<(DocumentId, f32)> {
debug_assert!(
results.iter().all(|(_, score)| score.is_finite()),
@@ -2161,16 +2124,17 @@ fn embedding_dim_for_model(model_id: &str) -> usize {
#[cfg(test)]
mod tests {
use super::*;
use std::path::PathBuf;
use std::time::{SystemTime, UNIX_EPOCH};
/// Scratch directory for tests that must write a real file.
struct TempDir {
path: std::path::PathBuf,
path: PathBuf,
}
impl TempDir {
fn new(tag: &str) -> Self {
let unique = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
let unique = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap()
.as_nanos();
let path = env::temp_dir().join(format!("coyote-rag-{tag}-{unique}"));
@@ -2272,10 +2236,6 @@ mod tests {
.collect()
}
/// The generated mixin must carry the kit v2 schema envelope:
/// `wrap_mixin_as_kit` copies it verbatim to `spec.yaml` for
/// `sbx create --kit`, with no `kind` rewrite and no validation on Coyote's
/// side, so a stale envelope breaks the launch itself.
#[test]
fn generated_sbx_mixin_carries_the_schema_envelope() {
let (text, parsed) = render_rag_mixin(
@@ -2290,14 +2250,11 @@ mod tests {
text.starts_with("schemaVersion:"),
"envelope must come first:\n{text}"
);
// Quoted string, never the numeric 2 — the kit rejects an int here.
assert_eq!(parsed["schemaVersion"].as_str(), Some("2"));
assert_eq!(parsed["kind"].as_str(), Some("mixin"));
assert_eq!(parsed["name"].as_str(), Some("rag-company-docs"));
assert!(parsed["description"].as_str().is_some());
// https/443 is the only bare-host case; 6333 must carry its port, and
// the bare host must NOT also be listed.
assert_eq!(allow_list(&parsed), vec!["rag.example.com:6333"]);
let credential = &parsed["credentials"][0];
@@ -2313,14 +2270,11 @@ mod tests {
assert_eq!(inject["header"].as_str(), Some("api-key"));
assert_eq!(inject["format"].as_str(), Some("%s"));
// sbx does not derive allow entries from inject rules; an inject
// domain that is not allowed is a dead rule.
assert!(
allow_list(&parsed).contains(&inject["domain"].as_str().unwrap().to_string()),
"every inject domain must also appear in allow:\n{text}"
);
// The v1 vocabulary is gone, not merely unused.
for dead in ["allowedDomains", "serviceDomains", "serviceAuth"] {
assert!(!text.contains(dead), "v1 key '{dead}' survived:\n{text}");
}
@@ -2334,8 +2288,6 @@ mod tests {
);
}
/// The allow entry has to name the port the client actually dials, which is
/// whatever `normalize_base_url` resolves to — not a Qdrant-specific guess.
#[test]
fn generated_sbx_mixin_allows_the_port_the_client_dials() {
let cases = [
@@ -2361,8 +2313,6 @@ mod tests {
}
}
/// `Authorization: Bearer` is spelled as a scheme; `header` and `scheme` are
/// mutually exclusive in the inject grammar.
#[test]
fn generated_sbx_mixin_spells_bearer_as_a_scheme() {
let (_, parsed) = render_rag_mixin(
@@ -2372,16 +2322,14 @@ mod tests {
"Authorization",
"Bearer %s",
);
let inject = &parsed["credentials"][0]["apiKey"]["inject"][0];
assert_eq!(inject["scheme"].as_str(), Some("bearer"));
assert!(inject["header"].is_null());
assert!(inject["format"].is_null());
}
/// The bind in `inject_rag_secrets` and the `service` declared here both run
/// the RAG name through `secret_service_id`. If they disagreed, the proxy
/// would hold a value under one id and an inject rule under another, and the
/// header would never be rewritten.
#[test]
fn generated_sbx_mixin_service_id_matches_the_host_side_bind() {
let (_, parsed) = render_rag_mixin(
@@ -2391,9 +2339,10 @@ mod tests {
"api-key",
"%s",
);
assert_eq!(
parsed["credentials"][0]["service"].as_str(),
Some(crate::sandbox::mcp_credentials::secret_service_id("My_Docs").as_str())
Some(mcp_credentials::secret_service_id("My_Docs").as_str())
);
assert_eq!(
parsed["credentials"][0]["service"].as_str(),
@@ -2401,12 +2350,11 @@ mod tests {
);
}
/// A store with no API key still needs egress, but declaring a credential
/// nothing ever binds would leave sbx waiting on a binding that never comes.
#[test]
fn generated_sbx_mixin_omits_credentials_when_there_is_no_api_key() {
let (text, parsed) =
render_rag_mixin("https://store.example.com", "docs", None, "api-key", "%s");
assert_eq!(allow_list(&parsed), vec!["store.example.com"]);
assert!(
parsed["credentials"].is_null(),
@@ -2423,7 +2371,6 @@ mod tests {
#[test]
fn driver_auth_header_uses_a_bare_api_key_for_qdrant() {
// Qdrant's REST API reads `api-key`, NOT `Authorization: Bearer`.
assert_eq!(driver_auth_header("qdrant"), ("api-key", "%s"));
assert_eq!(
driver_auth_header("something-else"),
@@ -2431,8 +2378,6 @@ mod tests {
);
}
/// An attached RAG has no local `files`, so the citation helpers would
/// otherwise emit "unknown" and an empty source list for every result.
#[test]
fn attached_rag_citation_helpers_do_not_fall_back_to_the_empty_file_index() {
let mut data = RagData {
@@ -2446,7 +2391,6 @@ mod tests {
.insert("collection".into(), "company-kb".into());
assert!(data.files.is_empty());
// The real helper — `resolve_source`/`format_sources` both delegate here.
assert_eq!(
data.attached_source_label(),
"[external collection: company-kb]"
@@ -2580,6 +2524,7 @@ mod tests {
.iter_documents()
.map(|(id, doc)| (id, doc.page_content.as_str()))
.collect();
assert_eq!(
documents,
vec![
@@ -2600,12 +2545,10 @@ mod tests {
None,
GraphRagConfig::default(),
);
assert_eq!(data.iter_documents().count(), 0);
}
/// The document id space is `files`, never `vectors`: `add`'s zip truncates
/// silently, so a vector may exist for an id no file provides. Content lookup
/// and BM25 both key off this iterator and must agree.
#[test]
fn rag_data_iter_documents_ignores_vector_only_ids() {
let mut data = RagData::new(
@@ -2876,14 +2819,17 @@ mod tests {
#[test]
fn merge_vector_results_empty_input() {
let result = super::merge_vector_results(vec![]);
let result = merge_vector_results(vec![]);
assert!(result.is_empty(), "empty input should produce empty output");
}
#[test]
fn merge_vector_results_keeps_best_score_per_document() {
let doc = DocumentId::new(0, 0);
let result = super::merge_vector_results(vec![(doc, 0.2), (doc, 0.9)]);
let result = merge_vector_results(vec![(doc, 0.2), (doc, 0.9)]);
assert_eq!(result.len(), 1, "a document must not be double-counted");
assert_eq!(result[0].0, doc);
assert_eq!(
@@ -2897,10 +2843,10 @@ mod tests {
let doc_a = DocumentId::new(0, 0);
let doc_b = DocumentId::new(1, 0);
let doc_c = DocumentId::new(2, 0);
// Interleaved as two per-chunk hit lists would arrive: concatenating them
// would yield a, c, b — only a global sort produces c, a, b.
let result = super::merge_vector_results(vec![(doc_a, 0.5), (doc_c, 0.9), (doc_b, 0.1)]);
let result = merge_vector_results(vec![(doc_a, 0.5), (doc_c, 0.9), (doc_b, 0.1)]);
let ids: Vec<DocumentId> = result.iter().map(|(id, _)| *id).collect();
assert_eq!(ids, vec![doc_c, doc_a, doc_b]);
}
@@ -2909,7 +2855,9 @@ mod tests {
let input: Vec<(DocumentId, f32)> = (0..10)
.map(|i| (DocumentId::new(i, 0), i as f32 / 10.0))
.collect();
let result = super::merge_vector_results(input);
let result = merge_vector_results(input);
assert_eq!(
result.len(),
10,
@@ -2935,6 +2883,7 @@ mod tests {
#[test]
fn force_reingest_re_embeds_hash_identical_files() {
let (files, to_deleted) = hash_skip_fixture();
assert_eq!(
find_hash_skip(true, &to_deleted, &files, "abc", "test.txt"),
None,
@@ -2945,6 +2894,7 @@ mod tests {
#[test]
fn refresh_without_force_still_hash_skips() {
let (files, to_deleted) = hash_skip_fixture();
assert_eq!(
find_hash_skip(false, &to_deleted, &files, "abc", "test.txt"),
Some((0, 7)),
@@ -2955,6 +2905,7 @@ mod tests {
#[test]
fn find_hash_skip_returns_none_on_path_change() {
let (files, to_deleted) = hash_skip_fixture();
assert_eq!(
find_hash_skip(false, &to_deleted, &files, "abc", "moved.txt"),
None
@@ -2976,6 +2927,7 @@ mod tests {
None,
GraphRagConfig::default(),
);
assert_eq!(data.driver, "yaml");
assert!(!data.attached);
}
@@ -2992,7 +2944,9 @@ document_paths: []
files: {}
vectors: {}
";
let data: RagData = serde_yaml::from_str(yaml).unwrap();
assert_eq!(data.driver, "yaml");
assert!(!data.attached);
}
@@ -3013,6 +2967,7 @@ vectors: {}
let yaml = serde_yaml::to_string(&data).unwrap();
let restored: RagData = serde_yaml::from_str(&yaml).unwrap();
assert_eq!(restored.driver, "qdrant");
assert!(restored.attached);
}
@@ -3029,7 +2984,9 @@ vectors: {}
GraphRagConfig::default(),
);
data.attached = true;
let err = data.validate().unwrap_err().to_string();
assert!(err.contains("cannot be attached"), "got: {err}");
}
@@ -3046,6 +3003,7 @@ vectors: {}
);
data.driver = "qdrant".to_string();
data.attached = true;
assert!(data.validate().is_ok());
}
src/rag/provider.rs
+7 -18
View File
@@ -4,11 +4,6 @@ use async_trait::async_trait;
/// Abstracts where RAG vector data is stored and queried.
///
/// Implementors:
/// - YamlProvider: HNSW in-memory, state derived from RagData.vectors/files
/// - DuckDbProvider: DuckDB on-disk vector index + document store
/// - QdrantProvider: remote Qdrant collection
///
/// The Rag orchestrator owns: embeddings, chunking, BM25 keyword search, graph RAG,
/// entity extraction, RRF merging. Providers own: vector storage and content retrieval.
#[async_trait]
@@ -26,7 +21,7 @@ pub trait RagProvider: Send + Sync {
///
/// **Ordering contract:** implementations MUST return results in the same
/// relative order as the input `ids` slice. `hybrid_search` passes an
/// RRF-ranked list and feeds the result straight to the LLM — a provider
/// RRF-ranked list and feeds the result straight to the LLM. A provider
/// that returns rows in storage order (e.g. Qdrant `get_points`, DuckDB
/// `WHERE id IN (...)`) would silently discard the ranking. Implementations
/// that query an unordered backend must re-sort by input position before
@@ -43,34 +38,28 @@ pub trait RagProvider: Send + Sync {
/// Called once at the end of every sync_documents pass.
///
/// `full_rebuild` mirrors `sync_documents`' `refresh` parameter:
/// - `true` — a full re-index (`.rebuild rag`, `--rebuild-rag`, initial build).
/// - `true`: a full re-index (`.rebuild rag`, `--rebuild-rag`, initial build).
/// Destructive strategies (wipe-then-reindex) are permitted.
/// - `false` — an incremental change (`.edit rag-docs` adding/removing a file).
/// - `false`: an incremental change (`.edit rag-docs` adding/removing a file).
/// Implementations MUST NOT wipe existing state; upsert only.
///
/// The parameter is part of the signature from the outset so it is fixed
/// while there is exactly one implementor. Yaml/DuckDb ignore it —
/// while there is exactly one implementor. Yaml/DuckDb ignore it,
/// rebuilding their local state wholesale is fast and always correct.
/// Only a remote provider is destructive enough to care.
///
/// YamlProvider: rebuilds HNSW + content map from data.vectors/files.
/// DuckDbProvider: writes new rows to DuckDB, deletes removed rows.
/// QdrantProvider: no-op while attach-only — remote data is unchanged.
async fn rebuild_indexes(&mut self, data: &RagData, full_rebuild: bool) -> Result<()>;
/// Keyword / full-text search. Returns (DocumentId, BM25-style score) sorted desc.
///
/// Default impl returns `Ok(vec![])` — callers fall back to `Rag.bm25` (local in-memory
/// BM25 built from `data.files`). DuckDbProvider overrides this with a native FTS query
/// (DuckDB's `fts` extension, installed once at schema-creation time).
/// Default impl returns `Ok(vec![])`. Callers fall back to `Rag.bm25` (local in-memory
/// BM25 built from `data.files`).
///
/// Callers check `has_native_keyword_search()` before deciding which path to take:
/// - true → call this method; skip `Rag.bm25`
/// - false → call `Rag.keyword_search()` which uses `Rag.bm25` (sync, infallible)
///
/// YamlProvider and QdrantProvider do NOT override this (return empty).
async fn keyword_search(&self, query: &str, top_k: usize) -> Result<Vec<(DocumentId, f32)>> {
let _ = (query, top_k);
Ok(vec![])
}
src/rag/providers/duckdb.rs
+56 -134
View File
@@ -1,9 +1,11 @@
use crate::rag::provider::RagProvider;
use crate::rag::{DocumentId, RagData};
use std::collections::HashMap;
use anyhow::{Context, Result, anyhow, bail};
use async_trait::async_trait;
use duckdb::Connection;
use duckdb::types::Value;
use indexmap::IndexMap;
use log::warn;
use std::path::{Path, PathBuf};
@@ -136,9 +138,6 @@ impl DuckDbProvider {
pub(crate) fn read_all_vectors(&self) -> Result<IndexMap<DocumentId, Vec<f32>>> {
let conn = self.lock_conn()?;
let mut stmt = conn.prepare("SELECT doc_id, embedding FROM vectors")?;
// Collect into a Result, NOT a filter_map. `.filter_map(|r| r.ok())` here would
// turn a systematic decode failure (e.g. a schema written by a different duckdb
// version) into a silently short map, indistinguishable from an empty store.
let raw: Vec<(u64, Vec<f32>)> = stmt
.query_map([], |row| {
let id: u64 = row.get(0)?;
@@ -146,12 +145,12 @@ impl DuckDbProvider {
// column is read as a `Value` and destructured. A FLOAT[N] column yields
// `Value::Array`, a FLOAT[] column yields `Value::List`; match both so
// the reader survives a file written under either schema.
let embedding: Vec<f32> = match row.get::<_, duckdb::types::Value>(1)? {
duckdb::types::Value::Array(vals) | duckdb::types::Value::List(vals) => vals
let embedding: Vec<f32> = match row.get::<_, Value>(1)? {
Value::Array(vals) | Value::List(vals) => vals
.into_iter()
.map(|v| match v {
duckdb::types::Value::Float(f) => f,
duckdb::types::Value::Double(d) => d as f32,
Value::Float(f) => f,
Value::Double(d) => d as f32,
_ => f32::NAN,
})
.collect(),
@@ -184,6 +183,7 @@ impl DuckDbProvider {
}
out.insert(DocumentId(id as usize), embedding);
}
Ok(out)
}
@@ -192,11 +192,10 @@ impl DuckDbProvider {
///
/// A schema-existence check alone is NOT sufficient. After
/// `CREATE OR REPLACE TABLE documents`, the `fts_main_documents` schema still
/// exists and `match_bm25` still SUCCEEDS — but returns zero rows for every term.
/// exists and `match_bm25` still SUCCEEDS, but returns zero rows for every term.
/// The index is silently dead. The probe therefore asserts that a KNOWN row comes
/// back rather than merely that the call did not error.
fn probe_fts_index(conn: &Connection) -> bool {
// 1. Structural check — cheap, and short-circuits a never-built index.
let schema_exists = conn
.query_row(
"SELECT COUNT(*) FROM duckdb_schemas() WHERE schema_name = 'fts_main_documents'",
@@ -208,11 +207,6 @@ impl DuckDbProvider {
if !schema_exists {
return false;
}
// 2. Liveness check — pull a real token out of a real row and confirm the index
// scores that same row. A live index returns >= 1; a stale one returns 0
// without erroring. An empty `documents` table cannot be probed, and
// reporting false is correct: there is nothing to keyword-search, and the
// next rebuild_indexes sets the flag directly.
conn.query_row(
"SELECT COUNT(*) FROM documents d
WHERE fts_main_documents.match_bm25(
@@ -250,7 +244,6 @@ impl RagProvider for DuckDbProvider {
top_k: usize,
min_score: f32,
) -> Result<Vec<(DocumentId, f32)>> {
// Validate before building the SQL literal — a NaN would produce malformed SQL.
if embedding.iter().any(|f| !f.is_finite()) {
bail!("Query embedding contains a non-finite value (NaN or infinity)");
}
@@ -276,11 +269,11 @@ impl RagProvider for DuckDbProvider {
let id: u64 = row.get(0)?;
// `array_cosine_distance` on a FLOAT[N] column returns FLOAT (f32), NOT
// DOUBLE. Reading it as f64 raises InvalidColumnType INSIDE the closure,
// which a bare `.filter_map(|r| r.ok())` would silently discard —
// which a bare `.filter_map(|r| r.ok())` would silently discard,
// yielding ZERO results with no error and no log line.
let distance: f32 = match row.get::<_, duckdb::types::Value>(1)? {
duckdb::types::Value::Float(f) => f,
duckdb::types::Value::Double(d) => d as f32,
let distance: f32 = match row.get::<_, Value>(1)? {
Value::Float(f) => f,
Value::Double(d) => d as f32,
other => {
warn!("unexpected distance type from DuckDB: {other:?}");
return Err(duckdb::Error::InvalidQuery);
@@ -299,6 +292,7 @@ impl RagProvider for DuckDbProvider {
})
.filter(|(_, score)| *score > min_score)
.collect();
Ok(results)
}
@@ -310,10 +304,7 @@ impl RagProvider for DuckDbProvider {
let placeholders = ids.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
let sql =
format!("SELECT doc_id, page_content FROM documents WHERE doc_id IN ({placeholders})");
let params: Vec<duckdb::types::Value> = ids
.iter()
.map(|id| duckdb::types::Value::UBigInt(id.0 as u64))
.collect();
let params: Vec<Value> = ids.iter().map(|id| Value::UBigInt(id.0 as u64)).collect();
let mut stmt = conn.prepare(&sql)?;
let mut rows: Vec<(DocumentId, String)> = stmt
.query_map(duckdb::params_from_iter(params.iter()), |row| {
@@ -329,22 +320,21 @@ impl RagProvider for DuckDbProvider {
}
})
.collect();
// Ordering contract: `WHERE doc_id IN (...)` returns rows in storage order, NOT
// in the order of `ids`. Since `ids` is the RRF-ranked list, returning storage
// order would silently discard the ranking. Re-sort by input position.
let position: std::collections::HashMap<DocumentId, usize> =
let position: HashMap<DocumentId, usize> =
ids.iter().enumerate().map(|(i, id)| (*id, i)).collect();
rows.sort_by_key(|(id, _)| position.get(id).copied().unwrap_or(usize::MAX));
Ok(rows)
}
async fn rebuild_indexes(&mut self, data: &RagData, _full_rebuild: bool) -> Result<()> {
// Local on-disk state — a wholesale table reset plus re-INSERT is always
// Local on-disk state. A wholesale table reset plus re-INSERT is always
// correct, so the incremental/full distinction is ignored.
//
// 🔴 ANTI-WIPE GUARD. The CREATE OR REPLACE TABLE below writes exactly what
// `data.vectors` holds. An empty map on a RAG that HAS indexed files, against a
// store that ALREADY holds vectors, is always a bug — a failed/skipped
// store that ALREADY holds vectors, is always a bug; a failed/skipped
// hydration, or a caller that emptied the live map. Refuse rather than commit
// the loss. Every legitimate empty-vector rebuild also has an empty `files`
// (a fresh RAG; the zero-document tests), so this cannot fire on a correct call.
@@ -353,7 +343,7 @@ impl RagProvider for DuckDbProvider {
if data.vectors.is_empty() && !data.files.is_empty() {
let existing: i64 = {
// Scoped: the guard MUST be dropped before `lock_conn()` is taken again
// below. `Mutex` is not reentrant — holding both self-deadlocks at
// below. `Mutex` is not reentrant; holding both self-deadlocks at
// runtime, with no compile error.
let conn = self.lock_conn()?;
conn.query_row("SELECT count(*) FROM vectors", [], |r| r.get(0))
@@ -382,7 +372,6 @@ impl RagProvider for DuckDbProvider {
}
}
let dim = self.dim;
// `Connection::transaction()` takes `&mut self`, so this binding must be `mut`.
let mut conn = self.lock_conn()?;
let tx = conn
.transaction()
@@ -408,7 +397,7 @@ impl RagProvider for DuckDbProvider {
{
// Plain INSERT: tables are empty after CREATE OR REPLACE, so no PK conflict.
// The embedding is bound as TEXT and cast in SQL — the duckdb crate's
// The embedding is bound as TEXT and cast in SQL. The duckdb crate's
// `bind_parameter` has no arm for List/Array, so binding a vector directly
// fails at runtime with "binding List parameters is not yet supported".
let mut vstmt = tx.prepare(&format!(
@@ -418,12 +407,12 @@ impl RagProvider for DuckDbProvider {
tx.prepare("INSERT INTO documents (doc_id, page_content) VALUES (?, ?)")?;
// 🔴 TWO INDEPENDENT LOOPS. `documents` is keyed on `files`, `vectors` on
// `vectors` — they are DIFFERENT key sets and neither is a subset of the
// `vectors`. They are DIFFERENT key sets and neither is a subset of the
// other. Do not merge these into one loop over `&data.vectors` gated on a
// lookup into `files`: that produces keys(documents) ⊆ keys(vectors), and
// every id in `files \ vectors` (which `RagData::add`'s zip truncation
// really does produce) becomes a live, rankable id from BM25 and
// graph_search that resolves to nothing — no error, no log line.
// graph_search that resolves to nothing; no error, no log line.
for (id, doc) in data.iter_documents() {
dstmt.execute(duckdb::params![id.0 as u64, doc.page_content.as_str()])?;
}
@@ -461,7 +450,7 @@ impl RagProvider for DuckDbProvider {
// Rebuild the FTS index (must be outside the transaction). This rebuild is
// MANDATORY on every pass, not an optimization: CREATE OR REPLACE TABLE above
// leaves the old fts_main_documents schema in place but DEAD — match_bm25 keeps
// leaves the old fts_main_documents schema in place but DEAD; match_bm25 keeps
// succeeding while returning zero rows for every term.
// GUARDED on a non-empty table: FTS cannot index an empty table, and
// rebuild_indexes is legitimately called with zero documents (a fresh RAG, and
@@ -480,7 +469,7 @@ impl RagProvider for DuckDbProvider {
// Live only if an index was actually built. With zero documents there is no FTS
// index, so `has_native_keyword_search()` must stay false and hybrid_search must
// fall back to local BM25 — which is also empty, and therefore correct.
// fall back to local BM25, which is also empty, and therefore correct.
self.fts_ready.store(doc_count > 0, Ordering::Relaxed);
Ok(())
@@ -501,11 +490,11 @@ impl RagProvider for DuckDbProvider {
let id: u64 = row.get(0)?;
// Same hazard as vector_search's distance column: guessing the width
// wrong raises InvalidColumnType INSIDE the closure, which a bare
// `.filter_map(|r| r.ok())` silently discards — yielding ZERO keyword
// `.filter_map(|r| r.ok())` silently discards, yielding ZERO keyword
// hits, indistinguishable from "the query matched nothing".
let score: f32 = match row.get::<_, duckdb::types::Value>(1)? {
duckdb::types::Value::Double(d) => d as f32,
duckdb::types::Value::Float(f) => f,
let score: f32 = match row.get::<_, Value>(1)? {
Value::Double(d) => d as f32,
Value::Float(f) => f,
other => {
warn!("unexpected match_bm25 score type from DuckDB: {other:?}");
return Err(duckdb::Error::InvalidQuery);
@@ -543,7 +532,7 @@ impl RagProvider for DuckDbProvider {
// This means DuckDbProvider clones are NOT independent snapshots: state lives on
// disk and a rebuild through one handle is immediately visible to the other.
// That is unavoidable for any on-disk store and is handled by the discipline
// documented on `Rag`'s Clone impl — the pre-clone instance must be discarded.
// documented on `Rag`'s Clone impl, the pre-clone instance must be discarded.
Box::new(DuckDbProvider {
path: self.path.clone(),
conn: Arc::clone(&self.conn),
@@ -556,44 +545,37 @@ impl RagProvider for DuckDbProvider {
#[cfg(test)]
mod tests {
use super::*;
// Trait methods are only callable with the trait in scope.
use crate::rag::provider::RagProvider;
// `RagFile` / `RagDocument` have private fields; struct-literal construction
// compiles only because this module is a descendant of `crate::rag`. They are
// deliberately not in the non-test `use` block — unused there, and `--deny warnings`
// rejects that.
use crate::rag::{RagDocument, RagFile};
use std::time::{SystemTime, UNIX_EPOCH};
use std::{env, fs};
/// Unique temp path per test. `tempfile` is not a dev-dependency; this mirrors the
/// house pattern used elsewhere in the tree.
struct TempDb {
path: PathBuf,
}
impl TempDb {
fn new(tag: &str) -> Self {
let unique = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
let unique = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap()
.as_nanos();
let path = std::env::temp_dir().join(format!("coyote-duckdb-{tag}-{unique}.duckdb"));
let path = env::temp_dir().join(format!("coyote-duckdb-{tag}-{unique}.duckdb"));
Self { path }
}
}
impl Drop for TempDb {
fn drop(&mut self) {
let _ = std::fs::remove_file(&self.path);
// DuckDB writes a `<path>.wal` sidecar; remove it too or /tmp accumulates
// one per test run.
let _ = std::fs::remove_file(self.path.with_extension("duckdb.wal"));
let _ = fs::remove_file(&self.path);
let _ = fs::remove_file(self.path.with_extension("duckdb.wal"));
}
}
/// ⚠️ NOTE THE EMPTY `files`. This fixture describes a RAG with ZERO documents.
/// Since `rebuild_indexes` populates the `documents` table from
/// `data.iter_documents()` (keyed on `files`), any test built on this helper alone
/// exercises the `vectors` table and NOTHING ELSE — no `documents` rows, no FTS
/// exercises the `vectors` table and NOTHING ELSE. No `documents` rows, no FTS
/// index. That is correct for the rebuild tests below, and is exactly why the
/// FTS-flag test and the `fetch_content` tests use `populated_rag_data()` instead.
/// Do not "simplify" them back onto this helper.
@@ -609,7 +591,7 @@ mod tests {
}
}
/// Two files, one document each — the minimum fixture that produces `documents`
/// Two files, one document each; the minimum fixture that produces `documents`
/// rows.
fn populated_rag_data() -> RagData {
let mut data = minimal_rag_data();
@@ -644,12 +626,14 @@ mod tests {
let db = TempDb::new("schema");
let provider = DuckDbProvider::open(&db.path, 3).unwrap();
let conn = provider.conn.lock().unwrap();
let v: i64 = conn
.query_row("SELECT COUNT(*) FROM vectors", [], |r| r.get(0))
.unwrap();
let d: i64 = conn
.query_row("SELECT COUNT(*) FROM documents", [], |r| r.get(0))
.unwrap();
assert_eq!(v, 0);
assert_eq!(d, 0);
}
@@ -668,10 +652,12 @@ mod tests {
)
.unwrap();
}
let results = provider
.vector_search(&[0.1, 0.2, 0.3], 5, 0.0)
.await
.unwrap();
assert_eq!(results.len(), 1);
assert_eq!(results[0].0.0, 0);
assert!(results[0].1 > 0.99);
@@ -689,7 +675,9 @@ mod tests {
)
.unwrap();
}
let results = provider.fetch_content(&[DocumentId(42)]).await.unwrap();
assert_eq!(results.len(), 1);
assert_eq!(results[0].1, "hello world");
}
@@ -700,19 +688,15 @@ mod tests {
let mut provider = DuckDbProvider::open(&db.path, 3).unwrap();
let data = minimal_rag_data();
provider.rebuild_indexes(&data, true).await.unwrap();
let results = provider
.vector_search(&[0.1, 0.2, 0.3], 5, 0.0)
.await
.unwrap();
assert!(results.is_empty()); // no vectors in empty data
assert!(results.is_empty());
}
/// Rebuilding TWICE must succeed.
///
/// The first rebuild always passes because the tables start empty. The bug this
/// guards against — `DELETE FROM` plus re-INSERT of the same PKs inside one
/// transaction — only fires on the SECOND rebuild, with "Duplicate key ... violates
/// primary key constraint". A single-rebuild test cannot catch it.
#[tokio::test]
async fn rebuild_indexes_is_idempotent_across_two_passes() {
let db = TempDb::new("idempotent");
@@ -722,7 +706,7 @@ mod tests {
data.vectors.insert(DocumentId(1), vec![0.4, 0.5, 0.6]);
provider.rebuild_indexes(&data, true).await.unwrap();
// Second pass over the SAME doc_ids — this is the assertion that matters.
provider
.rebuild_indexes(&data, true)
.await
@@ -735,27 +719,16 @@ mod tests {
assert_eq!(count, 2, "rebuild must replace rows, not duplicate them");
}
/// Guards the incremental data-loss bug: `sync_documents` hash-skips unchanged
/// files, so on an incremental pass `data.vectors` holds ONLY the newly embedded
/// chunks. If the live map is ever emptied, the `CREATE OR REPLACE TABLE` in
/// rebuild_indexes writes only those, destroying every previously indexed vector.
/// Hydration via `read_all_vectors()` is what prevents it.
///
/// This simulates the real restart-then-edit sequence: build, drop the in-memory
/// map, rehydrate from disk, add one vector, rebuild incrementally.
#[tokio::test]
async fn rebuild_indexes_preserves_prior_vectors_on_incremental_pass() {
let db = TempDb::new("incremental");
let mut provider = DuckDbProvider::open(&db.path, 3).unwrap();
// Pass 1 — initial full build with two vectors.
let mut data = minimal_rag_data();
data.vectors.insert(DocumentId(0), vec![0.1, 0.2, 0.3]);
data.vectors.insert(DocumentId(1), vec![0.4, 0.5, 0.6]);
provider.rebuild_indexes(&data, true).await.unwrap();
// Simulate a restart: the YAML on disk carries NO vectors, so a fresh RagData
// starts empty. create() hydrates it back from the sidecar.
let mut reloaded = minimal_rag_data();
assert!(
reloaded.vectors.is_empty(),
@@ -768,7 +741,6 @@ mod tests {
"hydration must restore both vectors"
);
// Pass 2 — incremental add of ONE new chunk.
reloaded.vectors.insert(DocumentId(2), vec![0.7, 0.8, 0.9]);
provider.rebuild_indexes(&reloaded, false).await.unwrap();
@@ -783,14 +755,6 @@ mod tests {
);
}
/// `has_native_keyword_search()` must be false before any FTS index exists,
/// otherwise hybrid_search routes into keyword_search and the `?` turns a DuckDB
/// catalog error into a total query failure.
///
/// 🔴 THE FIXTURE MUST CARRY DOCUMENTS. `rebuild_indexes` builds the FTS index only
/// when the `documents` table is non-empty, and that table is filled from
/// `data.iter_documents()` (keyed on `files`), never from `vectors`. With `files`
/// empty the pragma is skipped and `fts_ready` stores `false`.
#[tokio::test]
async fn keyword_search_is_not_advertised_before_first_rebuild() {
let db = TempDb::new("ftsflag");
@@ -800,13 +764,10 @@ mod tests {
"a freshly opened DB has no FTS index yet"
);
// 2 files × 1 document → 2 `documents` rows → doc_count > 0 → pragma runs.
let mut data = populated_rag_data();
data.vectors.insert(DocumentId(0), vec![0.1, 0.2, 0.3]);
provider.rebuild_indexes(&data, true).await.unwrap();
{
// Pin the precondition explicitly. If this ever reads 0 the assertion below
// is vacuous and the FTS hazard is unguarded again.
let conn = provider.conn.lock().unwrap();
let docs: i64 = conn
.query_row("SELECT COUNT(*) FROM documents", [], |r| r.get(0))
@@ -819,23 +780,11 @@ mod tests {
);
}
/// The `documents` table is keyed on `files`, NOT on `vectors`.
///
/// `fetch_content` is the sink for BOTH the BM25 and graph_search paths, and both
/// enumerate `files` via `iter_documents()`. If `rebuild_indexes` only inserts a
/// documents row for ids that also appear in `data.vectors`, every id in
/// `files \ vectors` resolves to nothing: healthy BM25 scores, healthy graph hits,
/// zero results, no error.
///
/// The incremental test CANNOT catch this — its fixture has an empty `files`, so the
/// documents table is empty in every assertion either way.
#[tokio::test]
async fn fetch_content_resolves_documents_without_vectors() {
let db = TempDb::new("docsnovec");
let mut provider = DuckDbProvider::open(&db.path, 3).unwrap();
// Two files, one document each — but a vector for ONLY THE FIRST. This is the
// `RagData::add` zip-truncation shape.
let mut data = populated_rag_data();
let id_a = DocumentId::new(0, 0);
let id_b = DocumentId::new(1, 0);
@@ -858,13 +807,6 @@ mod tests {
assert_eq!(got[1].1, "beta keyword");
}
/// `fetch_content` must return rows in INPUT order, not storage order.
///
/// `YamlProvider` satisfies this contract structurally — it maps over `ids` — so the
/// existing yaml test proves nothing about DuckDB. DuckDB's `WHERE doc_id IN (...)`
/// returns storage order and relies on an explicit positional re-sort, which is what
/// can actually regress. `ids` is the RRF-ranked list, so losing the order silently
/// discards the ranking while still returning the right documents.
#[tokio::test]
async fn duckdb_fetch_content_preserves_input_order() {
let db = TempDb::new("order");
@@ -872,11 +814,9 @@ mod tests {
let data = populated_rag_data();
provider.rebuild_indexes(&data, true).await.unwrap();
let id_a = DocumentId::new(0, 0); // inserted first → storage order 0
let id_b = DocumentId::new(1, 0); // inserted second → storage order 1
let id_a = DocumentId::new(0, 0);
let id_b = DocumentId::new(1, 0);
// REVERSED relative to storage order. Without the positional re-sort this
// returns ["alpha keyword", "beta keyword"] and the assertion fails.
let got = provider.fetch_content(&[id_b, id_a]).await.unwrap();
assert_eq!(got.len(), 2);
assert_eq!(got[0].0, id_b, "input order must win over storage order");
@@ -885,19 +825,11 @@ mod tests {
assert_eq!(got[1].1, "alpha keyword");
}
/// The anti-wipe guard.
///
/// `rebuild_indexes` does `CREATE OR REPLACE TABLE` and writes exactly what
/// `data.vectors` holds. An empty map on a RAG that HAS indexed files, against a
/// store that already holds vectors, is always a bug (failed hydration, or a caller
/// that emptied the live map) and must be refused rather than committed. The loss
/// would otherwise be permanent: nothing re-embeds it back.
#[tokio::test]
async fn rebuild_indexes_refuses_to_wipe_when_vectors_are_empty_but_files_are_not() {
let db = TempDb::new("nowipe");
let mut provider = DuckDbProvider::open(&db.path, 3).unwrap();
// A healthy store: two files with documents, two vectors.
let mut data = populated_rag_data();
data.vectors
.insert(DocumentId::new(0, 0), vec![0.1, 0.2, 0.3]);
@@ -905,8 +837,7 @@ mod tests {
.insert(DocumentId::new(1, 0), vec![0.4, 0.5, 0.6]);
provider.rebuild_indexes(&data, true).await.unwrap();
// Now the failure state: vectors lost in memory, files intact.
let broken = populated_rag_data(); // same files, NO vectors
let broken = populated_rag_data();
assert!(
broken.vectors.is_empty() && !broken.files.is_empty(),
"precondition"
@@ -918,7 +849,6 @@ mod tests {
"got: {err}"
);
// The store must be untouched — this is the whole point.
let conn = provider.conn.lock().unwrap();
let count: i64 = conn
.query_row("SELECT COUNT(*) FROM vectors", [], |r| r.get(0))
@@ -929,31 +859,23 @@ mod tests {
);
}
/// The guard must NOT fire on a legitimately empty RAG — otherwise a fresh
/// `Rag::init()` cannot complete. Empty `vectors` AND empty `files` is fine.
#[tokio::test]
async fn rebuild_indexes_allows_empty_vectors_when_files_are_also_empty() {
let db = TempDb::new("emptyok");
let mut provider = DuckDbProvider::open(&db.path, 3).unwrap();
let data = minimal_rag_data(); // no files, no vectors
let data = minimal_rag_data();
provider
.rebuild_indexes(&data, true)
.await
.expect("a fresh RAG with nothing indexed must rebuild cleanly");
}
/// `duplicate()` must clone the Arc, NOT call `open()` again.
///
/// This asserts SHARED state, which is the actual contract. It deliberately does NOT
/// use `fetch_content(&[])` — that early-returns before ever touching the
/// connection, so it would pass even against a broken double-opening implementation.
#[tokio::test]
async fn duplicate_shares_the_same_connection() {
let db = TempDb::new("dup");
let provider = DuckDbProvider::open(&db.path, 3).unwrap();
let dup = provider.duplicate(&minimal_rag_data());
// Write through the ORIGINAL...
{
let conn = provider.conn.lock().unwrap();
conn.execute(
@@ -962,8 +884,9 @@ mod tests {
)
.unwrap();
}
// ...and read it back through the DUPLICATE. Only possible if they share state.
let via_dup = dup.fetch_content(&[DocumentId(7)]).await.unwrap();
assert_eq!(
via_dup.len(),
1,
@@ -972,8 +895,6 @@ mod tests {
assert_eq!(via_dup[0].1, "shared row");
}
/// A decode failure must abort hydration rather than yield a thinned map: a short
/// map is what the next CREATE OR REPLACE commits as the new truth.
#[tokio::test]
async fn read_all_vectors_rejects_non_finite_embeddings() {
let db = TempDb::new("nonfinite");
@@ -1005,6 +926,7 @@ mod tests {
#[test]
fn duckdb_path_from_yaml_swaps_extension() {
let p = duckdb_path_from_yaml(Path::new("/tmp/rags/docs.yaml"));
assert_eq!(p, PathBuf::from("/tmp/rags/docs.duckdb"));
}
}
src/rag/providers/mod.rs
-7
View File
@@ -1,15 +1,8 @@
mod yaml;
// Use `self::` on every re-export in this file. Once a `mod duckdb;` sits here
// alongside a dependency on the `duckdb` CRATE, a bare `pub use duckdb::...`
// is ambiguous (E0659) — `use` paths resolve against both this module's items
// and the extern prelude, and `use` declarations may not shadow.
pub use self::yaml::YamlProvider;
mod duckdb;
pub use self::duckdb::DuckDbProvider;
// `create()` in rag/mod.rs derives the sidecar path through this. It is `pub(crate)`
// in providers/duckdb.rs, and the re-export must be `pub(crate)` too — a `pub use` of
// a `pub(crate)` item is E0364/E0365.
pub(crate) use self::duckdb::duckdb_path_from_yaml;
mod qdrant;
src/rag/providers/qdrant.rs
+46 -45
View File
@@ -3,6 +3,9 @@ use crate::rag::{DocumentId, RagData};
use anyhow::{Context, Result, bail};
use async_trait::async_trait;
use reqwest::header::{HeaderMap, HeaderValue};
use reqwest::{Client, Response, StatusCode};
use serde_json::Value;
use std::collections::HashMap;
/// Render Qdrant's error envelope into a human-readable message.
@@ -15,22 +18,18 @@ use std::collections::HashMap;
/// * routing-level 404s (a wrong HTTP verb) return an EMPTY body with no JSON at
/// all, which without the length check surfaces as "EOF while parsing a value"
/// instead of the actual 404.
fn format_error_body(status: reqwest::StatusCode, body: &str) -> String {
fn format_error_body(status: StatusCode, body: &str) -> String {
if body.is_empty() {
return format!("HTTP {status} (empty body — check the HTTP verb and path)");
}
serde_json::from_str::<serde_json::Value>(body)
serde_json::from_str::<Value>(body)
.ok()
.and_then(|v| v["status"]["error"].as_str().map(str::to_string))
.unwrap_or_else(|| format!("HTTP {status}: {body}"))
}
/// Read the vector dimension out of a parsed `GET /collections/{name}` response.
///
/// Unnamed collections put `size` directly under `vectors`; named ones nest it
/// under the vector's name. Both shapes occur in the wild, so try the flat one
/// first and fall back to the first named entry.
fn vector_dimension_from_collection(body: &serde_json::Value) -> Result<u64> {
fn vector_dimension_from_collection(body: &Value) -> Result<u64> {
let params = &body["result"]["config"]["params"];
params["vectors"]["size"]
.as_u64()
@@ -47,12 +46,12 @@ fn vector_dimension_from_collection(body: &serde_json::Value) -> Result<u64> {
/// (multi-vector) collection.
///
/// `vector_search` posts an unnamed vector, which a named-vector collection
/// rejects with HTTP 400 on every query — so attaching one yields a RAG that is
/// rejects with HTTP 400 on every query, so attaching one yields a RAG that is
/// silently 100% broken. A named collection holding a SINGLE vector is
/// structurally a map, identical in kind to the multi-named case, and rejects
/// the same way; testing for a numeric `size` directly under `vectors` catches
/// it, whereas counting keys (`len() > 1`) would wrongly accept it.
fn is_multi_vector_config(body: &serde_json::Value) -> bool {
fn is_multi_vector_config(body: &Value) -> bool {
body["result"]["config"]["params"]["vectors"]["size"]
.as_u64()
.is_none()
@@ -63,27 +62,21 @@ fn is_multi_vector_config(body: &serde_json::Value) -> bool {
/// Attach-only: this provider never writes to the remote collection. Coyote does
/// not own the data, and `rebuild_indexes` refuses rather than pretending to.
pub struct QdrantProvider {
/// `reqwest::Client` is Arc-backed, so `clone()` is O(1) and shares both the
/// connection pool and the `api-key` default header injected at build time.
client: reqwest::Client,
/// Includes the scheme, e.g. `http://qdrant.example.com:6333`.
client: Client,
base_url: String,
collection: String,
}
impl QdrantProvider {
/// The resolved API key is injected as a default header here and is
/// deliberately NOT stored on the struct: the plaintext value stays a local
/// of the caller and never outlives it.
fn make_client(api_key: Option<&str>) -> Result<reqwest::Client> {
let mut headers = reqwest::header::HeaderMap::new();
fn make_client(api_key: Option<&str>) -> Result<Client> {
let mut headers = HeaderMap::new();
if let Some(key) = api_key {
let mut value = reqwest::header::HeaderValue::from_str(key)
.context("api-key header value is not valid ASCII")?;
let mut value =
HeaderValue::from_str(key).context("api-key header value is not valid ASCII")?;
value.set_sensitive(true);
headers.insert("api-key", value);
}
reqwest::Client::builder()
Client::builder()
.default_headers(headers)
.build()
.context("Failed to build reqwest client")
@@ -97,7 +90,7 @@ impl QdrantProvider {
}
}
async fn error_message(resp: reqwest::Response) -> String {
async fn error_message(resp: Response) -> String {
let status = resp.status();
let body = resp.text().await.unwrap_or_default();
format_error_body(status, &body)
@@ -109,7 +102,7 @@ impl QdrantProvider {
host: &str,
collection: &str,
api_key: Option<&str>,
) -> Result<serde_json::Value> {
) -> Result<Value> {
let base_url = Self::normalize_base_url(host);
let client = Self::make_client(api_key)?;
let resp = client
@@ -123,13 +116,13 @@ impl QdrantProvider {
Self::error_message(resp).await
);
}
Ok(resp.json().await?)
}
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)?;
// Preflight: confirm the collection exists and we may read it.
let resp = client
.get(format!("{base_url}/collections/{collection}"))
.send()
@@ -141,6 +134,7 @@ impl QdrantProvider {
Self::error_message(resp).await
);
}
Ok(Self {
client,
base_url,
@@ -162,13 +156,15 @@ impl QdrantProvider {
Self::error_message(resp).await
);
}
let body: serde_json::Value = resp.json().await?;
let body: Value = resp.json().await?;
let names = body["result"]["collections"]
.as_array()
.context("Unexpected /collections response shape")?
.iter()
.filter_map(|v| v["name"].as_str().map(str::to_string))
.collect();
Ok(names)
}
@@ -178,6 +174,7 @@ impl QdrantProvider {
api_key: Option<&str>,
) -> Result<u64> {
let body = Self::fetch_collection(host, collection, api_key).await?;
vector_dimension_from_collection(&body)
}
@@ -187,11 +184,10 @@ impl QdrantProvider {
api_key: Option<&str>,
) -> Result<bool> {
let body = Self::fetch_collection(host, collection, api_key).await?;
Ok(is_multi_vector_config(&body))
}
/// Peek at one point to learn how its ID is typed. Returns the raw JSON
/// rendering, so a string ID comes back quoted and an integer one bare.
pub async fn sample_point_id(
host: &str,
collection: &str,
@@ -201,23 +197,27 @@ impl QdrantProvider {
let client = Self::make_client(api_key)?;
let url = format!("{base_url}/collections/{collection}/points/scroll");
let body = serde_json::json!({ "limit": 1, "with_payload": false });
let resp = client
.post(&url)
.json(&body)
.send()
.await
.with_context(|| format!("Failed to connect to {host}"))?;
if !resp.status().is_success() {
bail!(
"Failed to sample a point from '{collection}': {}",
Self::error_message(resp).await
);
}
let data: serde_json::Value = resp.json().await?;
let data: Value = resp.json().await?;
let id_val = data["result"]["points"]
.as_array()
.and_then(|pts| pts.first())
.map(|pt| pt["id"].to_string());
Ok(id_val)
}
}
@@ -251,7 +251,7 @@ impl RagProvider for QdrantProvider {
Self::error_message(resp).await
);
}
let data: serde_json::Value = resp.json().await?;
let data: Value = resp.json().await?;
let results = data["result"]
.as_array()
.context("Unexpected /points/search response shape")?
@@ -266,6 +266,7 @@ impl RagProvider for QdrantProvider {
})
.filter(|(_, score)| *score > min_score)
.collect();
Ok(results)
}
@@ -279,7 +280,9 @@ impl RagProvider for QdrantProvider {
"ids": id_list,
"with_payload": true,
});
let resp = self.client.post(&url).json(&body).send().await?;
if !resp.status().is_success() {
bail!(
"Qdrant point fetch on '{}' failed: {}",
@@ -287,7 +290,7 @@ impl RagProvider for QdrantProvider {
Self::error_message(resp).await
);
}
let data: serde_json::Value = resp.json().await?;
let data: Value = resp.json().await?;
let mut rows: Vec<(DocumentId, String)> = data["result"]
.as_array()
.context("Unexpected /points response shape")?
@@ -303,13 +306,14 @@ impl RagProvider for QdrantProvider {
let position: HashMap<DocumentId, usize> =
ids.iter().enumerate().map(|(i, id)| (*id, i)).collect();
rows.sort_by_key(|(id, _)| position.get(id).copied().unwrap_or(usize::MAX));
Ok(rows)
}
async fn rebuild_indexes(&mut self, data: &RagData, _full_rebuild: bool) -> Result<()> {
// Both arms refuse. A silent `Ok(())` would make `.rebuild rag` and
// `.edit rag-docs` look like they worked while writing nothing to the
// remote — leaving the user believing the collection was updated.
// remote, leaving the user believing the collection was updated.
if data.attached {
bail!(
"This RAG is attached to an external Qdrant collection. Coyote does not own \
@@ -340,7 +344,7 @@ mod tests {
fn error_message_reads_the_object_status_envelope() {
let body =
r#"{"status": {"error": "Wrong input: Not existing vector name error:"}, "time": 0.0}"#;
let msg = format_error_body(reqwest::StatusCode::BAD_REQUEST, body);
let msg = format_error_body(StatusCode::BAD_REQUEST, body);
assert!(msg.contains("Not existing vector name"), "got: {msg}");
assert!(
!msg.contains("EOF"),
@@ -350,15 +354,14 @@ mod tests {
#[test]
fn error_message_survives_the_string_status_and_the_empty_body() {
// Success envelope: `status` is a bare string, so the object lookup misses
// and we must fall back rather than panic or invent an error text.
let ok = format_error_body(reqwest::StatusCode::OK, r#"{"status": "ok", "time": 0.0}"#);
let ok = format_error_body(StatusCode::OK, r#"{"status": "ok", "time": 0.0}"#);
assert!(
ok.contains("200"),
"no `status.error` present → fall back to status+body: {ok}"
);
// Routing-level 404 from a wrong HTTP verb: empty body, no JSON at all.
let empty = format_error_body(reqwest::StatusCode::NOT_FOUND, "");
let empty = format_error_body(StatusCode::NOT_FOUND, "");
assert!(empty.contains("empty body"), "got: {empty}");
assert!(
empty.contains("verb"),
@@ -384,15 +387,11 @@ mod tests {
#[test]
fn is_multi_vector_rejects_the_named_single_collection() {
// The only supported shape: a single unnamed vector.
let unnamed = serde_json::json!({
"result": {"config": {"params": {"vectors": {"size": 1536, "distance": "Cosine"}}}}
});
assert!(!is_multi_vector_config(&unnamed));
// Named but SINGLE — structurally a map, and writes to it fail with
// `400 "Wrong input: Not existing vector name error:"`. A `len() > 1` check
// would wrongly accept this one; that is the bug this case exists to catch.
let named_single = serde_json::json!({
"result": {"config": {"params": {"vectors": {"text": {"size": 1536}}}}}
});
@@ -426,7 +425,7 @@ mod tests {
#[tokio::test]
async fn rebuild_indexes_refuses_for_attached_and_unattached_alike() {
let mut provider = QdrantProvider {
client: reqwest::Client::new(),
client: Client::new(),
base_url: "http://localhost:6333".to_string(),
collection: "c".to_string(),
};
@@ -459,13 +458,12 @@ mod tests {
#[tokio::test]
async fn fetch_content_short_circuits_on_an_empty_id_list() {
// No network is touched: the early return happens before any request, which
// is why this can assert against an unreachable host.
let provider = QdrantProvider {
client: reqwest::Client::new(),
client: Client::new(),
base_url: "http://127.0.0.1:1".to_string(),
collection: "c".to_string(),
};
assert!(provider.fetch_content(&[]).await.unwrap().is_empty());
}
@@ -475,6 +473,7 @@ mod tests {
let collections = QdrantProvider::list_collections("http://localhost:6333", None)
.await
.unwrap();
assert!(!collections.is_empty());
}
@@ -485,7 +484,9 @@ mod tests {
.await
.unwrap();
let embedding = vec![0.0f32; 1536];
let results = provider.vector_search(&embedding, 5, 0.0).await.unwrap();
assert!(results.len() <= 5);
}
}
src/rag/providers/yaml.rs
+9 -34
View File
@@ -20,9 +20,6 @@ impl YamlProvider {
}
fn build_content_map(data: &RagData) -> IndexMap<DocumentId, String> {
// Keyed on `files`, NOT `vectors`: this is the exact replacement for the
// per-id document lookup it supersedes, and it must resolve every id that
// BM25 or graph_search can produce — both of which enumerate `files`.
data.iter_documents()
.map(|(id, doc)| (id, doc.page_content.clone()))
.collect()
@@ -52,12 +49,11 @@ impl RagProvider for YamlProvider {
})
})
.collect();
Ok(results)
}
async fn fetch_content(&self, ids: &[DocumentId]) -> Result<Vec<(DocumentId, String)>> {
// Iterating `ids` (not `content_map`) satisfies the trait's ordering
// contract for free — output order mirrors input order.
Ok(ids
.iter()
.filter_map(|id| self.content_map.get(id).map(|text| (*id, text.clone())))
@@ -65,10 +61,10 @@ impl RagProvider for YamlProvider {
}
async fn rebuild_indexes(&mut self, data: &RagData, _full_rebuild: bool) -> Result<()> {
// Local in-memory state — a wholesale rebuild is fast and always correct,
// so the incremental/full distinction is irrelevant here.
self.hnsw = data.build_hnsw();
self.content_map = Self::build_content_map(data);
Ok(())
}
@@ -80,14 +76,9 @@ impl RagProvider for YamlProvider {
#[cfg(test)]
mod provider_tests {
use super::*;
// `RagFile` and `RagDocument` are not used by the impl above, so they are
// imported here rather than at module scope. Both have private fields, which
// is why these tests must live in-crate rather than under `tests/`.
use crate::rag::{RagDocument, RagFile};
fn minimal_rag_data() -> RagData {
// `..Default::default()` rather than an exhaustive struct literal so that
// later additions to `RagData` do not break this helper.
RagData {
embedding_model: "text-embedding-3-small".to_string(),
chunk_size: 1024,
@@ -99,7 +90,7 @@ mod provider_tests {
}
}
/// Two files, one chunk each, with vectors — the minimum needed to exercise
/// Two files, one chunk each, with vectors, the minimum needed to exercise
/// `build_content_map` and the `fetch_content` ordering contract.
/// `DocumentId::new(f, d)` packs (file_index, document_index); `RagData::add`
/// is the real insertion path but a direct literal is sufficient and avoids
@@ -143,13 +134,12 @@ mod provider_tests {
async fn yaml_provider_empty_data_returns_nothing() {
let data = minimal_rag_data();
let provider = YamlProvider::from_data(&data);
let results = provider.fetch_content(&[]).await.unwrap();
assert!(results.is_empty());
}
/// `fetch_content` MUST return results in the same relative order as the input
/// ids. The reversed-input case is the one that fails if an implementation ever
/// iterates its own map instead of `ids`.
#[tokio::test]
async fn yaml_provider_fetch_content_preserves_input_order() {
let data = populated_rag_data();
@@ -163,8 +153,8 @@ mod provider_tests {
assert_eq!(forward[0].1, "alpha");
assert_eq!(forward[1].1, "beta");
// Reversed input must produce reversed output — NOT storage order.
let reversed = provider.fetch_content(&[b, a]).await.unwrap();
assert_eq!(
reversed[0].1, "beta",
"fetch_content must honor input order"
@@ -172,8 +162,6 @@ mod provider_tests {
assert_eq!(reversed[1].1, "alpha");
}
/// A missing id is skipped, not an error, and does not disturb the order of
/// the ids that DO resolve.
#[tokio::test]
async fn yaml_provider_fetch_content_skips_missing_ids() {
let data = populated_rag_data();
@@ -189,23 +177,16 @@ mod provider_tests {
assert_eq!(out[1].1, "beta");
}
/// `YamlProvider::duplicate()` rebuilds from `data`, so the clone is a genuine
/// independent snapshot. Providers backed by a shared store deliberately are not.
#[tokio::test]
async fn yaml_provider_duplicate_returns_equivalent_content() {
// MUST be populated_rag_data(): on minimal_rag_data() both providers hold an
// EMPTY content map, so `assert_eq!(r1, r2)` compares two empty vectors and
// passes against a duplicate() that returns nothing at all.
let data = populated_rag_data();
let provider = YamlProvider::from_data(&data);
let dup = provider.duplicate(&data);
let ids = [DocumentId::new(0, 0), DocumentId::new(1, 0)];
// Query with REAL ids, not `&[]` — an empty slice is answered without ever
// touching the content map, so it would pass against a broken duplicate().
let r1 = provider.fetch_content(&ids).await.unwrap();
let r2 = dup.fetch_content(&ids).await.unwrap();
// Guard against the vacuous case: if both sides resolved nothing, the equality
// below proves nothing. Assert the fixture actually produced content first.
assert_eq!(r1.len(), 2, "fixture must resolve both documents");
assert_eq!(
r1, r2,
@@ -213,11 +194,6 @@ mod provider_tests {
);
}
/// The content store is keyed on `files`, never on `vectors`. A vector may exist
/// for an id with no backing file (a stale entry, or a file dropped mid-sync);
/// keying on `vectors` would surface such an id with empty text instead of
/// dropping it. The shared fixture only ever inserts vectors for ids that also
/// have files, so this case has to be constructed here.
#[tokio::test]
async fn yaml_provider_content_is_keyed_on_files_not_vectors() {
let mut data = populated_rag_data();
@@ -232,7 +208,6 @@ mod provider_tests {
"an id present only in `vectors` must not resolve to content"
);
// The file-backed ids still resolve, so the assertion above is not vacuous.
let real = provider
.fetch_content(&[DocumentId::new(0, 0), DocumentId::new(1, 0)])
.await
src/render/markdown.rs
-11
View File
@@ -1749,11 +1749,6 @@ std::error::Error>> {
);
}
/// Removes CSI escape sequences so only printable content is measured.
///
/// Deliberately tolerant of malformed input: a sequence that was sliced
/// mid-escape swallows the following characters, which is precisely the
/// corruption `render_table_pads_columns_by_display_width` exists to catch.
fn strip_ansi(text: &str) -> String {
let mut out = String::with_capacity(text.len());
let mut chars = text.chars();
@@ -1780,12 +1775,6 @@ std::error::Error>> {
assert_eq!(strip_ansi("plain"), "plain");
}
/// `render_table` hands comfy-table pre-styled cells that already contain
/// ANSI escapes, and `colorize_box_chars` adds more afterwards. Column
/// widths are therefore only correct if the escapes are excluded from the
/// width calculation. When they are not, the table still renders and every
/// other assertion in this file still passes -- only the alignment silently
/// degrades -- so this is the sole guard over that behaviour.
#[test]
fn render_table_pads_columns_by_display_width() {
use unicode_width::UnicodeWidthStr;
src/repl/mod.rs
+10 -15
View File
@@ -219,7 +219,7 @@ static REPL_COMMANDS: LazyLock<[ReplCommand; 60]> = LazyLock::new(|| {
),
ReplCommand::new(
".rag attach",
"Attach to a pre-existing external RAG (Qdrant)",
"Attach to a pre-existing external RAG",
AssertState::False(StateFlags::AGENT),
),
ReplCommand::new(
@@ -889,22 +889,17 @@ pub async fn run_repl_command(
let version = args.map(|s| s.trim().to_string());
task::spawn_blocking(move || config::run_self_update(version, false)).await??;
}
".rag" => {
// `split_first_arg` rather than `starts_with("attach ")`: the latter
// misses a bare `.rag attach`, which would silently create a RAG
// literally named "attach".
match split_first_arg(args) {
Some(("attach", rest)) => match rest {
Some(name) if !name.trim().is_empty() => {
ctx.attach_rag(name.trim()).await?;
}
_ => println!("Usage: .rag attach <name>"),
},
_ => {
ctx.use_rag(args, abort_signal.clone()).await?;
".rag" => match split_first_arg(args) {
Some(("attach", rest)) => match rest {
Some(name) if !name.trim().is_empty() => {
ctx.attach_rag(name.trim()).await?;
}
_ => println!("Usage: .rag attach <name>"),
},
_ => {
ctx.use_rag(args, abort_signal.clone()).await?;
}
}
},
".agent" => match split_first_arg(args) {
Some((agent_name, args)) => {
let (new_args, _) = split_args_text(args.unwrap_or_default(), cfg!(windows));
src/sandbox/mcp_credentials.rs
-10
View File
@@ -387,10 +387,6 @@ pub(crate) fn collect_server_allow_entries(
out.into_iter().collect()
}
/// The single definition of the sbx kit v2 allow-list entry grammar: https on
/// the default port yields a bare host, anything else is spelled `host:port`.
/// Bracketed IPv6 hosts and non-http(s) schemes have no representation in the
/// grammar and yield `None`.
pub(crate) fn allow_entry_for_url(raw: &str) -> Option<String> {
let url = Url::parse(raw).ok()?;
let scheme = url.scheme();
@@ -465,12 +461,6 @@ struct Network {
allow: Vec<String>,
}
/// Serializes one sbx kit v2 mixin document.
///
/// Every `inject[].domain` is unioned into `permissions.network.allow`: sbx
/// does not derive allow entries from inject rules, so a rule whose domain is
/// not allowed would be dead. Enforcing it here keeps the invariant in one
/// place for every mixin Coyote generates.
pub(crate) fn render_mixin_document(
name: &str,
description: &str,
src/sandbox/mixins.rs
-10
View File
@@ -73,10 +73,6 @@ pub fn discover() -> Result<Vec<DiscoveredMixin>> {
for path in collect_subdir_mixins(&paths::agents_data_dir()) {
out.push(read_mixin(path)?);
}
// RAG sidecars are FLAT files named `<rag>.sbx-mixin.yaml` inside rags/, not
// the `<subdir>/sbx-mixin.yaml` shape the two scans above walk. Loaded
// unconditionally, mirroring agents/*: a RAG mixin only adds an outbound
// allowlist entry for that RAG's host and opens no inbound rules.
for path in collect_flat_mixins(&paths::rags_dir()) {
out.push(read_mixin(path)?);
}
@@ -184,8 +180,6 @@ fn collect_subdir_mixins(dir: &Path) -> Vec<PathBuf> {
result
}
/// Mixins stored as flat `<name>.sbx-mixin.yaml` files directly inside `dir`,
/// matched by suffix rather than by exact filename.
fn collect_flat_mixins(dir: &Path) -> Vec<PathBuf> {
let mut result = Vec::new();
let Ok(rd) = read_dir(dir) else { return result };
@@ -516,16 +510,13 @@ network:
}
}
/// RAG sidecars are flat `<name>.sbx-mixin.yaml` files, matched by SUFFIX.
#[test]
fn collect_flat_mixins_matches_rag_sidecars_by_suffix() {
let root = unique_root("flat-mixins");
fs::write(root.join("company-docs.sbx-mixin.yaml"), "kind: mixin\n").unwrap();
fs::write(root.join("alpha.sbx-mixin.yaml"), "kind: mixin\n").unwrap();
// The RAGs themselves must not be picked up, only their sidecars.
fs::write(root.join("company-docs.yaml"), "driver: qdrant\n").unwrap();
fs::write(root.join("notes.yaml"), "driver: yaml\n").unwrap();
// A directory whose name ends in the suffix is not a mixin file.
fs::create_dir_all(root.join("decoy.sbx-mixin.yaml")).unwrap();
let found = collect_flat_mixins(&root);
@@ -533,7 +524,6 @@ network:
.iter()
.map(|p| p.file_name().unwrap().to_str().unwrap())
.collect();
// Sorted by file name, so the order is deterministic.
assert_eq!(
names,
vec!["alpha.sbx-mixin.yaml", "company-docs.sbx-mixin.yaml"]
src/sandbox/mod.rs
+2 -13
View File
@@ -314,11 +314,6 @@ fn inject_mcp_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<Opt
)?))
}
/// Registers the API key of every attached RAG with the sbx proxy.
///
/// `launch()` has no notion of an active RAG — that is runtime state set by
/// `--rag` / `.rag` and never persisted — so every attached RAG is scanned
/// unconditionally, exactly as `inject_mcp_secrets` does for MCP servers.
fn inject_rag_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<()> {
let rags_dir = paths::rags_dir();
if !rags_dir.exists() {
@@ -330,7 +325,6 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<()>
continue;
}
let stem = match path.file_stem().and_then(|s| s.to_str()) {
// Skip sidecars ("myrag.sbx-mixin.yaml" has stem "myrag.sbx-mixin").
Some(s) if !paths::is_rag_sidecar_name(s) => s.to_string(),
_ => continue,
};
@@ -346,10 +340,6 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<()>
let Some(placeholder) = data.driver_config.get("api_key") else {
continue;
};
// The sidecar mixin declares `credentials[].service` under the same
// derivation, so the bound value and the inject rule that consumes it
// always name the same service. Passing the raw stem here would produce
// an id sbx rejects for any RAG whose name is not already a valid id.
let service_id = mcp_credentials::secret_service_id(&stem);
if service_id.is_empty() || registered.contains(&service_id) {
continue;
@@ -358,9 +348,7 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<()>
.trim_start_matches("{{")
.trim_end_matches("}}")
.trim();
// Degrade rather than abort: one stale RAG key must not block the whole
// sandbox launch. Queries to that RAG fail with a 401 at runtime, which
// is recoverable without a restart.
match vault.get_secret(secret_name, false) {
Ok(secret_value) => {
sbx_secret_set(&service_id, &secret_value)
@@ -375,6 +363,7 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet<String>) -> Result<()>
}
}
}
Ok(())
}