fix: address review feedback — transaction safety, patch mode, formatting

- Wrap libSQL save_version in a transaction to prevent race condition
  where concurrent writers could allocate the same version number
- Make content optional in memory_write when in patch mode (old_string
  present) — LLM no longer forced to provide unused content param
- Improve metadata update error handling with explicit match arms
- Run cargo fmt across all files

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

src/db/libsql/workspace.rs

@@ -855,9 +855,10 @@ impl WorkspaceStore for LibSqlBackend {
                 reason: e.to_string(),
             })?;
         let now = fmt_ts(&Utc::now());
-        let meta_str = serde_json::to_string(metadata).map_err(|e| WorkspaceError::SearchFailed {
-            reason: format!("Failed to serialize metadata: {e}"),
-        })?;
+        let meta_str =
+            serde_json::to_string(metadata).map_err(|e| WorkspaceError::SearchFailed {
+                reason: format!("Failed to serialize metadata: {e}"),
+            })?;
         conn.execute(
             "UPDATE memory_documents SET metadata = ?2, updated_at = ?3 WHERE id = ?1",
             params![id.to_string(), meta_str, now],
@@ -899,9 +900,13 @@ impl WorkspaceStore for LibSqlBackend {
             })?;
 
         let mut docs = Vec::new();
-        while let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed {
-            reason: format!("Failed to read config document row: {e}"),
-        })? {
+        while let Some(row) = rows
+            .next()
+            .await
+            .map_err(|e| WorkspaceError::SearchFailed {
+                reason: format!("Failed to read config document row: {e}"),
+            })?
+        {
             docs.push(row_to_memory_document(&row));
         }
         Ok(docs)
@@ -926,8 +931,17 @@ impl WorkspaceStore for LibSqlBackend {
         let doc_id = document_id.to_string();
         let now = fmt_ts(&Utc::now());
 
-        // Get next version number
-        let mut rows = conn
+        // Use a transaction to prevent race conditions: the SELECT and INSERT
+        // must be atomic so concurrent writers don't allocate the same version.
+        let tx = conn
+            .transaction()
+            .await
+            .map_err(|e| WorkspaceError::SearchFailed {
+                reason: format!("Failed to start transaction: {e}"),
+            })?;
+
+        // Get next version number (inside transaction — serializes writers)
+        let mut rows = tx
             .query(
                 "SELECT COALESCE(MAX(version), 0) + 1 FROM memory_document_versions WHERE document_id = ?1",
                 params![doc_id.clone()],
@@ -937,27 +951,45 @@ impl WorkspaceStore for LibSqlBackend {
                 reason: format!("Failed to get next version number: {e}"),
             })?;
 
-        let next_version = if let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed {
-            reason: format!("Failed to read version number: {e}"),
-        })? {
+        let next_version = if let Some(row) =
+            rows.next()
+                .await
+                .map_err(|e| WorkspaceError::SearchFailed {
+                    reason: format!("Failed to read version number: {e}"),
+                })? {
             get_i64(&row, 0) as i32
         } else {
             1
         };
+        drop(rows);
 
-        conn.execute(
+        tx.execute(
             r#"
             INSERT INTO memory_document_versions
                 (id, document_id, version, content, content_hash, created_at, changed_by)
             VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)
             "#,
-            params![id, doc_id, next_version as i64, content, content_hash, now, changed_by],
+            params![
+                id,
+                doc_id,
+                next_version as i64,
+                content,
+                content_hash,
+                now,
+                changed_by
+            ],
         )
         .await
         .map_err(|e| WorkspaceError::SearchFailed {
             reason: format!("Failed to save version: {e}"),
         })?;
 
+        tx.commit()
+            .await
+            .map_err(|e| WorkspaceError::SearchFailed {
+                reason: format!("Failed to commit version: {e}"),
+            })?;
+
         Ok(next_version)
     }
 
@@ -999,12 +1031,16 @@ impl WorkspaceStore for LibSqlBackend {
             })?;
 
         Ok(DocumentVersion {
-            id: get_text(&row, 0).parse().map_err(|e| WorkspaceError::SearchFailed {
-                reason: format!("Invalid version UUID: {e}"),
-            })?,
-            document_id: get_text(&row, 1).parse().map_err(|e| WorkspaceError::SearchFailed {
-                reason: format!("Invalid document UUID: {e}"),
-            })?,
+            id: get_text(&row, 0)
+                .parse()
+                .map_err(|e| WorkspaceError::SearchFailed {
+                    reason: format!("Invalid version UUID: {e}"),
+                })?,
+            document_id: get_text(&row, 1)
+                .parse()
+                .map_err(|e| WorkspaceError::SearchFailed {
+                    reason: format!("Invalid document UUID: {e}"),
+                })?,
             version: get_i64(&row, 2) as i32,
             content: get_text(&row, 3),
             content_hash: get_text(&row, 4),
@@ -1041,9 +1077,13 @@ impl WorkspaceStore for LibSqlBackend {
             })?;
 
         let mut versions = Vec::new();
-        while let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed {
-            reason: format!("Failed to read version row: {e}"),
-        })? {
+        while let Some(row) = rows
+            .next()
+            .await
+            .map_err(|e| WorkspaceError::SearchFailed {
+                reason: format!("Failed to read version row: {e}"),
+            })?
+        {
             versions.push(VersionSummary {
                 version: get_i64(&row, 0) as i32,
                 content_hash: get_text(&row, 1),
@@ -1074,9 +1114,13 @@ impl WorkspaceStore for LibSqlBackend {
                 reason: format!("Failed to get latest version number: {e}"),
             })?;
 
-        if let Some(row) = rows.next().await.map_err(|e| WorkspaceError::SearchFailed {
-            reason: format!("Failed to read version number: {e}"),
-        })? {
+        if let Some(row) = rows
+            .next()
+            .await
+            .map_err(|e| WorkspaceError::SearchFailed {
+                reason: format!("Failed to read version number: {e}"),
+            })?
+        {
             // MAX returns NULL if no rows — libsql returns Null for the value
             let val = row.get::<libsql::Value>(0).ok();
             match val {

src/tools/builtin/memory.rs

@@ -265,7 +265,7 @@ impl Tool for MemoryWriteTool {
                     "default": false
                 }
             },
-            "required": ["content"]
+            "required": []
         })
     }
 
@@ -276,7 +276,9 @@ impl Tool for MemoryWriteTool {
     ) -> Result<ToolOutput, ToolError> {
         let start = std::time::Instant::now();
 
-        let content = require_str(&params, "content")?;
+        // In patch mode (old_string present), content is not required.
+        let is_patch_mode = params.get("old_string").and_then(|v| v.as_str()).is_some();
+        let content = params.get("content").and_then(|v| v.as_str()).unwrap_or("");
 
         let target = params
             .get("target")
@@ -316,9 +318,9 @@ impl Tool for MemoryWriteTool {
             return Ok(ToolOutput::success(output, start.elapsed()));
         }
 
-        if content.trim().is_empty() {
+        if !is_patch_mode && content.trim().is_empty() {
             return Err(ToolError::InvalidParameters(
-                "content cannot be empty".to_string(),
+                "content cannot be empty (use old_string/new_string for patch mode)".to_string(),
             ));
         }
 
@@ -491,14 +493,20 @@ impl Tool for MemoryWriteTool {
         }
 
         // Apply metadata if provided (after write/append, works for all targets).
+        // We read the document once to get its ID — this is a hot read right
+        // after the write, so it's effectively free (same DB connection/cache).
         if let Some(meta) = params.get("metadata")
             && meta.is_object()
         {
-            // Read the document to get its ID
-            if let Ok(doc) = workspace.read(&resolved_path).await
-                && let Err(e) = workspace.update_metadata(doc.id, meta).await
-            {
-                tracing::warn!(path = %resolved_path, "failed to update metadata: {e}");
+            match workspace.read(&resolved_path).await {
+                Ok(doc) => {
+                    if let Err(e) = workspace.update_metadata(doc.id, meta).await {
+                        tracing::warn!(path = %resolved_path, "failed to update metadata: {e}");
+                    }
+                }
+                Err(e) => {
+                    tracing::warn!(path = %resolved_path, "failed to read doc for metadata update: {e}");
+                }
             }
         }
 

src/workspace/document.rs

@@ -540,11 +540,17 @@ mod tests {
         });
         let meta = DocumentMetadata::from_value(&value);
         assert_eq!(meta.skip_indexing, Some(true));
-        assert_eq!(meta.extra.get("custom_field").and_then(|v| v.as_str()), Some("hello"));
+        assert_eq!(
+            meta.extra.get("custom_field").and_then(|v| v.as_str()),
+            Some("hello")
+        );
 
         // Round-trip preserves the field
         let back = meta.to_value();
-        assert_eq!(back.get("custom_field").and_then(|v| v.as_str()), Some("hello"));
+        assert_eq!(
+            back.get("custom_field").and_then(|v| v.as_str()),
+            Some("hello")
+        );
     }
 
     #[test]

src/workspace/hygiene.rs

@@ -174,11 +174,7 @@ pub async fn run_if_due(workspace: &Workspace, config: &HygieneConfig) -> Hygien
         match cleanup_directory(workspace, &directory, hygiene.retention_days).await {
             Ok(deleted) => {
                 if deleted > 0 {
-                    tracing::info!(
-                        directory,
-                        deleted,
-                        "memory hygiene: cleaned directory"
-                    );
+                    tracing::info!(directory, deleted, "memory hygiene: cleaned directory");
                 }
                 report.directories_cleaned.push((directory, deleted));
             }
@@ -454,11 +450,7 @@ mod tests {
         }
 
         /// Helper to seed a .config document with hygiene metadata on a directory.
-        async fn seed_hygiene_config(
-            workspace: &Workspace,
-            directory: &str,
-            retention_days: u32,
-        ) {
+        async fn seed_hygiene_config(workspace: &Workspace, directory: &str, retention_days: u32) {
             let config_path = format!("{}.config", directory);
             // Create the .config document with empty content
             workspace
@@ -492,9 +484,7 @@ mod tests {
             ws.write("daily/2024-01-15.md", "Old log")
                 .await
                 .expect("write log");
-            ws.write("daily/.config", "")
-                .await
-                .expect("write config");
+            ws.write("daily/.config", "").await.expect("write config");
 
             // Run cleanup with 0-day retention (deletes everything old)
             let deleted = cleanup_directory(&ws, "daily/", 0)