From 266db5eadcac4931cc381a025f2812dddbddf9a9 Mon Sep 17 00:00:00 2001 From: Alfredo Montesinos <22755327+ForkedInTime@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:56:09 -0700 Subject: [PATCH 1/2] fix(fs): atomic writes, real MultiEdit atomicity, bounded glob results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes Phase 2. The security half shipped in #16; this is the data-integrity half — the four items that PR deliberately left open. ## MultiEdit was not atomic, despite saying it was Its doc comment promised edits were applied "atomically", but it wrote inside the loop — one `fs::write` per edit — and `continue`d past failures. A batch where edit 3 of 5 failed therefore left edits 1, 2, 4 and 5 on disk: a half-finished refactor, on disk, reported as a single "✗" among several "✓". For the usual case (rename a symbol in five places) that is a broken file the model may not notice is broken. Edits are now staged in memory and committed per file, all-or-nothing. Granularity is per file rather than per batch, so a failure in file A does not discard good edits to file B. Note the subtlety this exposed: edits used to compose only because each write landed immediately and the next edit re-read the file — the very thing that made it non-atomic. Staging had to carry that forward explicitly, so later edits read the staged content rather than the disk. Covered by a test. ## No atomic writes anywhere `fs::write` truncates before writing, so a crash, a full disk, or a kill between truncate and write left the user's file empty or partial — silently, with the original gone. `session/` was given temp+rename in an earlier audit; the file tools never were. `atomic_write` writes to a temp file in the *same directory* (so the rename is same-filesystem and therefore atomic — `/tmp` may be a different mount, where rename degrades to copy-then-delete), fsyncs before renaming so a crash cannot produce a present-but-empty file, and removes the temp file on any failure. **It carries the original file's mode across.** Renaming replaces the inode, so without that an edit to a `0600` file would silently republish it at `0644` — turning a routine edit into a disclosure. Verified by test. ## glob results were unbounded A broad pattern over a large tree built an unbounded Vec and joined it into one enormous string sent to the model as a tool result — cost and memory scaling with the repository, with nothing downstream capping it. Now capped at 1000, applied *after* the mtime sort so the most recently modified matches survive, with the truncation stated rather than silent. ## Encoding: checked, no defect CRLF and BOM both round-trip byte-identically through an edit. Non-UTF-8 files are refused with the bytes left untouched — no corruption. Tracker item closed rather than "fixed"; there was nothing to fix. The refusal surfaces as a hard `Err` rather than a tool-level error result, which is a consistency wrinkle shared with other tools, not a data-integrity problem — noted, not changed here. QA (triple-checked): 601 tests, 0 failures. Clippy clean under the CI gate. Release 19.08 MB. Zero panics in production code across all six Phase 2 files plus tools/mod.rs. The security tests from #16 re-run green. Each fix verified by reverting it: writing inside the loop fails the atomicity test, dropping mode preservation fails the permissions test, removing the cap fails the bounds test. Co-Authored-By: Arch Linux --- src/tools/file_edit.rs | 4 +- src/tools/file_write.rs | 2 +- src/tools/glob.rs | 25 +++- src/tools/mod.rs | 62 +++++++++ src/tools/multi_edit.rs | 79 ++++++++--- tests/fs_tool_integrity_tests.rs | 218 +++++++++++++++++++++++++++++++ 6 files changed, 365 insertions(+), 25 deletions(-) create mode 100644 tests/fs_tool_integrity_tests.rs diff --git a/src/tools/file_edit.rs b/src/tools/file_edit.rs index 9d530bf..d74c9cd 100644 --- a/src/tools/file_edit.rs +++ b/src/tools/file_edit.rs @@ -93,7 +93,7 @@ impl Tool for FileEditTool { path.display() ))); } - fs::write(&path, &new_content).await?; + super::atomic_write(&path, &new_content).await?; return Ok(ToolOutput::success(format!( "Replaced {count} occurrence(s) in {}", path.display() @@ -109,7 +109,7 @@ impl Tool for FileEditTool { ))), 1 => { let new_content = content.replacen(&input.old_string, &input.new_string, 1); - fs::write(&path, &new_content).await?; + super::atomic_write(&path, &new_content).await?; Ok(ToolOutput::success(format!( "Edit applied successfully to {}", path.display() diff --git a/src/tools/file_write.rs b/src/tools/file_write.rs index d846414..3307f9e 100644 --- a/src/tools/file_write.rs +++ b/src/tools/file_write.rs @@ -67,7 +67,7 @@ impl Tool for FileWriteTool { })?; } - fs::write(&path, &input.content) + super::atomic_write(&path, &input.content) .await .map_err(|e| anyhow::anyhow!("Failed to write {}: {}", path.display(), e))?; diff --git a/src/tools/glob.rs b/src/tools/glob.rs index d826c3a..39d4162 100644 --- a/src/tools/glob.rs +++ b/src/tools/glob.rs @@ -9,6 +9,11 @@ use serde_json::json; use std::path::Path; use std::time::SystemTime; +/// Cap on returned matches. Generous enough that ordinary searches are never +/// truncated, low enough that a repo-wide `**/*` cannot produce a multi-megabyte +/// tool result. +const MAX_GLOB_RESULTS: usize = 1000; + pub struct GlobTool; #[derive(Deserialize)] @@ -100,16 +105,34 @@ impl Tool for GlobTool { // Sort by modification time, most recent first entries.sort_by_key(|e| std::cmp::Reverse(e.0)); + // Bound the result. A broad pattern over a large tree (`**/*`) otherwise + // builds an unbounded Vec and then joins it into one enormous string + // that is sent to the model as a tool result — cost and memory scale + // with the repository, and nothing downstream caps it. Sorting first + // means the cap keeps the most recently modified matches, which is what + // the ordering exists to surface. + let total = entries.len(); + let truncated = total > MAX_GLOB_RESULTS; + entries.truncate(MAX_GLOB_RESULTS); + if entries.is_empty() { return Ok(ToolOutput::success("No files matched the pattern.")); } - let output = entries + let mut output = entries .into_iter() .map(|(_, path)| path) .collect::>() .join("\n"); + if truncated { + output.push_str(&format!( + "\n\n... {} of {total} matches shown (most recently modified first). \ + Narrow the pattern or search a subdirectory to see the rest.", + MAX_GLOB_RESULTS + )); + } + Ok(ToolOutput::success(output)) } } diff --git a/src/tools/mod.rs b/src/tools/mod.rs index fb0ddfc..210a0f7 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -268,6 +268,68 @@ fn is_dotenv(file_name: &str) -> bool { /// Returns Some(error ToolOutput) if the path should be blocked for `op`. /// Read mode blocks private keys only. Write mode additionally blocks /// dotenv files, credential stores, and secrets directories. +/// Write a file atomically: temp file in the same directory, fsync, rename. +/// +/// A direct `fs::write` truncates the target first, so a crash, a full disk, or +/// a kill between truncate and write leaves the user's file empty or partial — +/// silently, and with the original gone. `session/` was given this treatment in +/// an earlier audit; the file tools were not. +/// +/// The temp file is created in the *same directory* so the rename is a +/// same-filesystem operation and therefore atomic; `/tmp` may be a different +/// mount, where rename degrades to copy-then-delete and loses the guarantee. +/// +/// **Permissions are carried over from the original.** Renaming replaces the +/// inode, so without this an edit to a `0600` file would silently republish it +/// at the default `0644` — turning a routine edit into a disclosure. +pub async fn atomic_write(path: &std::path::Path, content: &str) -> std::io::Result<()> { + use std::sync::atomic::{AtomicU64, Ordering}; + static SEQ: AtomicU64 = AtomicU64::new(0); + + let parent = path.parent().unwrap_or_else(|| std::path::Path::new(".")); + let stem = path.file_name().and_then(|n| n.to_str()).unwrap_or("file"); + let tmp = parent.join(format!( + ".{stem}.rustyclaw-{}-{}.tmp", + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )); + + // Mode of the file we are replacing, if it exists. + #[cfg(unix)] + let mode = { + use std::os::unix::fs::PermissionsExt; + tokio::fs::metadata(path) + .await + .ok() + .map(|m| m.permissions().mode()) + }; + + let write_result = async { + let mut f = tokio::fs::File::create(&tmp).await?; + tokio::io::AsyncWriteExt::write_all(&mut f, content.as_bytes()).await?; + // Durability: without this the rename can land before the data does, so + // a crash yields a present-but-empty file — the exact outcome this is + // meant to prevent. + f.sync_all().await?; + drop(f); + + #[cfg(unix)] + if let Some(mode) = mode { + use std::os::unix::fs::PermissionsExt; + tokio::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(mode)).await?; + } + + tokio::fs::rename(&tmp, path).await + } + .await; + + if write_result.is_err() { + // Never leave a stray temp file behind on failure. + let _ = tokio::fs::remove_file(&tmp).await; + } + write_result +} + /// Deny-listed read paths expressed as ripgrep exclusion globs. /// /// The Grep tool has two backends — a `ripgrep` subprocess and a pure-Rust diff --git a/src/tools/multi_edit.rs b/src/tools/multi_edit.rs index 1c94a54..f13c8c8 100644 --- a/src/tools/multi_edit.rs +++ b/src/tools/multi_edit.rs @@ -91,6 +91,13 @@ impl Tool for MultiEditTool { let mut results: Vec = Vec::new(); let mut had_error = false; + // path -> fully-edited content, written only if every edit to that path + // succeeded. + let mut staged: std::collections::BTreeMap = + std::collections::BTreeMap::new(); + let mut failed_files: std::collections::BTreeSet = + std::collections::BTreeSet::new(); + for (i, edit) in input.edits.iter().enumerate() { let path = match resolve_path(&edit.file_path, &ctx.cwd) { Ok(p) => p, @@ -115,6 +122,7 @@ impl Tool for MultiEditTool { .join(" "); results.push(format!("{label} ✗ {msg}")); had_error = true; + failed_files.insert(path.clone()); continue; } if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Write) { @@ -138,16 +146,24 @@ impl Tool for MultiEditTool { if !path.exists() { results.push(format!("{} ✗ File not found", label)); had_error = true; + failed_files.insert(path.clone()); continue; } - let content = match fs::read_to_string(&path).await { - Ok(c) => c, - Err(e) => { - results.push(format!("{} ✗ Read error: {}", label, e)); - had_error = true; - continue; - } + // Later edits must see earlier ones. Previously each edit re-read + // the file, which worked only because each write landed + // immediately — the very thing that made this non-atomic. + let content = match staged.get(&path) { + Some(c) => c.clone(), + None => match fs::read_to_string(&path).await { + Ok(c) => c, + Err(e) => { + results.push(format!("{} ✗ Read error: {}", label, e)); + had_error = true; + failed_files.insert(path.clone()); + continue; + } + }, }; if edit.replace_all { @@ -155,32 +171,24 @@ impl Tool for MultiEditTool { if count == 0 { results.push(format!("{} ✗ old_string not found", label)); had_error = true; + failed_files.insert(path.clone()); continue; } let new_content = content.replace(&edit.old_string, &edit.new_string); - match fs::write(&path, &new_content).await { - Ok(_) => results.push(format!("{} ✓ Replaced {} occurrence(s)", label, count)), - Err(e) => { - results.push(format!("{} ✗ Write error: {}", label, e)); - had_error = true; - } - } + staged.insert(path.clone(), new_content); + results.push(format!("{} ✓ Replaced {} occurrence(s)", label, count)); } else { let count = content.matches(&edit.old_string as &str).count(); match count { 0 => { results.push(format!("{} ✗ old_string not found", label)); had_error = true; + failed_files.insert(path.clone()); } 1 => { let new_content = content.replacen(&edit.old_string, &edit.new_string, 1); - match fs::write(&path, &new_content).await { - Ok(_) => results.push(format!("{} ✓ Edit applied", label)), - Err(e) => { - results.push(format!("{} ✗ Write error: {}", label, e)); - had_error = true; - } - } + staged.insert(path.clone(), new_content); + results.push(format!("{} ✓ Edit applied", label)); } n => { results.push(format!( @@ -188,11 +196,40 @@ impl Tool for MultiEditTool { label, n )); had_error = true; + failed_files.insert(path.clone()); } } } } + // Commit phase — per-file all-or-nothing. + // + // Writing inside the loop meant a batch where edit 3 of 5 failed left + // the file with edits 1, 2, 4 and 5 applied: a half-finished refactor, + // on disk, reported only as one "✗" among several "✓". The doc comment + // promised these were applied atomically; now they are. + // + // Granularity is per file, not per batch: a failure in file A should not + // discard good edits to file B. + let mut committed = 0usize; + for (path, content) in &staged { + if failed_files.contains(path) { + results.push(format!( + " ↩ {} — no changes written; another edit to this file failed", + path.display() + )); + continue; + } + match super::atomic_write(path, content).await { + Ok(_) => committed += 1, + Err(e) => { + results.push(format!(" ✗ {} write error: {e}", path.display())); + had_error = true; + } + } + } + let _ = committed; + let summary = results.join("\n"); if had_error { Ok(ToolOutput::error(summary)) diff --git a/tests/fs_tool_integrity_tests.rs b/tests/fs_tool_integrity_tests.rs new file mode 100644 index 0000000..907d1d0 --- /dev/null +++ b/tests/fs_tool_integrity_tests.rs @@ -0,0 +1,218 @@ +//! Phase 2, data-integrity half: writes must not corrupt, MultiEdit must be +//! atomic as documented, and result sets must be bounded. + +use rustyclaw::tools::{ + Tool, ToolContext, file_edit::FileEditTool, file_write::FileWriteTool, glob::GlobTool, + multi_edit::MultiEditTool, +}; +use serde_json::json; +use std::path::PathBuf; +use tempfile::TempDir; + +fn text(o: &rustyclaw::tools::ToolOutput) -> String { + o.content + .iter() + .map(|c| match c { + rustyclaw::api::types::ToolResultContent::Text { text } => text.as_str(), + }) + .collect::>() + .join("") +} + +/// MultiEdit documented its edits as atomic but wrote inside the loop, so a +/// batch where one edit failed left the others on disk — a half-finished +/// refactor, reported as a single "✗" among several "✓". +#[tokio::test] +async fn multi_edit_writes_nothing_to_a_file_when_any_edit_to_it_fails() { + let td = TempDir::new().unwrap(); + let ctx = ToolContext::new(PathBuf::from(td.path())); + let original = "one\ntwo\nthree\n"; + std::fs::write(td.path().join("a.txt"), original).unwrap(); + + let out = MultiEditTool + .execute( + json!({"edits": [ + {"file_path": "a.txt", "old_string": "one", "new_string": "ONE"}, + {"file_path": "a.txt", "old_string": "MISSING", "new_string": "X"}, + {"file_path": "a.txt", "old_string": "three", "new_string": "THREE"} + ]}), + &ctx, + ) + .await + .unwrap(); + + assert!(out.is_error, "a failed edit must surface as an error"); + assert_eq!( + std::fs::read_to_string(td.path().join("a.txt")).unwrap(), + original, + "file must be byte-identical when any edit to it failed" + ); +} + +/// A failure in one file must not discard good edits to another — the +/// granularity is per file, not per batch. +#[tokio::test] +async fn multi_edit_failure_in_one_file_does_not_block_another() { + let td = TempDir::new().unwrap(); + let ctx = ToolContext::new(PathBuf::from(td.path())); + std::fs::write(td.path().join("good.txt"), "alpha\n").unwrap(); + std::fs::write(td.path().join("bad.txt"), "beta\n").unwrap(); + + let out = MultiEditTool + .execute( + json!({"edits": [ + {"file_path": "good.txt", "old_string": "alpha", "new_string": "ALPHA"}, + {"file_path": "bad.txt", "old_string": "MISSING", "new_string": "X"} + ]}), + &ctx, + ) + .await + .unwrap(); + + assert!(out.is_error, "got: {}", text(&out)); + assert_eq!( + std::fs::read_to_string(td.path().join("good.txt")).unwrap(), + "ALPHA\n", + "the succeeding file must still be written" + ); + assert_eq!( + std::fs::read_to_string(td.path().join("bad.txt")).unwrap(), + "beta\n", + "the failing file must be untouched" + ); +} + +/// Sequential edits to one file must compose — staging must not lose earlier +/// edits now that writes happen once at the end. +#[tokio::test] +async fn multi_edit_applies_all_edits_when_all_succeed() { + let td = TempDir::new().unwrap(); + let ctx = ToolContext::new(PathBuf::from(td.path())); + std::fs::write(td.path().join("a.txt"), "one\ntwo\nthree\n").unwrap(); + + let out = MultiEditTool + .execute( + json!({"edits": [ + {"file_path": "a.txt", "old_string": "one", "new_string": "ONE"}, + {"file_path": "a.txt", "old_string": "two", "new_string": "TWO"}, + {"file_path": "a.txt", "old_string": "three", "new_string": "THREE"} + ]}), + &ctx, + ) + .await + .unwrap(); + + assert!(!out.is_error, "got: {}", text(&out)); + assert_eq!( + std::fs::read_to_string(td.path().join("a.txt")).unwrap(), + "ONE\nTWO\nTHREE\n", + "every edit must be present" + ); +} + +/// An edit chained onto a previous edit's output must see it. +#[tokio::test] +async fn multi_edit_later_edits_see_earlier_ones() { + let td = TempDir::new().unwrap(); + let ctx = ToolContext::new(PathBuf::from(td.path())); + std::fs::write(td.path().join("a.txt"), "aaa\n").unwrap(); + + MultiEditTool + .execute( + json!({"edits": [ + {"file_path": "a.txt", "old_string": "aaa", "new_string": "bbb"}, + {"file_path": "a.txt", "old_string": "bbb", "new_string": "ccc"} + ]}), + &ctx, + ) + .await + .unwrap(); + + assert_eq!( + std::fs::read_to_string(td.path().join("a.txt")).unwrap(), + "ccc\n", + "the second edit must operate on the first edit's result" + ); +} + +/// Renaming replaces the inode, so an atomic write must carry the original +/// mode across — otherwise editing a 0600 file silently republishes it at 0644. +#[cfg(unix)] +#[tokio::test] +async fn atomic_write_preserves_file_permissions() { + use std::os::unix::fs::PermissionsExt; + let td = TempDir::new().unwrap(); + let ctx = ToolContext::new(PathBuf::from(td.path())); + let p = td.path().join("secret.conf"); + std::fs::write(&p, "token=abc\n").unwrap(); + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o600)).unwrap(); + + FileEditTool + .execute( + json!({"file_path": "secret.conf", "old_string": "abc", "new_string": "xyz"}), + &ctx, + ) + .await + .unwrap(); + + let mode = std::fs::metadata(&p).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "permissions must survive the rename, got {mode:o}"); + assert_eq!(std::fs::read_to_string(&p).unwrap(), "token=xyz\n"); +} + +/// The temp file used for the atomic rename must never be left behind. +#[tokio::test] +async fn atomic_write_leaves_no_temp_files() { + let td = TempDir::new().unwrap(); + let ctx = ToolContext::new(PathBuf::from(td.path())); + + FileWriteTool + .execute(json!({"file_path": "out.txt", "content": "hello\n"}), &ctx) + .await + .unwrap(); + + let strays: Vec<_> = std::fs::read_dir(td.path()) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.contains("rustyclaw-") || n.ends_with(".tmp")) + .collect(); + assert!(strays.is_empty(), "temp files left behind: {strays:?}"); + assert_eq!(std::fs::read_to_string(td.path().join("out.txt")).unwrap(), "hello\n"); +} + +/// A broad pattern over a large tree must not build an unbounded result. +#[tokio::test] +async fn glob_results_are_bounded() { + let td = TempDir::new().unwrap(); + for i in 0..1500 { + std::fs::write(td.path().join(format!("f{i:05}.txt")), "x").unwrap(); + } + let ctx = ToolContext::new(PathBuf::from(td.path())); + let out = GlobTool + .execute(json!({"pattern": "**/*.txt"}), &ctx) + .await + .unwrap(); + let body = text(&out); + + let listed = body.lines().filter(|l| l.ends_with(".txt")).count(); + assert!(listed <= 1000, "expected a cap, got {listed} entries"); + assert!( + body.contains("of 1500 matches shown"), + "truncation must be stated, not silent: {}", + &body[body.len().saturating_sub(200)..] + ); +} + +/// Ordinary searches must not be truncated or gain a spurious notice. +#[tokio::test] +async fn glob_small_result_sets_are_untouched() { + let td = TempDir::new().unwrap(); + for i in 0..5 { + std::fs::write(td.path().join(format!("f{i}.txt")), "x").unwrap(); + } + let ctx = ToolContext::new(PathBuf::from(td.path())); + let body = text(&GlobTool.execute(json!({"pattern": "**/*.txt"}), &ctx).await.unwrap()); + assert_eq!(body.lines().filter(|l| l.ends_with(".txt")).count(), 5); + assert!(!body.contains("matches shown"), "no notice expected: {body}"); +} From ad655dc69faf708c41bd88972f4aca1aa1876190 Mon Sep 17 00:00:00 2001 From: Alfredo Montesinos <22755327+ForkedInTime@users.noreply.github.com> Date: Mon, 3 Aug 2026 01:04:48 -0700 Subject: [PATCH 2/2] test(fs): scope a unix-only import into its test Windows CI. `FileEditTool` is used solely by the `#[cfg(unix)]` permissions test, so a module-level import is dead code on Windows and trips `-D warnings`. Third time a Windows-only lint has caught something the Linux run could not. The pattern is now explicit: an import used only by a cfg-gated test has to be gated with it. Co-Authored-By: Arch Linux --- tests/fs_tool_integrity_tests.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/fs_tool_integrity_tests.rs b/tests/fs_tool_integrity_tests.rs index 907d1d0..ac2a70a 100644 --- a/tests/fs_tool_integrity_tests.rs +++ b/tests/fs_tool_integrity_tests.rs @@ -2,8 +2,7 @@ //! atomic as documented, and result sets must be bounded. use rustyclaw::tools::{ - Tool, ToolContext, file_edit::FileEditTool, file_write::FileWriteTool, glob::GlobTool, - multi_edit::MultiEditTool, + Tool, ToolContext, file_write::FileWriteTool, glob::GlobTool, multi_edit::MultiEditTool, }; use serde_json::json; use std::path::PathBuf; @@ -140,6 +139,10 @@ async fn multi_edit_later_edits_see_earlier_ones() { #[cfg(unix)] #[tokio::test] async fn atomic_write_preserves_file_permissions() { + // Imported here rather than at module scope: this is the only user, and the + // test is unix-only, so a top-level import is dead code on Windows and + // trips `-D warnings` there. + use rustyclaw::tools::file_edit::FileEditTool; use std::os::unix::fs::PermissionsExt; let td = TempDir::new().unwrap(); let ctx = ToolContext::new(PathBuf::from(td.path()));