From cc11d3e5a0f0f44cf2fead3c4ba77eb353acff24 Mon Sep 17 00:00:00 2001 From: Taylor Eernisse Date: Sun, 8 Feb 2026 08:07:59 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20peer=20review=20=E2=80=94=205=20correctn?= =?UTF-8?q?ess=20bugs=20across=20who,=20db,=20lock,=20embedding,=20main?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive peer code review identified and fixed the following: 1. who.rs: @-prefixed path routing used `target` (with @) instead of `clean` (stripped) when checking for '/' and passing to Expert mode, causing `lore who @src/auth/` to silently return zero results because the SQL LIKE matched against `@src/auth/%` which never exists. 2. db.rs: After ROLLBACK TO savepoint on migration failure, the savepoint was never RELEASEd, leaving it active on the connection. Fixed in both run_migrations() and run_migrations_from_dir(). 3. lock.rs: Multiple acquire() calls (e.g. re-acquiring a stale lock) replaced the heartbeat_handle without stopping the old thread, causing two concurrent heartbeat writers competing on the same lock row. Now signals the old thread to stop and joins it before spawning a new one. 4. chunk_ids.rs: encode_rowid() had no guard for chunk_index >= 1000 (CHUNK_ROWID_MULTIPLIER), which would cause rowid collisions between adjacent documents. Added range assertion [0, 1000). 5. main.rs: Fallback JSON error formatting in handle_auth_test interpolated LoreError Display output without escaping quotes or backslashes, potentially producing malformed JSON for robot-mode consumers. Now escapes both characters before interpolation. Co-Authored-By: Claude Opus 4.6 --- src/cli/commands/who.rs | 4 ++-- src/core/db.rs | 2 ++ src/core/lock.rs | 7 +++++++ src/embedding/chunk_ids.rs | 4 ++++ src/main.rs | 7 ++++++- 5 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/cli/commands/who.rs b/src/cli/commands/who.rs index a33a0fc..d878820 100644 --- a/src/cli/commands/who.rs +++ b/src/cli/commands/who.rs @@ -54,9 +54,9 @@ fn resolve_mode<'a>(args: &'a WhoArgs) -> Result> { // Disambiguation: if target contains '/', it's a file path. // GitLab usernames never contain '/'. // Root files (no '/') require --path. - if target.contains('/') { + if clean.contains('/') { return Ok(WhoMode::Expert { - path: normalize_repo_path(target), + path: normalize_repo_path(clean), }); } return Ok(WhoMode::Workload { username: clean }); diff --git a/src/core/db.rs b/src/core/db.rs index 5858c7b..0e2feb5 100644 --- a/src/core/db.rs +++ b/src/core/db.rs @@ -140,6 +140,7 @@ pub fn run_migrations(conn: &Connection) -> Result<()> { } Err(e) => { let _ = conn.execute_batch(&format!("ROLLBACK TO {}", savepoint_name)); + let _ = conn.execute_batch(&format!("RELEASE {}", savepoint_name)); return Err(LoreError::MigrationFailed { version, message: e.to_string(), @@ -216,6 +217,7 @@ pub fn run_migrations_from_dir(conn: &Connection, migrations_dir: &Path) -> Resu } Err(e) => { let _ = conn.execute_batch(&format!("ROLLBACK TO {}", savepoint_name)); + let _ = conn.execute_batch(&format!("RELEASE {}", savepoint_name)); return Err(LoreError::MigrationFailed { version, message: e.to_string(), diff --git a/src/core/lock.rs b/src/core/lock.rs index 2e43ed2..056dad6 100644 --- a/src/core/lock.rs +++ b/src/core/lock.rs @@ -135,6 +135,13 @@ impl AppLock { } fn start_heartbeat(&mut self) { + // Stop any existing heartbeat thread before starting a new one + if let Some(handle) = self.heartbeat_handle.take() { + self.released.store(true, Ordering::SeqCst); + let _ = handle.join(); + self.released.store(false, Ordering::SeqCst); + } + let name = self.name.clone(); let owner = self.owner.clone(); let interval = Duration::from_millis(self.heartbeat_interval_ms); diff --git a/src/embedding/chunk_ids.rs b/src/embedding/chunk_ids.rs index e834585..4214b03 100644 --- a/src/embedding/chunk_ids.rs +++ b/src/embedding/chunk_ids.rs @@ -1,6 +1,10 @@ pub const CHUNK_ROWID_MULTIPLIER: i64 = 1000; pub fn encode_rowid(document_id: i64, chunk_index: i64) -> i64 { + assert!( + (0..CHUNK_ROWID_MULTIPLIER).contains(&chunk_index), + "chunk_index {chunk_index} out of range [0, {CHUNK_ROWID_MULTIPLIER})" + ); document_id .checked_mul(CHUNK_ROWID_MULTIPLIER) .and_then(|v| v.checked_add(chunk_index)) diff --git a/src/main.rs b/src/main.rs index e18e5c8..ccd5126 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1217,7 +1217,12 @@ async fn handle_auth_test( eprintln!( "{}", serde_json::to_string(&output).unwrap_or_else(|_| { - format!(r#"{{"error":{{"code":"{}","message":"{}"}}}}"#, e.code(), e) + let msg = e.to_string().replace('\\', "\\\\").replace('"', "\\\""); + format!( + r#"{{"error":{{"code":"{}","message":"{}"}}}}"#, + e.code(), + msg + ) }) ); } else {