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.
The driver picker still told users a duckdb RAG can only be open in one Coyote
process at a time. That stopped being true once the store began opening
read-only for queries: any number of processes can now query it concurrently.
The restriction that remains is narrower and only bites while writing, so the
prompt now states that instead — several processes can query at once, but an
ingest or rebuild locks the others out until it finishes.
The attach wizard mapped a collection's vector dimension to a hardcoded list of
model names and printed them as likely candidates. The list was never checked
against the models the user actually has configured, so it could recommend a
model they cannot select, and one entry was a parenthetical note rather than a
model id and so could never match anything. Any list like this rots as models
are released.
The dimension itself comes from the server and is worth stating, so it is still
printed, as is the warning that a mismatched embedding model returns bad
results. Deriving real candidates would need a dimension recorded against each
configured model, which the model config does not carry today.
Point ids were read with as_u64() inside a filter_map, so a string id was
silently dropped and a UUID-keyed collection returned zero hits with no error.
The attach wizard therefore refused such collections and told the user to
rebuild with integer ids, which defeats the purpose of attaching to a
collection someone else already built. LangChain, a common way to populate
Qdrant, uses UUIDs by default.
The integer id was never load-bearing for this driver. DocumentId is a packed
(file, chunk) pair used positionally by the local drivers, but an attached RAG
holds no local files or vectors and every positional consumer already returns
early on it, so the id only has to survive the round trip from search back to
the content fetch. Ids that cannot make that trip as a u64 are interned behind
a synthetic handle and restored when the fetch is issued, leaving collections
that already use integer ids on exactly the path they used before.
`sample_point_id` returns `None` for a collection with no points, so the
UUID guard's `if let Some(..)` fell straight through and the wizard attached
happily. The result is a RAG that answers every query with zero hits and
never says why.
Sample once, then check for emptiness explicitly. This warns and asks rather
than hard-failing: an empty collection is not necessarily a mistake, since
another tool may be about to populate it, and none of the wizard's remaining
probes can distinguish that from a misconfiguration. The confirmation
defaults to "no" so it cannot be walked past by accident, and `attach`
already refuses to run non-interactively, so no unattended path reaches it.
The wizard hard-errored with "Secret 'X' not found in vault. Run
`coyote --add-secret X` first.", throwing away every answer the user had
already given it. Offer to create the secret in place instead, deferring to
`Vault::add_secret` for the masked prompt, the provider write and the
confirmation line, then read it back.
Only a genuine `SecretError::NotFound` triggers the offer. An auth failure,
a provider outage, or the vault being disabled inside a sandbox all
propagate with their own message, because prompting for a value that cannot
be stored would fail one step later and bury the real cause. Declining the
offer fails with both ways out spelled: add the secret up front, or answer
"no" to the API-key question.
Only `driver_config["api_key"]` was interpolated, so any credential-bearing
driver field added later would have shipped its raw `{{PLACEHOLDER}}` to the
server. Resolve every value instead, via `resolve_driver_config`.
Resolution still happens into a function-local copy and never touches
`RagData`: `save()` serializes `self.data` and is called by `.set rag_top_k`
and friends, so a resolved credential parked there would be written to the
RAG's YAML in plaintext. The literal `{{NAME}}` also has to survive on disk
because sandbox credential provisioning parses it back out to learn which
vault secret to bind. Scope stays `driver_config` deliberately: the rest of
a RAG file is ingested document text, where `{{...}}` is ordinary content.
Covered by a test that saves after a load and asserts the placeholder, not
the secret, is what reaches the file.
`interpolate_secrets` does not error on a secret the vault cannot resolve:
it substitutes the empty string and returns the name in its second tuple
element. `load_async` discarded that vec, so a typo'd or deleted vault
secret produced `api_key = ""` and an unexplained 401 from Qdrant.
Bail instead, naming the RAG and the missing secrets, matching what global
config loading already does.
The RAG attach sidecar was written against the sbx kit v1 spec and still emitted schemaVersion "1" with network.allowedDomains, network.serviceDomains, network.serviceAuth, credentials.sources.<n>.env and environment.proxyManaged. Every one of those keys was removed in kit v2. Coyote does not validate mixins, it copies them byte-for-byte into spec.yaml, so the invalid document surfaced only as an opaque sbx failure with no indication of which mixin caused it.
generate_rag_sbx_mixin now builds the document from the shared serializer structs instead of a format! string, which is how the envelope drifted unnoticed in the first place. render_mixin_yaml and the RAG sidecar both go through a new render_mixin_document, giving one definition of the envelope and one enforcement point for the rule that every inject domain must also appear in permissions.network.allow.
Fix an auth bug the port exposed: inject_rag_secrets bound the API key with sbx secret set, but nothing ever emitted a matching credentials entry, so the proxy held a value with no inject rule and never rewrote the auth header. An attached RAG credential silently did not work inside the sandbox. The sidecar now declares that credential; a RAG with no API key declares none while still receiving egress.
Fix the service id: the bind passed the raw file stem instead of routing it through secret_service_id, so a RAG named My_Docs produced an illegal id. The bind and the generated credentials service now share that derivation and cannot disagree.
Retire sbx_domain_forms in favour of allow_entry_for_url, now pub(crate). It emitted both a bare host and host:port because v1 serviceDomains needed a bare key; v2 has no such need, so the extra entry is simply wrong. It also defaulted a schemeless host to port 6333 while normalize_base_url resolves it to http and port 80, meaning the allow entry named a port the client never dialled.
Adds QdrantProvider as a read-only driver for pre-existing remote Qdrant
collections, an interactive '.rag attach' wizard, and the sandbox credential
and domain-whitelisting wiring that lets an attached RAG work inside a sandbox.
Attach-only by design: rebuild_indexes bails for both the attached and the
unattached case rather than silently succeeding. Coyote never writes to Qdrant
in this change.
Vectors are never hydrated back from Qdrant. Cosine collections L2-normalize
stored vectors on write, so reading them back returns unit-length copies of the
originals; the YAML vector copy is authoritative and the serialization guard
stays scoped to the duckdb driver alone.
Collections keyed by string or UUID point IDs are rejected at attach time. The
read path parses point ids as u64 inside a filter_map, so such a collection
would otherwise yield zero results with no error.
The three response-shape parsers are pure functions over an already-parsed JSON
body, unit-tested against captured fixtures, with the async wrappers delegating
to them rather than duplicating the logic.
'.rag' now splits its first argument, so '.rag attach <name>' no longer tries to
load a RAG literally named 'attach <name>'.
Phase 3 of the RAG driver abstraction. Adds a `DuckDbProvider` that keeps
vectors and document content in a `.duckdb` sidecar next to the existing
YAML metadata, selected by the `driver: duckdb` field.
- `src/rag/providers/duckdb.rs` (new): vector search via the vss extension
and keyword search via fts, an all-or-nothing hydration path (a partial
read is an error, never a shorter map), and an anti-wipe guard that
refuses the destructive `CREATE OR REPLACE TABLE` when `data.vectors` is
empty while `data.files` is not and the store still holds rows.
- `src/rag/mod.rs`: `sync_documents` now refreshes `bm25`/`node_to_docs`
BEFORE the fallible `provider.rebuild_indexes`. `self.data` is already
mutated by that point, so propagating a provider error afterwards would
leave the derived in-memory state describing the previous corpus while
`data` describes the new one. Both rebuilds are pure functions of
`self.data` and cannot fail, so running them first is always safe.
- `src/config/paths.rs`: sidecar path helpers.
- `src/rag/providers/mod.rs`, `src/config/agent.rs`: driver dispatch and
RAG cache keying.
Also keeps `RequestContext::rag_key` in lockstep with `rag` at the two
sites that were still missing it, so that a cache insert and its matching
invalidate are structurally incapable of disagreeing:
- `use_agent` assigned `self.rag` from the agent but never set `rag_key`.
This one was live. Agent RAGs are inserted under `RagKey::Agent(<name>)`,
so with `rag_key == None` the invalidation guards in `rebuild_rag` and
`edit_rag_docs` matched nothing and `.rebuild rag` left the stale cache
entry in place. Worse, a preceding `.rag <name>` left a stale
`Named(<name>)` key attached to the agent's RAG, pointing the
invalidation at an unrelated RAG's cache entry. Now mirrors the insert
key exactly, yielding `None` when the agent has no RAG.
- `exit_agent` cleared `self.rag` but left `rag_key` behind. Latent rather
than live, since `rebuild_rag`/`edit_rag_docs` both bail on
`rag.is_none()` before reaching the invalidate guards, but the guards
that make it unobservable are not the kind of thing to depend on.
Covered by `use_agent_does_not_carry_stale_rag_key`, and by a new
assertion in `exit_agent_clears_all_agent_state`.
Adds the duckdb crate with the bundled feature ahead of any provider
code, so the dependency and build surface can be proven on every CI
target on its own.
duckdb constrains comfy-table to ~7.1, so comfy-table moves from 7.2.2
to 7.1.4 while keeping custom_styling. That feature swaps
measure_text_width for an ANSI-stripping implementation that
render_table's pre-styled cells depend on; dropping it still compiles
and still passes every other test, and only corrupts column widths. A
regression test now renders a styled table at a fixed wrap width and
asserts every line has an equal ANSI-stripped display width.
comfy-table 7.1.4 pins crossterm 0.28 while coyote pins 0.29, so both
now build side by side. comfy_table::Color, Attribute and Cell are
consequently crossterm 0.28 types and must not be used; table styling
stays ANSI-string based.
Caches the DuckDB extension directory in CI, keyed on the runner OS and
the DuckDB version, so vss and fts survive an upstream outage.
Also switches merge_vector_results to f32::total_cmp. The previous
partial_cmp().unwrap_or(Equal) comparator is not total under NaN, which
sort_by is permitted to answer with a panic in release builds.
Introduce a narrow `RagProvider` trait covering vector search and content
retrieval, and make `Rag` delegate to a boxed provider instead of owning an
HNSW index directly. `YamlProvider` is the sole implementation for now.
The trait deliberately stays narrow: embeddings, chunking, BM25 keyword
search, graph RAG, entity extraction, RRF merging and persistence all remain
on `Rag`/`RagData`, so a new storage backend does not have to reimplement
Coyote's indexing logic.
Notable points:
- `fetch_content`'s ordering contract is part of the trait, not an accident.
Implementations must return results in input-`ids` order; `hybrid_search`
passes an RRF-ranked list straight to the prompt builder, so a provider
returning storage order would silently discard the ranking.
- The content store is keyed on `data.files`, never `data.vectors`. Both the
content map and BM25 now route through the new `RagData::iter_documents()`
so the two key spaces match by construction. `RagData::add` zips document
ids with embeddings and truncates silently, so ids in `files \ vectors` are
genuinely reachable.
- A provider keyword-search failure degrades to an empty ranker with a
warning rather than failing the whole query; it is one of three RRF inputs.
It deliberately does not fall back to the local BM25, which would be a
silent ranking-algorithm swap once a provider with native FTS exists.
- The rerank path builds its text and id vectors from a single `fetch_content`
result in one pass, so the reranker's positional indices cannot desync.
This is not a bit-for-bit no-op. `vector_search` now dedups by best score and
sorts globally instead of concatenating per-chunk hit lists. Single-chunk
queries (the common case) are unaffected. Multi-chunk queries get corrected
rank assignment and no longer let a document that matched several query
chunks accumulate multiple RRF contributions. There is no overall cap on the
merged pool — truncation remains `reciprocal_rank_fusion`'s job.
`RagData::get()` is removed: its only two callers were the content lookups
replaced here, and an unused private-module method fails the build under
`--deny warnings`. Its three tests were rewritten against `iter_documents()`,
one of which now guards the files-vs-vectors keying directly.
Implements Phase 2 of the RAG driver abstraction design (§6).
Phase 1 of the RAG driver abstraction (design doc sections 5.1-5.5a).
Data model:
- Add `driver: String` (serde default "yaml" via RagData::default_driver) and
`attached: bool` as the first two fields of RagData, so driver metadata sits
at the top of each RAG YAML. Old files without them load unchanged.
- Add `#[serde(default)]` to the non-Option fields so a minimal attached-RAG
YAML deserializes, and add `skip_serializing_if` to `vectors` so an empty
map renders no `vectors:` key.
- Add a hand-written `impl Default for RagData` delegating to `RagData::new()`.
It is deliberately not derived: a derived impl yields `driver: ""`, which is
not a valid driver string.
Validation (the price of the new serde defaults):
- Add `RagData::validate()`, called from `Rag::load()` after deserialization.
It enforces the (driver, attached) matrix and, critically, numeric floors
that the new defaults would otherwise mask: `top_k >= 1` unconditionally
(a 0 makes every query return nothing, silently), and `chunk_size >= 1` plus
`chunk_overlap < chunk_size` when not attached (a 0 chunk_size is a real
divide-by-zero panic while sizing embedding batches).
- Reject `.set rag_top_k 0` at the setter, before the set/update fork. Without
this, the new load-time floor turns one keystroke into an unloadable RAG:
the setter saves immediately and no dot-command can reach the file again.
Rebuild actually re-embeds now:
- `.rebuild rag` and `--rebuild-rag` previously re-scanned paths and re-embedded
nothing, because the content-hash skip fired regardless of the refresh flag.
Extract that decision into a module-level `find_hash_skip()` free function and
thread a `force_reingest` flag through `sync_documents()` and
`refresh_document_paths()`, set true only from `rebuild_rag()`. `.edit rag-docs`
stays incremental. Re-embedding costs time and API spend, so `rebuild_rag()`
now prints a one-line file-count warning first (no prompt: the path is
reachable from a non-interactive CLI flag).
Attached-RAG guards:
- Block `.rebuild rag` / `--rebuild-rag` and `.edit rag-docs` on attached RAGs,
which Coyote did not index and whose source documents it does not own.
- Add `Rag::driver()`, `Rag::is_attached()` and `Rag::file_count()`, and surface
driver/attached through `Rag::export()` so `.info rag` shows them.
Adds 12 unit tests (1299 -> 1311), including the two gate tests pinning that a
forced re-ingest does not hash-skip while an ordinary refresh still does.
Dark-Alex-17