fix(fs): atomic writes through one sink; file_write takes the path lock

crates/tinytools-std/src/filesystem/apply_patch/mod.rs

@@ -14,6 +14,7 @@
 //! containing `x`). An empty `old_string` against a file that *does* exist is
 //! still an error — "replace nothing" is ambiguous, not a create.
 
+use super::file_sink::atomic_write;
 use super::gate::{FsGate, gate_for_context};
 use crate::file_state;
 use async_trait::async_trait;
@@ -390,7 +391,7 @@ impl ApplyPatchTool {
         let mut written: Vec<&FileBuffer> = Vec::new();
         for buf in buffers.values() {
             let path = &buf.path;
-            if let Err(e) = tokio::fs::write(&buf.resolved, &buf.contents).await {
+            if let Err(e) = atomic_write(&buf.resolved, buf.contents.as_bytes()).await {
                 let restore_errors = restore_originals(&written).await;
                 let suffix = if restore_errors.is_empty() {
                     "; previously-written files restored from snapshot".to_string()
@@ -428,7 +429,7 @@ async fn restore_originals(written: &[&FileBuffer]) -> Vec<String> {
     let mut errors = Vec::new();
     for buf in written {
         let result = match &buf.original {
-            Some(original) => tokio::fs::write(&buf.resolved, original).await,
+            Some(original) => atomic_write(&buf.resolved, original.as_bytes()).await,
             // The batch created this file, so "restore" means remove it —
             // otherwise a failed multi-file patch leaves a half-written new
             // file behind and the caller cannot tell it apart from a success.

crates/tinytools-std/src/filesystem/csv_export/mod.rs

@@ -241,7 +241,7 @@ impl CsvExportTool {
         }
 
         // Write the CSV file
-        match tokio::fs::write(&resolved_target, &csv_content).await {
+        match super::file_sink::atomic_write(&resolved_target, csv_content.as_bytes()).await {
             Ok(()) => {
                 let size_display = if csv_bytes >= 1024 * 1024 {
                     format!("{:.1} MB", csv_bytes as f64 / (1024.0 * 1024.0))

crates/tinytools-std/src/filesystem/file_sink.rs

@@ -16,8 +16,10 @@
 
 use std::path::Path;
 use std::sync::Arc;
+use std::sync::atomic::{AtomicU64, Ordering};
 
 use async_trait::async_trait;
+use tokio::io::AsyncWriteExt;
 
 /// Where a tool's file writes land.
 #[async_trait]
@@ -32,10 +34,58 @@ pub(super) struct OsFileSink;
 #[async_trait]
 impl FileSink for OsFileSink {
     async fn write(&self, path: &Path, contents: &[u8]) -> std::io::Result<()> {
-        tokio::fs::write(path, contents).await
+        atomic_write(path, contents).await
     }
 }
 
+/// Monotonic counter for temp-file names within a process.
+static TEMP_FILE_SEQ: AtomicU64 = AtomicU64::new(0);
+
+/// Atomically replace `path`'s contents with `contents`.
+///
+/// Writes a fresh (`create_new`) sibling temp file in the same directory, so
+/// the rename stays on one filesystem, gives it the target's permissions, and
+/// `rename`s it over the target. A crash mid-write leaves the old file or the
+/// complete new one, never a truncation, and a symlink at `path` is replaced
+/// rather than followed. The rename installs a new inode: other hard links
+/// keep the old contents, and the file is owned by the writing user.
+pub(super) async fn atomic_write(path: &Path, contents: &[u8]) -> std::io::Result<()> {
+    let dir = path.parent().ok_or_else(|| {
+        std::io::Error::new(
+            std::io::ErrorKind::InvalidInput,
+            format!("{} has no parent directory", path.display()),
+        )
+    })?;
+    let name = path.file_name().unwrap_or_default().to_string_lossy();
+    let seq = TEMP_FILE_SEQ.fetch_add(1, Ordering::Relaxed);
+    let tmp = dir.join(format!(".{name}.{}.{seq}.tmp", std::process::id()));
+    let mut file = tokio::fs::OpenOptions::new()
+        .write(true)
+        .create_new(true)
+        .open(&tmp)
+        .await
+        .map_err(|e| std::io::Error::new(e.kind(), format!("stage temp file: {e}")))?;
+    let mut result = stage(&mut file, path, contents).await;
+    drop(file);
+    if result.is_ok() {
+        result = tokio::fs::rename(&tmp, path).await;
+    }
+    if result.is_err() {
+        let _ = tokio::fs::remove_file(&tmp).await;
+    }
+    result
+}
+
+/// Fill the temp file, copy the target's permissions onto it (or `+x` is
+/// lost on the rename), and flush it to disk before the rename.
+async fn stage(file: &mut tokio::fs::File, target: &Path, contents: &[u8]) -> std::io::Result<()> {
+    file.write_all(contents).await?;
+    if let Ok(meta) = tokio::fs::metadata(target).await {
+        file.set_permissions(meta.permissions()).await?;
+    }
+    file.sync_all().await
+}
+
 /// The sink a tool uses unless a caller installs another one.
 pub(super) fn os_sink() -> Arc<dyn FileSink> {
     Arc::new(OsFileSink)
@@ -57,3 +107,7 @@ impl FileSink for RefusingSink {
         ))
     }
 }
+
+#[cfg(test)]
+#[path = "file_sink_tests.rs"]
+mod tests;

crates/tinytools-std/src/filesystem/file_sink_tests.rs

@@ -0,0 +1,62 @@
+//! The OS sink replaces a file atomically: through a temp file and a rename.
+
+#![allow(clippy::unwrap_used)]
+
+use super::*;
+
+// A symlink swapped in after a tool's symlink check must not redirect the
+// write: the rename replaces the link itself, and its target is untouched.
+#[cfg(unix)]
+#[tokio::test]
+async fn a_write_replaces_a_symlink_instead_of_following_it() {
+    let dir = tempfile::tempdir().unwrap();
+    let outside = tempfile::tempdir().unwrap();
+    let secret = outside.path().join("secret.txt");
+    std::fs::write(&secret, "secret").unwrap();
+    let link = dir.path().join("a.txt");
+    std::os::unix::fs::symlink(&secret, &link).unwrap();
+
+    OsFileSink.write(&link, b"new").await.unwrap();
+
+    assert_eq!(std::fs::read_to_string(&secret).unwrap(), "secret");
+    assert!(!std::fs::symlink_metadata(&link).unwrap().is_symlink());
+    assert_eq!(std::fs::read_to_string(&link).unwrap(), "new");
+}
+
+// The rename installs a new inode, so the target's mode bits are copied to
+// it first: an executable script stays executable after an edit.
+#[cfg(unix)]
+#[tokio::test]
+async fn a_write_keeps_the_targets_permissions() {
+    use std::os::unix::fs::PermissionsExt;
+    let dir = tempfile::tempdir().unwrap();
+    let script = dir.path().join("run.sh");
+    std::fs::write(&script, "old").unwrap();
+    std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o750)).unwrap();
+
+    OsFileSink.write(&script, b"new").await.unwrap();
+
+    let mode = std::fs::metadata(&script).unwrap().permissions().mode() & 0o777;
+    assert_eq!(mode, 0o750);
+    assert_eq!(std::fs::read_to_string(&script).unwrap(), "new");
+    let leftovers = std::fs::read_dir(dir.path()).unwrap().count();
+    assert_eq!(leftovers, 1, "a temp file was left behind");
+}
+
+#[tokio::test]
+async fn failed_writes_report_why_and_leave_no_temp_file() {
+    let no_parent = atomic_write(Path::new("/"), b"x").await.unwrap_err();
+    assert_eq!(no_parent.kind(), std::io::ErrorKind::InvalidInput);
+
+    let dir = tempfile::tempdir().unwrap();
+    let missing_dir = atomic_write(&dir.path().join("nope").join("a.txt"), b"x")
+        .await
+        .unwrap_err();
+    assert!(missing_dir.to_string().contains("stage temp file"));
+
+    // Renaming a file over a directory fails after the temp file was staged.
+    let target = dir.path().join("a.txt");
+    std::fs::create_dir(&target).unwrap();
+    assert!(atomic_write(&target, b"x").await.is_err());
+    assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1);
+}

crates/tinytools-std/src/filesystem/file_write/mod.rs

@@ -194,6 +194,10 @@ impl FileWriteTool {
             ));
         }
 
+        // Per-path lock, as edit_file and apply_patch take: the stale check and
+        // the write must not interleave with another tool's edit of the file.
+        let _path_guard = file_state::acquire_path_lock(&resolved_target).await;
+
         // File-state guard: reject writes based on stale or partial reads.
         if let Some(agent_id) = file_state::current_file_state_agent_id() {
             if let Some(msg) = file_state::check_stale_read(&agent_id, &resolved_target) {

crates/tinytools-std/src/filesystem/update_memory_md/mod.rs

@@ -4,7 +4,6 @@ use async_trait::async_trait;
 use serde_json::json;
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
-use std::sync::atomic::{AtomicU64, Ordering};
 use std::sync::{Arc, LazyLock, Mutex};
 use tinytools::ToolRunContext;
 use tinytools::{PermissionLevel, Tool, ToolCallOptions, ToolResult};
@@ -25,9 +24,6 @@ const ALLOWED_FILES: &[&str] = &["MEMORY.md", "SKILL.md"];
 static WORKSPACE_WRITE_LOCKS: LazyLock<Mutex<HashMap<PathBuf, Arc<tokio::sync::Mutex<()>>>>> =
     LazyLock::new(|| Mutex::new(HashMap::new()));
 
-/// Monotonic counter for temp-file uniqueness within a process.
-static TEMP_FILE_SEQ: AtomicU64 = AtomicU64::new(0);
-
 /// Return (creating if needed) the shared async write lock for `workspace_dir`.
 ///
 /// The lock key is the canonicalized workspace path when it resolves (so two
@@ -93,46 +89,13 @@ async fn acquire_cross_process_write_lock(workspace_dir: &Path) -> anyhow::Resul
     .map_err(|e| anyhow::anyhow!("workspace lock task join failed: {e}"))?
 }
 
-/// Atomically replace `path`'s contents with `content`.
-///
-/// Writes to a sibling temp file in the same directory (so the rename stays on
-/// one filesystem and is atomic) and `rename`s it over the target. A crash
-/// mid-write leaves either the old file or the complete new file — never a
-/// half-written truncation. Callers MUST hold the per-workspace write lock so
-/// the read-modify-write is serialized end-to-end.
+/// [`super::file_sink::atomic_write`], with the error naming `file`.
+/// Callers MUST hold the per-workspace write lock so the read-modify-write is
+/// serialized end-to-end.
 async fn atomic_write(path: &Path, file: &str, content: &str) -> anyhow::Result<()> {
-    let dir = path
-        .parent()
-        .ok_or_else(|| anyhow::anyhow!("target path has no parent directory"))?;
-    let seq = TEMP_FILE_SEQ.fetch_add(1, Ordering::Relaxed);
-    let tmp_name = format!(".{file}.{}.{seq}.tmp", std::process::id());
-    let tmp_path = dir.join(tmp_name);
-
-    tracing::debug!(
-        tmp = %tmp_path.display(),
-        target = %path.display(),
-        bytes = content.len(),
-        "[update_memory_md] atomic write: staging temp file"
-    );
-
-    if let Err(e) = tokio::fs::write(&tmp_path, content).await {
-        // Clean up a partially-written temp file so a failed stage doesn't
-        // litter the workspace (CodeRabbit: temp not cleaned on initial write).
-        let _ = tokio::fs::remove_file(&tmp_path).await;
-        return Err(anyhow::anyhow!("Failed to stage temp file for {file}: {e}"));
-    }
-
-    if let Err(e) = tokio::fs::rename(&tmp_path, path).await {
-        // Best-effort cleanup so a failed rename doesn't litter the workspace.
-        let _ = tokio::fs::remove_file(&tmp_path).await;
-        return Err(anyhow::anyhow!("Failed to atomically write {file}: {e}"));
-    }
-
-    tracing::debug!(
-        target = %path.display(),
-        "[update_memory_md] atomic write: rename committed"
-    );
-    Ok(())
+    super::file_sink::atomic_write(path, content.as_bytes())
+        .await
+        .map_err(|e| anyhow::anyhow!("Failed to atomically write {file}: {e}"))
 }
 
 /// Appends or replaces a named section in MEMORY.md or SKILL.md.

crates/tinytools-std/src/filesystem/update_memory_md/mod_tests.rs

@@ -265,7 +265,7 @@ async fn atomic_write_reports_staging_and_rename_failures() {
     let err = atomic_write(&missing_dir_target, "MEMORY.md", "x")
         .await
         .unwrap_err();
-    assert!(err.to_string().contains("Failed to stage temp file"));
+    assert!(err.to_string().contains("stage temp file"), "{err}");
 
     // Renaming a file over a directory fails after the temp file was staged.
     let dir_target = dir.path().join("MEMORY.md");