fix: wire memory system — was completely broken (never initialized)#60
fix: wire memory system — was completely broken (never initialized)#60kienbui1995 merged 1 commit intomainfrom
Conversation
Critical fix: MemoryStore was never initialized in main.rs.
All memory features (memory_read/write tools, auto-memory, prompt
injection) were silently returning 'Memory not configured'.
Fixes:
1. Initialize MemoryStore in main.rs with per-project path
(~/.local/share/magic-code/memory/{project-hash}.json)
2. Wire /memory command to actually read/write/delete/list
3. Add memory_read/memory_write public methods to runtime
4. Expand auto-memory heuristics (10 patterns vs 3 before):
convention, always use, project uses, test command,
configured with, running on port, database is, deploy with
5. Add memory nudge in system prompt — agent proactively saves
test commands, framework versions, conventions
274 tests, 0 fail.
📝 WalkthroughWalkthroughThe changes introduce a persistent per-project memory store that initializes on CLI startup, loads previous conversation context from disk, and exposes memory operations through slash commands and runtime methods while improving the auto-save heuristic to detect and store semantic facts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TUI as TUI/CLI Handler
participant Main as main.rs
participant FileSystem as File System
participant Runtime as ConversationRuntime
participant Store as MemoryStore
Main->>Main: Compute hash from CWD
Main->>FileSystem: Resolve path ~/.local/share/magic-code/memory/<hash>.json
FileSystem-->>Main: Return file path
Main->>Store: MemoryStore::load(path, 200)
Store->>FileSystem: Read JSON file
FileSystem-->>Store: Return memory data
Store-->>Main: Initialized MemoryStore
Main->>Runtime: Inject MemoryStore instance
Runtime-->>Main: Ready for conversation
sequenceDiagram
participant User
participant TUI as TUI Handler
participant Main as main.rs
participant Runtime as ConversationRuntime
participant Store as MemoryStore
User->>TUI: Enter /memory get <key>
TUI->>Main: Parse subcommand via splitn()
Main->>Runtime: try_lock() for non-blocking access
Runtime-->>Main: Lock acquired
Main->>Runtime: memory_read({key})
Runtime->>Store: Read from memory store
Store-->>Runtime: Return value
Runtime-->>Main: Return result string
Main->>TUI: Display memory content
TUI-->>User: Show output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a persistent memory system for the CLI, enabling the storage and retrieval of project-specific facts across sessions. It adds a new /memory command for manual management and enhances the runtime's ability to automatically capture facts from AI responses. Feedback highlights several critical issues: the reliance on the HOME environment variable breaks Windows compatibility, and using millisecond timestamps for keys can lead to data loss due to collisions. Furthermore, the implementation lacks proper error handling for disk persistence and performs inefficient disk writes within loops.
| let key = format!( | ||
| "auto_{}_{}", | ||
| "auto_{}", | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis(), | ||
| trimmed.len(), | ||
| ); |
There was a problem hiding this comment.
Using as_millis() for key generation is highly likely to cause collisions when multiple facts are detected in a single response, as the loop processes lines much faster than the clock resolution. This leads to data loss where only the last fact is stored. Using a hash of the content instead ensures uniqueness and makes the auto-save operation idempotent. Additionally, calling memory.save() inside the loop (line 1330) is inefficient as it triggers a disk write for every fact found.
| let key = format!( | |
| "auto_{}_{}", | |
| "auto_{}", | |
| std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap_or_default() | |
| .as_millis(), | |
| trimmed.len(), | |
| ); | |
| let key = format!( | |
| "auto_{:x}", | |
| trimmed.bytes().fold(0u64, |h, b| h.wrapping_mul(31).wrapping_add(u64::from(b))) | |
| ); |
| let memory_path = std::env::var_os("HOME").map(|h| { | ||
| let cwd = std::env::current_dir().unwrap_or_default(); | ||
| let project_hash = format!( | ||
| "{:x}", | ||
| cwd.to_string_lossy() | ||
| .bytes() | ||
| .fold(0u64, |h, b| h.wrapping_mul(31).wrapping_add(u64::from(b))) | ||
| ); | ||
| std::path::PathBuf::from(h) | ||
| .join(".local/share/magic-code/memory") | ||
| .join(format!("{project_hash}.json")) | ||
| }); |
There was a problem hiding this comment.
The memory system is currently broken on Windows because HOME is not a standard environment variable (Windows uses USERPROFILE or APPDATA). When HOME is missing, memory_path becomes None, and the MemoryStore is never initialized, leading to 'Memory not configured' errors. Furthermore, current_dir().unwrap_or_default() will cause multiple projects to share the same 0.json memory file if the current directory cannot be resolved.
| let out = store.handle_write(input); | ||
| let _ = store.save(); | ||
| out |
There was a problem hiding this comment.
The result of store.save() is ignored. If persistence fails (e.g., due to disk space or permission issues), the user will receive a success message from handle_write, but the data won't actually be saved. It is safer to check the result and report any I/O errors.
let out = store.handle_write(input);
if let Err(e) = store.save() {
return format!("Error persisting memory: {e}");
}
out![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-cli/src/main.rs`:
- Around line 1097-1113: The handlers for the "get", "set", and "delete"
branches currently allow empty keys; update each branch in the match so that you
extract the key from parts (as you already do), validate that key is non-empty
(and for "set" also ensure value is non-empty if desired), and if the key is
empty return or push an error output to app.output_lines instead of calling
rt.memory_read/rt.memory_write; keep using the same identifiers (parts, key,
value, rt.memory_read, rt.memory_write, app.output_lines) so the change is
localized to those branches and preserves existing behavior when the key is
valid.
- Around line 390-405: Memory is only being attached to the ConversationRuntime
in the TUI path, leaving single-run mode without memory; move or duplicate the
initialization so the same memory is set for the runtime used in run_single.
Locate the memory_path construction and the call
rt.set_memory(mc_core::MemoryStore::load(path, 200)) and ensure it executes for
the runtime used by run_single (either by moving this block to before branching
where rt is created or by invoking rt.set_memory(...) after the
ConversationRuntime instance is created in run_single) so
memory_read/memory_write are configured in non-interactive runs as well.
In `@mc/crates/mc-core/src/runtime.rs`:
- Around line 1322-1328: The current auto key generation (the variable key built
with format!("auto_{}",
SystemTime::now().duration_since(UNIX_EPOCH).as_millis())) can collide when
multiple facts are created in one pass; replace it with a collision-resistant
identifier (e.g., use uuid::Uuid::new_v4() or include higher-resolution time + a
per-process/per-thread counter or random nonce) so keys are unique. Update the
code that builds key (the format!("auto_{}" ...) expression) to produce
something like "auto_{UUID}" or "auto_{nanos}_{counter}" to avoid overwrites and
ensure uniqueness across concurrent inserts.
- Around line 184-190: The memory_write function is currently swallowing
persistence errors by calling store.save() and ignoring its Result; update
memory_write (and its call sites) to surface save failures instead of discarding
them—either change memory_write's signature to return a Result<String, E> (or a
domain error type) and propagate the Err from store.save(), or, if you must keep
a String return, detect Err from store.save() and return a clear failure string
including the error; ensure you call store.handle_write(input) as before but
check the Result from store.save() and propagate or report that error rather
than ignoring it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc1b378b-8f3b-4f8c-9992-fe53a63a7891
📒 Files selected for processing (2)
mc/crates/mc-cli/src/main.rsmc/crates/mc-core/src/runtime.rs
| // Initialize persistent memory | ||
| let memory_path = std::env::var_os("HOME").map(|h| { | ||
| let cwd = std::env::current_dir().unwrap_or_default(); | ||
| let project_hash = format!( | ||
| "{:x}", | ||
| cwd.to_string_lossy() | ||
| .bytes() | ||
| .fold(0u64, |h, b| h.wrapping_mul(31).wrapping_add(u64::from(b))) | ||
| ); | ||
| std::path::PathBuf::from(h) | ||
| .join(".local/share/magic-code/memory") | ||
| .join(format!("{project_hash}.json")) | ||
| }); | ||
| if let Some(ref path) = memory_path { | ||
| rt.set_memory(mc_core::MemoryStore::load(path, 200)); | ||
| } |
There was a problem hiding this comment.
Memory is only initialized for TUI; single-run mode remains unconfigured.
This block fixes run_tui, but run_single still builds ConversationRuntime without set_memory(...). In non-interactive mode, memory_read/memory_write will still return “Memory not configured”.
Proposed fix
async fn run_single(
@@
) -> Result<()> {
@@
let mut runtime =
mc_core::ConversationRuntime::new(model.to_string(), max_tokens, system.to_string());
+ if let Some(path) = std::env::var_os("HOME").map(|h| {
+ let cwd = std::env::current_dir().unwrap_or_default();
+ let project_hash = format!(
+ "{:x}",
+ cwd.to_string_lossy()
+ .bytes()
+ .fold(0u64, |h, b| h.wrapping_mul(31).wrapping_add(u64::from(b)))
+ );
+ std::path::PathBuf::from(h)
+ .join(".local/share/magic-code/memory")
+ .join(format!("{project_hash}.json"))
+ }) {
+ runtime.set_memory(mc_core::MemoryStore::load(&path, 200));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Initialize persistent memory | |
| let memory_path = std::env::var_os("HOME").map(|h| { | |
| let cwd = std::env::current_dir().unwrap_or_default(); | |
| let project_hash = format!( | |
| "{:x}", | |
| cwd.to_string_lossy() | |
| .bytes() | |
| .fold(0u64, |h, b| h.wrapping_mul(31).wrapping_add(u64::from(b))) | |
| ); | |
| std::path::PathBuf::from(h) | |
| .join(".local/share/magic-code/memory") | |
| .join(format!("{project_hash}.json")) | |
| }); | |
| if let Some(ref path) = memory_path { | |
| rt.set_memory(mc_core::MemoryStore::load(path, 200)); | |
| } | |
| async fn run_single( | |
| /* parameters */ | |
| ) -> Result<()> { | |
| let mut runtime = | |
| mc_core::ConversationRuntime::new(model.to_string(), max_tokens, system.to_string()); | |
| if let Some(path) = std::env::var_os("HOME").map(|h| { | |
| let cwd = std::env::current_dir().unwrap_or_default(); | |
| let project_hash = format!( | |
| "{:x}", | |
| cwd.to_string_lossy() | |
| .bytes() | |
| .fold(0u64, |h, b| h.wrapping_mul(31).wrapping_add(u64::from(b))) | |
| ); | |
| std::path::PathBuf::from(h) | |
| .join(".local/share/magic-code/memory") | |
| .join(format!("{project_hash}.json")) | |
| }) { | |
| runtime.set_memory(mc_core::MemoryStore::load(&path, 200)); | |
| } | |
| /* rest of run_single function */ | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-cli/src/main.rs` around lines 390 - 405, Memory is only being
attached to the ConversationRuntime in the TUI path, leaving single-run mode
without memory; move or duplicate the initialization so the same memory is set
for the runtime used in run_single. Locate the memory_path construction and the
call rt.set_memory(mc_core::MemoryStore::load(path, 200)) and ensure it executes
for the runtime used by run_single (either by moving this block to before
branching where rt is created or by invoking rt.set_memory(...) after the
ConversationRuntime instance is created in run_single) so
memory_read/memory_write are configured in non-interactive runs as well.
| "get" => { | ||
| let key = parts.get(1).copied().unwrap_or(""); | ||
| let output = rt.memory_read(&serde_json::json!({"key": key})); | ||
| app.output_lines.push(output); | ||
| } | ||
| "set" => { | ||
| let key = parts.get(1).copied().unwrap_or(""); | ||
| let value = parts.get(2).copied().unwrap_or(""); | ||
| let output = rt | ||
| .memory_write(&serde_json::json!({"key": key, "value": value})); | ||
| app.output_lines.push(output); | ||
| } | ||
| "delete" => { | ||
| let key = parts.get(1).copied().unwrap_or(""); | ||
| let output = rt | ||
| .memory_write(&serde_json::json!({"key": key, "delete": true})); | ||
| app.output_lines.push(output); |
There was a problem hiding this comment.
Validate memory keys for get/set/delete commands.
/memory get, /memory set, and /memory delete currently allow empty keys, which can create ambiguous or unintended entries.
Proposed fix
"get" => {
let key = parts.get(1).copied().unwrap_or("");
+ if key.is_empty() {
+ app.output_lines.push("Usage: /memory get <key>".into());
+ continue;
+ }
let output = rt.memory_read(&serde_json::json!({"key": key}));
app.output_lines.push(output);
}
"set" => {
let key = parts.get(1).copied().unwrap_or("");
let value = parts.get(2).copied().unwrap_or("");
+ if key.is_empty() {
+ app.output_lines.push("Usage: /memory set <key> <value>".into());
+ continue;
+ }
let output = rt
.memory_write(&serde_json::json!({"key": key, "value": value}));
app.output_lines.push(output);
}
"delete" => {
let key = parts.get(1).copied().unwrap_or("");
+ if key.is_empty() {
+ app.output_lines.push("Usage: /memory delete <key>".into());
+ continue;
+ }
let output = rt
.memory_write(&serde_json::json!({"key": key, "delete": true}));
app.output_lines.push(output);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-cli/src/main.rs` around lines 1097 - 1113, The handlers for the
"get", "set", and "delete" branches currently allow empty keys; update each
branch in the match so that you extract the key from parts (as you already do),
validate that key is non-empty (and for "set" also ensure value is non-empty if
desired), and if the key is empty return or push an error output to
app.output_lines instead of calling rt.memory_read/rt.memory_write; keep using
the same identifiers (parts, key, value, rt.memory_read, rt.memory_write,
app.output_lines) so the change is localized to those branches and preserves
existing behavior when the key is valid.
| pub fn memory_write(&mut self, input: &serde_json::Value) -> String { | ||
| match &mut self.memory { | ||
| Some(store) => { | ||
| let out = store.handle_write(input); | ||
| let _ = store.save(); | ||
| out | ||
| } |
There was a problem hiding this comment.
Do not swallow persistence failures in memory writes.
Line 188 ignores store.save() errors, so /memory set can appear successful even when nothing is persisted.
Proposed fix
pub fn memory_write(&mut self, input: &serde_json::Value) -> String {
match &mut self.memory {
Some(store) => {
let out = store.handle_write(input);
- let _ = store.save();
- out
+ match store.save() {
+ Ok(()) => out,
+ Err(e) => format!("{out}\n⚠ Failed to persist memory: {e}"),
+ }
}
None => "Memory not configured".into(),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn memory_write(&mut self, input: &serde_json::Value) -> String { | |
| match &mut self.memory { | |
| Some(store) => { | |
| let out = store.handle_write(input); | |
| let _ = store.save(); | |
| out | |
| } | |
| pub fn memory_write(&mut self, input: &serde_json::Value) -> String { | |
| match &mut self.memory { | |
| Some(store) => { | |
| let out = store.handle_write(input); | |
| match store.save() { | |
| Ok(()) => out, | |
| Err(e) => format!("{out}\n⚠ Failed to persist memory: {e}"), | |
| } | |
| } | |
| None => "Memory not configured".into(), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/runtime.rs` around lines 184 - 190, The memory_write
function is currently swallowing persistence errors by calling store.save() and
ignoring its Result; update memory_write (and its call sites) to surface save
failures instead of discarding them—either change memory_write's signature to
return a Result<String, E> (or a domain error type) and propagate the Err from
store.save(), or, if you must keep a String return, detect Err from store.save()
and return a clear failure string including the error; ensure you call
store.handle_write(input) as before but check the Result from store.save() and
propagate or report that error rather than ignoring it.
| let key = format!( | ||
| "auto_{}_{}", | ||
| "auto_{}", | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis(), | ||
| trimmed.len(), | ||
| ); |
There was a problem hiding this comment.
Auto-memory keys can collide and overwrite facts.
Lines 1322-1328 use millisecond timestamp only. When multiple facts are detected in one pass, keys can collide and earlier facts get overwritten.
Proposed fix
- for line in text.lines() {
+ for (line_idx, line) in text.lines().enumerate() {
let trimmed = line.trim();
// ...
if is_fact {
let key = format!(
- "auto_{}",
+ "auto_{}_{}",
std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.unwrap_or_default()
.as_millis(),
+ line_idx,
);
memory.set(&key, trimmed);
let _ = memory.save();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/runtime.rs` around lines 1322 - 1328, The current auto
key generation (the variable key built with format!("auto_{}",
SystemTime::now().duration_since(UNIX_EPOCH).as_millis())) can collide when
multiple facts are created in one pass; replace it with a collision-resistant
identifier (e.g., use uuid::Uuid::new_v4() or include higher-resolution time + a
per-process/per-thread counter or random nonce) so keys are unique. Update the
code that builds key (the format!("auto_{}" ...) expression) to produce
something like "auto_{UUID}" or "auto_{nanos}_{counter}" to avoid overwrites and
ensure uniqueness across concurrent inserts.
1 participant
Critical Bug Fix
MemoryStore was never initialized in main.rs. All 3 memory features were dead:
memory_read/memory_writetools → 'Memory not configured'auto_save_memory()→ early return (None)to_prompt_section()→ empty stringFixes
~/.local/share/magic-code/memory/{hash}.json/memory list|get|set|deletenow worksruntime.memory_read()/memory_write()for TUIMemory Architecture (now working)
274 tests, 0 fail.
Summary by CodeRabbit
/memoryslash-command enabling manual memory management with subcommands:list,get,set, anddelete.