feat(memory): persistent agent memory with SQLite + FTS5 (Phase 1) - #154
Merged
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a SQLite-backed persistent memory subsystem with observation storage, full-text search, session and summary management, MCP tools, CLI commands, schema migrations, and SQLite tuning. ChangesPersistent memory subsystem
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant AgentflareMcp
participant MemoryHandlers
participant SQLiteMemoryStore
MCPClient->>AgentflareMcp: Invoke memory tool
AgentflareMcp->>MemoryHandlers: Construct input and call handler
MemoryHandlers->>SQLiteMemoryStore: Open database and execute memory operation
SQLiteMemoryStore-->>MemoryHandlers: Return stored or queried data
MemoryHandlers-->>AgentflareMcp: Serialize JSON response
AgentflareMcp-->>MCPClient: Return tool result
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
src/db.rs (1)
104-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared SQLite tuning helper
src/db.rsandsrc/memory/store.rsboth define the sametune(conn: &Connection)setup. Move it to a sharedpub(crate)helper so the two SQLite connections don’t drift if the tuning changes later.🤖 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/db.rs` around lines 104 - 110, Extract the duplicated tune helper from db.rs and memory/store.rs into one shared pub(crate) function, preserving its busy timeout, WAL journal setup, synchronous setting, and rusqlite::Result behavior. Update both connection setup paths to call the shared helper and remove their local tune definitions.crates/agentflare-artifacts/src/lib.rs (1)
499-509: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
InvalidInputfor the whitespace case too.Using only
is_err()could allow an unrelated failure to satisfy this test. Capture the error withunwrap_err()and asserterr.kind() == std::io::ErrorKind::InvalidInput, matching the missing-recipient case.This is based on the test’s stated error contract.
🤖 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 `@crates/agentflare-artifacts/src/lib.rs` around lines 499 - 509, Update the whitespace-recipient assertion in the publish test to capture the result with unwrap_err() and assert that the error kind is std::io::ErrorKind::InvalidInput, matching the missing-recipient case while preserving the existing PublishRequest setup.src/cli/memory.rs (1)
68-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Sessions/Observationsbypass the mcp handler layer used byContext/Search.These two arms call
crate::memory::store::open()directly and querysessions/observationsmodule functions, whileContext/Searchgo throughcrate::memory::mcp::handle_context/handle_recall(which internally open their own connection). Worth confirming both paths apply identical connection setup so behavior doesn't silently drift as the mcp layer evolves; otherwise consider routing all four subcommands through mcp handlers for a single source of truth.🤖 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 68 - 85, Route the MemoryCommands::Sessions and MemoryCommands::Observations arms through the same crate::memory::mcp handler layer used by Context and Search, rather than opening the store and querying sessions/observations directly. Reuse the corresponding handle_context or handle_recall entry points and preserve the existing project, limit, JSON output, and error behavior.src/memory/schema.rs (1)
45-60: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFTS triggers reindex on every UPDATE, even for non-indexed column changes.
obs_au/prompts_auunconditionally delete+reinsert the FTS row on every UPDATE, including updates that only touchpinned,revision_count,review_after, orlast_seen_at(none of which are indexed columns). This causes unnecessary FTS churn on hot paths like pinning/curation.♻️ Add a WHEN guard so only relevant column changes trigger reindexing
CREATE TRIGGER IF NOT EXISTS obs_au AFTER UPDATE ON observations +WHEN new.title IS NOT old.title OR new.content IS NOT old.content + OR new.tool_name IS NOT old.tool_name OR new.type IS NOT old.type + OR new.project IS NOT old.project BEGIN INSERT INTO observations_fts(observations_fts, rowid, title, content, tool_name, type, project) VALUES('delete', old.id, old.title, old.content, old.tool_name, old.type, old.project); INSERT INTO observations_fts(rowid, title, content, tool_name, type, project) VALUES (new.id, new.title, new.content, new.tool_name, new.type, new.project); END;🤖 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/schema.rs` around lines 45 - 60, Update the obs_au UPDATE trigger, and the corresponding prompts_au trigger if present, to add a WHEN guard that fires only when an indexed column changes: title, content, tool_name, type, or project. Preserve the existing delete-and-reinsert FTS synchronization for those changes while skipping reindexing for updates limited to non-indexed fields such as pinned, revision_count, review_after, or last_seen_at.src/memory/search.rs (1)
101-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate row-mapping logic vs.
observations::map_observation.
map_search_rowreimplements the exact same 18-column →Observationmapping already defined inobservations.rs. Consider exposingobservations::map_observation(e.g.pub(crate)) and reusing it here to avoid the two copies drifting apart when the schema changes.🤖 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/search.rs` around lines 101 - 122, Expose observations::map_observation at crate visibility and update map_search_row to delegate to it instead of duplicating the 18-column Observation mapping. Preserve the existing rusqlite::Result<Observation> behavior while ensuring search results reuse the canonical observations mapper.
🤖 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 `@crates/agentflare-artifacts/src/store.rs`:
- Around line 141-147: Update the publish flow so ordinary artifact_publish
requests do not populate handoff sender routing unless an explicit handoff is
requested; keep sender/recipient validation for actual handoffs. Adjust the
relevant PublishRequest construction in mcp_server.rs and add a regression test
covering a configured agent publishing without a recipient.
In `@src/memory/mcp.rs`:
- Around line 56-76: Update handle_recall and the underlying search::search and
observations::list_recent APIs to accept the optional input.r#type filter and
apply it in SQL before enforcing limit. Remove the post-fetch filtered
collection in handle_recall, while preserving unfiltered behavior when no type
is provided.
- Around line 115-168: Refactor handle_handoff so the enrichment data and
snapshot are combined into a single sessions::update_enriched call, then execute
that update, sessions::close, and summaries::append inside one
conn.transaction() closure, propagating each operation’s errors and committing
only after all succeed. Preserve the existing validation, snapshot contents, and
response behavior.
In `@src/memory/observations.rs`:
- Around line 33-83: Update save() so both find_duplicate and the topic_key
lookup restrict matches to the same project and scope as the incoming
observation, preserving separation across project boundaries. In the topic-key
revision UPDATE, recompute the normalized hash for the new title/content and
persist it alongside content, title, and revision metadata so future duplicate
detection uses the current content.
In `@src/memory/relations.rs`:
- Around line 21-45: The create function returns an unrelated ID when the upsert
takes the conflict-update path. Update create to retrieve and return the ID of
the affected memory_relations row after the INSERT ... ON CONFLICT operation,
preferably by using RETURNING id or by re-reading the row with source_id,
target_id, and relation, while preserving the existing upsert behavior.
In `@src/memory/search.rs`:
- Around line 69-91: Update build_fts_query so that when all tokens sanitize
away, it returns a safe neutral FTS5 query rather than raw.to_string(). Preserve
the existing sanitized token joining behavior for non-empty token results, and
ensure punctuation-only inputs cannot reintroduce unsanitized MATCH syntax or
trigger a search failure.
- Around line 14-63: Fix project-scoped placeholder numbering in both search
queries by using distinct parameter indices for the project predicate and LIMIT,
then keep the bindings in params! aligned for both FTS and LIKE paths. Update
build_fts_query so punctuation-only input produces a safe empty/no-op result
instead of raw invalid MATCH syntax, and handle that result cleanly in the
search flow.
In `@src/memory/store.rs`:
- Around line 11-24: Update the directory and permission setup around
Connection::open to propagate errors from create_dir_all and both
set_permissions calls instead of discarding them. Preserve the intended 0700
parent-directory and 0600 database-file permissions so setup failures stop the
operation with the underlying error.
- Around line 30-35: Update the tune function to execute PRAGMA foreign_keys=ON
for each SQLite connection, alongside its existing connection pragmas. Preserve
the current timeout, WAL, synchronous settings, error propagation, and return
behavior.
In `@src/memory/summaries.rs`:
- Around line 14-34: The append function’s separate MAX(seq)+1 query and INSERT
can race under concurrent calls. Serialize sequence allocation by wrapping the
next_seq query and session_summaries INSERT in a transaction, preserving the
existing project scope and returned row ID.
---
Nitpick comments:
In `@crates/agentflare-artifacts/src/lib.rs`:
- Around line 499-509: Update the whitespace-recipient assertion in the publish
test to capture the result with unwrap_err() and assert that the error kind is
std::io::ErrorKind::InvalidInput, matching the missing-recipient case while
preserving the existing PublishRequest setup.
In `@src/cli/memory.rs`:
- Around line 68-85: Route the MemoryCommands::Sessions and
MemoryCommands::Observations arms through the same crate::memory::mcp handler
layer used by Context and Search, rather than opening the store and querying
sessions/observations directly. Reuse the corresponding handle_context or
handle_recall entry points and preserve the existing project, limit, JSON
output, and error behavior.
In `@src/db.rs`:
- Around line 104-110: Extract the duplicated tune helper from db.rs and
memory/store.rs into one shared pub(crate) function, preserving its busy
timeout, WAL journal setup, synchronous setting, and rusqlite::Result behavior.
Update both connection setup paths to call the shared helper and remove their
local tune definitions.
In `@src/memory/schema.rs`:
- Around line 45-60: Update the obs_au UPDATE trigger, and the corresponding
prompts_au trigger if present, to add a WHEN guard that fires only when an
indexed column changes: title, content, tool_name, type, or project. Preserve
the existing delete-and-reinsert FTS synchronization for those changes while
skipping reindexing for updates limited to non-indexed fields such as pinned,
revision_count, review_after, or last_seen_at.
In `@src/memory/search.rs`:
- Around line 101-122: Expose observations::map_observation at crate visibility
and update map_search_row to delegate to it instead of duplicating the 18-column
Observation mapping. Preserve the existing rusqlite::Result<Observation>
behavior while ensuring search results reuse the canonical observations mapper.
🪄 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: c73b9f32-b15b-403f-a5a5-f079a8cc9825
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlcrates/agentflare-artifacts/src/lib.rscrates/agentflare-artifacts/src/store.rssrc/cli/memory.rssrc/cli/mod.rssrc/db.rssrc/main.rssrc/mcp_server.rssrc/memory/mcp.rssrc/memory/mod.rssrc/memory/observations.rssrc/memory/prompts.rssrc/memory/relations.rssrc/memory/schema.rssrc/memory/search.rssrc/memory/sessions.rssrc/memory/store.rssrc/memory/summaries.rs
Comment on lines
+141
to
+147
| if meta.sender.is_some() | ||
| && meta.recipient.as_deref().unwrap_or("").trim().is_empty() | ||
| { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::InvalidInput, | ||
| "handoff sets `sender` but no `recipient` — the artifact would land in no inbox; set recipient=<agent>", | ||
| )); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the surrounding code.
git ls-files 'crates/agentflare-artifacts/src/store.rs' 'src/mcp_server.rs' 'crates/agentflare-artifacts/src/*' 'src/*' | sed 's#^`#FILE` #'
echo
echo '--- store.rs outline ---'
ast-grep outline crates/agentflare-artifacts/src/store.rs --view expanded || true
echo
echo '--- mcp_server.rs outline ---'
ast-grep outline src/mcp_server.rs --view expanded || true
echo
echo '--- store.rs relevant range ---'
sed -n '120,175p' crates/agentflare-artifacts/src/store.rs
echo
echo '--- mcp_server.rs relevant matches ---'
rg -n -A4 -B4 'sender|recipient|artifact_publish|PublishRequest' src/mcp_server.rs crates/agentflare-artifacts/src -g '!**/target/**' || trueRepository: getappz/agentflare
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- artifact_publish implementation ---'
sed -n '634,705p' src/mcp_server.rs
echo
echo '--- sender-default test ---'
sed -n '2030,2072p' src/mcp_server.rs
echo
echo '--- PublishRequest definition ---'
sed -n '125,160p' crates/agentflare-artifacts/src/types.rsRepository: getappz/agentflare
Length of output: 6106
Don't default sender for non-handoff publishes.
src/mcp_server.rs always fills PublishRequest.sender from self.agent, while crates/agentflare-artifacts/src/store.rs rejects any publish with sender set and an empty recipient. That makes ordinary artifact_publish calls from configured agents fail with InvalidInput. Gate the default behind an explicit handoff path, or separate publisher identity from handoff routing; add a regression test.
🤖 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 `@crates/agentflare-artifacts/src/store.rs` around lines 141 - 147, Update the
publish flow so ordinary artifact_publish requests do not populate handoff
sender routing unless an explicit handoff is requested; keep sender/recipient
validation for actual handoffs. Adjust the relevant PublishRequest construction
in mcp_server.rs and add a regression test covering a configured agent
publishing without a recipient.
6 composed MCP tools (remember, recall, context, handoff, relate, curate) backed by brain.db at ~/.agentflare/memory/brain.db. Schema: sessions, observations (+FTS5), user_prompts (+FTS5), memory_relations, session_summaries with auto-sync triggers. lean-ctx enriched session model. CLI subcommand: agentflare memory.
…onditions across the memory subsystem - search.rs: project_filter reused the same ?2 slot as LIMIT, so every project-scoped search errored (InvalidParameterCount) or misbound; filters now use always-present (?N IS NULL OR ...) clauses with their own indices, which also lets a type filter be threaded through cleanly. build_fts_query no longer passes an unsanitized raw string to FTS5 MATCH when every token sanitizes to empty. - observations.rs: dedup and topic_key lookups weren't scoped by project, so unrelated projects with similar content collided; the topic_key revision path now also refreshes normalized_hash so later duplicates of revised content are still detected. - relations.rs: create() used last_insert_rowid() after an upsert, which SQLite doesn't update on the ON CONFLICT DO UPDATE path; switched to RETURNING id so re-relating an existing triple returns the right id. - store.rs: foreign keys were declared in schema but never enforced (no PRAGMA foreign_keys=ON), and dir/permission/db-open failures during open() were silently swallowed with `let _ =`. - mcp.rs: handle_recall filtered by type after fetching a hard limit, dropping in-scope matches; type filtering now happens in SQL before LIMIT. handle_handoff wrote the session snapshot as a redundant second update_enriched call and ran enrich/close/append as separate unbatched statements with no transaction; merged into one write and wrapped the sequence in a transaction. sessions::create was never called from anywhere, so handoff always failed with "session not found" for any session_id; handoff now auto-creates the session. - summaries.rs: seq assignment via separate SELECT MAX + INSERT raced under concurrent handoffs; now a single INSERT...SELECT computes seq inline so it's atomic without needing its own transaction (important since it also runs inside handle_handoff's transaction). Adds regression tests for each of the above plus basic CRUD coverage for observations and sessions.
…ghted convention already used by gateway-registry and skill-registry
This was referenced Jul 11, 2026
getappz
added a commit
that referenced
this pull request
Jul 11, 2026
Engram's cross-session memory role is now fully covered by the in-binary memory subsystem (SQLite + FTS5, shipped in PR #154 / #157). Removes: - The whole engram install/setup Component (plugin install, mise-based binary install, per-host MCP registration) and its dedicated engram_install.rs module. - The ENGRAM rule text written to ~/.claude/rules/engram.md (and other hosts' equivalents) and the config.rs handoff_on_session_end toggle that gated firing an engram-cli handoff on SessionEnd. - All doc/comment references (README, AGENTS.md, CONTRIBUTING.md, SECURITY.md, install.ps1) — rewritten to describe the built-in memory module instead of engram where relevant. Backward compatibility: 'agentflare hook session-end' is kept as a no-op CLI subcommand (was previously the engram-cli handoff trigger) so a settings.json entry written by an older agentflare version doesn't start erroring after an upgrade — new installs never wire it. 'agentflare uninstall''s engram cleanup (removing old rule files, MCP registrations from CONTINUE/opencode/cline configs) is intentionally left in place so users migrating away from an old engram-integrated install can still clean up after it. Also dropped now-orphaned dead code this exposed: mise_install.rs's install_tool/which_tool/use_global (only ever called by engram_install.rs) and components.rs's host_marker/mark_done (only used by the engram Component's check/apply closures).
5 tasks
3 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In-binary agent memory subsystem for agentflare. 6 composed MCP tools backed by a dedicated
brain.dbat~/.agentflare/memory/.What's in Phase 1
6 composed MCP tools
memory_remembermemory_recallmemory_contextmemory_handoffmemory_relatememory_curateDatabase (
brain.db)sessions— lean-ctx enriched (findings[], decisions[], files_touched[], evidence[] as JSON columns, compaction_snapshot)observations— with FTS5 full-text search + auto-sync triggers on INSERT/UPDATE/DELETEuser_prompts— with FTS5 full-text searchmemory_relations— idempotent pair+relation upsert for semantic linkssession_summaries— bounded per-project summary historyCLI
agentflare memory context,agentflare memory search,agentflare memory sessions,agentflare memory observationsSide effects
PRAGMA synchronous=NORMALadded todb.rstune()(safe under WAL)Build & tests
hex(SHA-256 dedup hash encoding)Not in Phase 1
Summary by CodeRabbit