Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion mc/crates/mc-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,22 @@ async fn run_tui(
if !hooks.is_empty() {
rt.set_hooks(mc_tools::HookEngine::new(hooks));
}
// 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"))
});
Comment on lines +391 to +402
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

if let Some(ref path) = memory_path {
rt.set_memory(mc_core::MemoryStore::load(path, 200));
}
Comment on lines +390 to +405
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
// 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.

// Load hierarchical instructions (CLAUDE.md, AGENTS.md from root to cwd)
let cwd = std::env::current_dir().unwrap_or_default();
let instructions = mc_config::load_hierarchical_instructions(&cwd);
Expand Down Expand Up @@ -1070,7 +1086,40 @@ async fn run_tui(
}
}
PendingCommand::Memory(cmd) => {
app.output_lines.push(format!("📌 memory: {cmd}"));
if let Ok(mut rt) = runtime.try_lock() {
let parts: Vec<&str> = cmd.splitn(3, ' ').collect();
match parts.first().copied().unwrap_or("list") {
"list" | "" => {
let output = rt.memory_read(&serde_json::json!({}));
app.output_lines.push("📌 Project Memory:".into());
app.output_lines.push(output);
}
"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);
Comment on lines +1097 to +1113
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

}
_ => {
app.output_lines.push("Usage: /memory [list|get <key>|set <key> <value>|delete <key>]".into());
}
}
} else {
app.output_lines
.push("Memory not available (runtime busy)".into());
}
}
PendingCommand::ThinkingToggle => {
app.output_lines.push("💭 Thinking toggled".into());
Expand Down Expand Up @@ -1850,6 +1899,8 @@ fn build_system_prompt(project: &mc_config::ProjectContext) -> String {
- `lsp_query`: Query the Language Server for diagnostics, definitions, references. Use for type errors and navigation.\n\n\
## Context & Memory\n\
- `memory_read`/`memory_write`: Read/write persistent project facts across sessions.\n\
- Proactively save useful facts: test commands, framework versions, coding conventions, architecture decisions.\n\
- Use `memory_write` after discovering project patterns (e.g. \"test_cmd\" = \"cargo test\").\n\
- `web_fetch`: Fetch content from a URL. Use to read documentation or API specs.\n\
- `web_search`: Search the web for current information.\n\
- `ask_user`: Ask the user a clarifying question when requirements are ambiguous.\n\n\
Expand Down
43 changes: 35 additions & 8 deletions mc/crates/mc-core/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,26 @@ impl ConversationRuntime {
self.memory = Some(memory);
}

/// Read from memory (for /memory command).
pub fn memory_read(&self, input: &serde_json::Value) -> String {
match &self.memory {
Some(store) => store.handle_read(input),
None => "Memory not configured".into(),
}
}

/// Write to memory (for /memory command).
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
Comment on lines +187 to +189
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

}
Comment on lines +184 to +190
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

None => "Memory not configured".into(),
}
}

/// Attach an image to the next user message.
pub fn attach_image(&mut self, path: String, media_type: String) {
self.pending_image = Some((path, media_type));
Expand Down Expand Up @@ -1282,22 +1302,29 @@ Fix this before continuing."
let Some(ref mut memory) = self.memory else {
return;
};
// Heuristic: save lines that look like project facts
for line in text.lines() {
let trimmed = line.trim();
if (trimmed.starts_with("Note:")
if trimmed.len() < 20 || trimmed.len() > 300 {
continue;
}
// Detect project facts worth remembering
let is_fact = trimmed.starts_with("Note:")
|| trimmed.starts_with("Remember:")
|| trimmed.contains("convention is"))
&& trimmed.len() > 20
&& trimmed.len() < 200
{
|| trimmed.contains("convention is")
|| trimmed.contains("always use")
|| trimmed.contains("project uses")
|| trimmed.contains("test command:")
|| trimmed.contains("configured with")
|| trimmed.contains("running on port")
|| trimmed.contains("database is")
|| trimmed.contains("deploy with");
if is_fact {
let key = format!(
"auto_{}_{}",
"auto_{}",
std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.unwrap_or_default()
.as_millis(),
trimmed.len(),
);
Comment on lines 1322 to 1328
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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)))
);

Comment on lines 1322 to 1328
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

memory.set(&key, trimmed);
let _ = memory.save();
Expand Down
Loading