Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe memory subsystem now uses versioned SQLite migrations, stores optional observation embeddings, supports feature-gated hybrid semantic recall with FTS fallback, and provides a CLI command for embedding backfill. ChangesMemory and semantic retrieval
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MemoryMCP
participant EmbeddingEngine
participant MemoryEmbeddings
participant HybridSearch
participant Retrieval
MemoryMCP->>EmbeddingEngine: Embed document or query
EmbeddingEngine-->>MemoryMCP: Return optional vector
MemoryMCP->>MemoryEmbeddings: Upsert observation embedding
MemoryMCP->>HybridSearch: Run hybrid recall
HybridSearch->>MemoryEmbeddings: Fetch vector candidates
HybridSearch->>Retrieval: Merge, decay, and rerank candidates
Retrieval-->>MemoryMCP: Return ordered observations
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
getappz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/memory/observations.rs (1)
244-245: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnable foreign-key enforcement in every in-memory memory test database. These helpers migrate raw SQLite connections without
PRAGMA foreign_keys = ON, so tests may not validate the PR's foreign-key enforcement contract.
src/memory/observations.rs#L244-L245: enable and assert the pragma beforeschema::migrate.src/memory/mcp.rs#L404-L405: enable and assert the pragma before migration.src/memory/relations.rs#L112-L113: enable and assert the pragma before migration.src/memory/sessions.rs#L184-L185: enable and assert the pragma before migration.src/memory/summaries.rs#L78-L79: enable and assert the pragma before migration.src/memory/embeddings.rs (1)
41-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
candidates()doesn't filter by embeddingmodel; mixed-model vectors would be compared directly.
observations_vec.modelexists to track which model produced each vector (and the backfill CLI is designed to re-embed after a model change), butcandidates()'s SQL never filters on it. If the embedding model is ever swapped and only some rows have been re-backfilled, cosine similarity would be computed between vectors from two different, semantically incompatible embedding spaces — silently degradingsearch_hybrid(src/memory/search.rs) recall quality with no error surfaced. Current tests only exercise a single, uniform model tag, so this isn't covered.Consider filtering
candidates()by the caller's current model name (threaded fromengine::model_name()), so rows embedded with a stale model simply fall back to BM25-only ranking until re-backfilled.♻️ Sketch of a model-aware filter
pub fn candidates( conn: &Connection, query_vec: &[f32], project: Option<&str>, r#type: Option<&str>, + model: &str, k: usize, ) -> rusqlite::Result<Vec<(i64, f64)>> { let mut stmt = conn.prepare( "SELECT o.id, v.embedding FROM observations_vec v JOIN observations o ON o.id = v.obs_id WHERE o.deleted_at IS NULL + AND v.model = ?3 AND (?1 IS NULL OR o.project = ?1) AND (?2 IS NULL OR o.type = ?2)", )?; - let rows = stmt.query_map(params![project, r#type], |r| { + let rows = stmt.query_map(params![project, r#type, model], |r| {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/embeddings.rs` around lines 41 - 70, Update candidates() to accept the caller’s current embedding model name and add an observations_vec.model filter to its SQL parameters, excluding vectors tagged with stale models. Thread engine::model_name() through the search_hybrid path into candidates(), while preserving BM25-only fallback behavior when no current-model vectors are available.src/cli/memory.rs (1)
107-138: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the backfill upserts into a single transaction.
Each
embeddings::upsertcall runs as its own implicit write transaction. Forbatchup to hundreds of rows, this means hundreds of separate commits (with associated WAL checkpoints) instead of one — wrap the loop in an explicit transaction and commit once at the end for a meaningful throughput win on larger backfills.⚡ Sketch of batching the writes
Ok(todo) => { let total = todo.len(); let mut ok = 0usize; + let tx = conn.unchecked_transaction().ok(); for (id, text) in todo { match crate::memory::engine::embed_doc(&text) { Some(vec) => { match crate::memory::embeddings::upsert( &conn, id, &vec, &model, ) { Ok(()) => ok += 1, Err(e) => eprintln!("obs {id}: store failed: {e}"), } } None => eprintln!("obs {id}: embed failed"), } } + if let Some(tx) = tx { + let _ = tx.commit(); + } println!("backfilled {ok}/{total} embeddings (model: {model})");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/memory.rs` around lines 107 - 138, Update the MemoryCommands::BackfillEmbeddings flow to open one explicit database transaction before iterating over todo, perform each successful embeddings::upsert through that transaction, and commit once after the loop completes. Preserve the existing per-observation error reporting and success count, and ensure transaction setup or final commit errors are surfaced through the existing UI error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory/mcp.rs`:
- Around line 80-86: Update the search path centered on search::search_hybrid
and super::engine::embed_query to restrict hybrid candidates to the active
embedding model/version before scoring. Propagate the active model identifier
through the search arguments and filter observations_vec.model accordingly, or
enforce and validate a single-model invariant so vectors from mixed models are
never ranked together.
- Around line 44-53: Update the observation update path around embed_doc and
embeddings::upsert so a non-duplicate update removes or marks stale the existing
vector whenever embedding returns None or upsert fails. Preserve best-effort
remember behavior by not propagating the cleanup or indexing errors, while
leaving duplicate handling and successful upserts unchanged.
---
Nitpick comments:
In `@src/cli/memory.rs`:
- Around line 107-138: Update the MemoryCommands::BackfillEmbeddings flow to
open one explicit database transaction before iterating over todo, perform each
successful embeddings::upsert through that transaction, and commit once after
the loop completes. Preserve the existing per-observation error reporting and
success count, and ensure transaction setup or final commit errors are surfaced
through the existing UI error path.
In `@src/memory/embeddings.rs`:
- Around line 41-70: Update candidates() to accept the caller’s current
embedding model name and add an observations_vec.model filter to its SQL
parameters, excluding vectors tagged with stale models. Thread
engine::model_name() through the search_hybrid path into candidates(), while
preserving BM25-only fallback behavior when no current-model vectors are
available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39f18faa-e2ce-42ad-9adf-571691dc407e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
CHANGELOG.mdCargo.tomlcrates/agentflare-store/src/embed.rscrates/agentflare-store/src/lib.rscrates/agentflare-store/src/retrieval.rssrc/cli/memory.rssrc/memory/embeddings.rssrc/memory/engine.rssrc/memory/mcp.rssrc/memory/mod.rssrc/memory/observations.rssrc/memory/relations.rssrc/memory/schema.rssrc/memory/search.rssrc/memory/sessions.rssrc/memory/store.rssrc/memory/summaries.rs
| search::search_hybrid( | ||
| conn, | ||
| q, | ||
| input.project.as_deref(), | ||
| input.r#type.as_deref(), | ||
| limit, | ||
| super::engine::embed_query, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Filter hybrid candidates by embedding model.
search_hybrid receives only the query vector, while src/memory/embeddings.rs stores observations_vec.model but does not filter candidates by it. After a model change or mixed backfill, same-dimensional vectors from different models are scored together, producing invalid rankings. Pass the active model/version through the search path or enforce a single-model invariant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/memory/mcp.rs` around lines 80 - 86, Update the search path centered on
search::search_hybrid and super::engine::embed_query to restrict hybrid
candidates to the active embedding model/version before scoring. Propagate the
active model identifier through the search arguments and filter
observations_vec.model accordingly, or enforce and validate a single-model
invariant so vectors from mixed models are never ranked together.
|
Addressed CodeRabbit review (commit
Deferred to item #202 (medium, under store epic #187): the Major mixed-model ranking finding + Gate re-run: fmt clean, clippy clean for authored code, 28/28 memory tests pass. |
Migrates the memory brain DB (
brain.db) onto the sharedagentflare-store/agentflare-db-kitengine and adds optional hybrid recall (BM25 + vector) that degrades cleanly to today's FTS-only behavior when no embedding model is present.What changed
agentflare-store::retrieval— pure, dependency-free merge/decay/MMR/jaccard math ported from openpawz's memory engine.open()now goes throughdb_kit::open_filewith a versionedMigrationslist (frozenV1_DDL). FK enforcement and unix perms preserved; hand-rolledtune()deleted.observations_vectable +embeddingsmodule — vector storage plusmissing/candidateshelpers, every query filtered ondeleted_at IS NULL.semanticcargo feature — OnceLock ONNX engine that returnsNoneeverywhere on non-semantic builds, missing model, or a failed call.search_hybridwired into recall — merges FTS + vector (0.4/0.6), temporal decay (30d half-life), MMR rerank (lambda=0.7). With no embedder it is byte-identical tosearch().agentflare memory backfill-embeddingsCLI + CHANGELOG entry.Verification
cargo fmt --checkpassescargo clippy --workspace --all-targets --all-features -D warningsclean for all authored codecargo test --workspace— 35 test binaries ok, 0 failedResolves review blockers A-G (FK enforcement, soft-delete leak, error handling, dual schema, project scoping, duplicate-outcome corruption, missing tests) by construction.
Summary by CodeRabbit
New Features
semanticfeature).memory backfill-embeddingscommand to populate embeddings for existing memories.Improvements
Bug Fixes