fix: migration drops mem_branches on every startup + UTF-8 panic in diff preview#171
fix: migration drops mem_branches on every startup + UTF-8 panic in diff preview#171
Conversation
…iff preview Two bugs fixed: 1. query_has_rows returns false on MatrixOne because COUNT(*) > 0 returns a bool that sqlx cannot decode as i64. This causes migrate_user() to DROP and recreate mem_branches (and potentially other tables) on every MCP/API startup, losing all branch registrations. Fix: strip '> 0' suffix and compare in Rust. 2. memory_diff panics on multi-byte UTF-8 content when truncating the preview to 80 bytes. The byte index can land inside a multi-byte character (e.g. CJK). Fix: use floor_char_boundary(). Both bugs affect MCP embedded mode and API mode equally.
There was a problem hiding this comment.
Pull request overview
Fixes two MatrixOne/MCP stability issues: preventing accidental mem_branches metadata resets during startup migrations and avoiding UTF-8 slicing panics when generating memory_diff previews.
Changes:
- Adjust
query_has_rows()to normalizeCOUNT(*) > 0-style probes for MatrixOne/sqlx decoding compatibility. - Truncate
memory_diffpreview strings usingfloor_char_boundary()to avoid invalid UTF-8 slicing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
memoria/crates/memoria-storage/src/store.rs |
Updates query_has_rows() SQL normalization used by migration probes that gate schema changes. |
memoria/crates/memoria-mcp/src/git_tools.rs |
Fixes diff preview truncation to avoid panics on multibyte UTF-8 content. |
Comments suppressed due to low confidence (1)
memoria/crates/memoria-storage/src/store.rs:52
query_has_rows()currently swallows all SQLx errors via.await.unwrap_or(0), treating failures as “no rows”. Inmigrate_user()this boolean is used to decide destructive paths (e.g., dropping/recreatingmem_branches), so transient DB errors or decode errors can silently trigger data-lossy migrations. Consider returningResult<bool, MemoriaError>(or at least logging and failing startup) for probe failures that gate DROP/CREATE behavior.
sqlx::query_scalar::<_, i64>(&sql)
.fetch_one(pool)
.await
.unwrap_or(0)
> 0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Strip "> 0" suffix if present — MatrixOne returns bool for "COUNT(*) > 0" | ||
| // which sqlx cannot decode as i64. Use plain COUNT(*) and compare in Rust. | ||
| let sql = sql | ||
| .trim() | ||
| .trim_end_matches(';') | ||
| .trim() | ||
| .strip_suffix("> 0") | ||
| .map(|s| s.trim().to_string()) | ||
| .unwrap_or_else(|| sql.to_string()); |
There was a problem hiding this comment.
The SQL rewrite in query_has_rows() only strips a literal "> 0" at the end of the whole SQL string. That won’t affect queries of the form SELECT COUNT(*) > 0 FROM ... (where > 0 is inside the SELECT expression), which is the failure mode described in the PR. Also, when no suffix matches, the code falls back to sql.to_string() (the original untrimmed input), so the earlier trim_end_matches(';') is effectively ignored. Recommend normalizing the query using the trimmed value and rewriting the COUNT(*) > 0 expression (or updating callers to pass plain COUNT(*)).
| let preview = if r.content.len() > 80 { | ||
| format!("{}...", &r.content[..80]) | ||
| let end = r.content.floor_char_boundary(80); | ||
| format!("{}...", &r.content[..end]) |
There was a problem hiding this comment.
Please add a regression test for the diff preview truncation with multi-byte UTF-8 (e.g., CJK) to ensure memory_diff never panics when slicing previews. This file already has unit tests, so it should be straightforward to cover the new floor_char_boundary() logic.
Problem
Two bugs that affect both MCP embedded mode and API mode:
1.
mem_branchesdropped on every startupquery_has_rows()instore.rsexecutes SQL likeSELECT COUNT(*) > 0 FROM .... MatrixOne returns a bool forCOUNT(*) > 0, but sqlx'squery_scalar::<_, i64>cannot decode bool →unwrap_or(0)→ always returns false.This causes
migrate_user()to take theDROP TABLE IF EXISTS mem_branches+CREATE TABLEpath on every startup, wiping all branch registrations. The branch tables themselves survive (they're MatrixOne CoW tables), but the metadata linking names to tables is lost.Fix: Strip the
> 0suffix from the SQL and compare in Rust instead.2. UTF-8 panic in
memory_diffpreviewgit_tools.rstruncates diff content preview with&r.content[..80]. For multi-byte UTF-8 content (e.g. CJK characters), byte index 80 can land inside a character, causing a panic that crashes the MCP/API process.Fix: Use
floor_char_boundary(80)to find the nearest valid character boundary.Changes
memoria-storage/src/store.rs:query_has_rows()strips "> 0" suffix before executingmemoria-mcp/src/git_tools.rs: diff preview usesfloor_char_boundary()