fix(tools): every whole-file writer checks the read is fresh

92f900edf0 guarded the visible `write`. The 2026-10-17 safety hunt
(round 3) found three other whole-file overwrites without that guard,
so a refused `write` could be retried through any of them:

- apply_patch `replace` (build_pending_writes_from_replace) read the
  original only for rollback. It now calls
  require_fresh_read_before_write when the file exists, before reading
  it. Unified-diff patches are unchanged, since their context lines
  already fail against changed content. After either mode applies, each
  written path is recorded as read, so read, patch, write in one session
  is not refused as stale.
- The legacy WriteFileTool::execute (`write_file`, and `File
  action=write`), hidden from the catalog but still executable through
  exact_hidden_handler, now runs the same check unless the caller passed
  `expected_hash`, which is the stricter content-snapshot guard.
- fim_edit read the file, waited seconds on the FIM completion, then
  wrote its old prefix and suffix around the new middle. It now re-reads
  before writing and refuses if the content changed, records the read
  after a successful write, and resolves its path through
  resolve_guarded_read_path (it sent the raw file to the provider past
  the deny-list).

The workspace write carve-out still applies, so these run without a card
in a git workspace in Ask. They are now refused instead of silently
overwriting.

Five tests overwrote a pre-seeded file they had never read. Each now
records a read of the resolved path first, the way the read tool would:
test_apply_patch_replace_list, ..._rolls_back_on_write_failure,
test_apply_patch_legacy_changes_list,
write_file_tool_preserves_existing_mode and
write_file_over_crlf_file_preserves_crlf_line_endings.

Verification:
- scripts/dev-test.sh tui tools::file:: tools::apply_patch: 210 passed,
  0 failed.
- The wider run in 864d0db02a (4085 run) failed only on these five
  before the update, plus the three environmental test_runner tests.
- clippy with the CI flags: clean. fmt: clean.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01NBKbhx19BRbZtvWK8rVKjz
This commit is contained in:
CodeWhale BotandClaude Opus 5.5 committed 2026-10-17 17:22:59 -07:00
1 parent 864d0db02a
commit 74214911aa
6 files changed
+41 -1

No files matched your search

+7
View File
@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
- The other whole-file writers now refuse to replace an existing file that
is unread or stale, as `write` does: `apply_patch` with `replace`, and the
`write_file` and `File action=write` paths that older prompts still call.
A successful patch counts as a read, so patch-then-write still works.
`fim_edit` honors the read deny-list. It also refuses to write when the
file changed while its completion was running, which used to overwrite
your edit.
- `grep_files` (also `File search_content`) aimed at a single file, `rlm`
`open` with `file_path`, and the cache-maximal working set now honor the
read deny-list. Each read files such as `.env` past the check and returned
+7
View File
@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
- The other whole-file writers now refuse to replace an existing file that
is unread or stale, as `write` does: `apply_patch` with `replace`, and the
`write_file` and `File action=write` paths that older prompts still call.
A successful patch counts as a read, so patch-then-write still works.
`fim_edit` honors the read deny-list. It also refuses to write when the
file changed while its completion was running, which used to overwrite
your edit.
- `grep_files` (also `File search_content`) aimed at a single file, `rlm`
`open` with `file_path`, and the cache-maximal working set now honor the
read deny-list. Each read files such as `.env` past the check and returned
+12
View File
@@ -428,6 +428,9 @@ impl ToolSpec for ApplyPatchTool {
.await?;
normalize_pending_rust(&mut pending).await;
let pending = apply_pending_writes_blocking(pending).await?;
for entry in pending.iter().filter(|p| p.content.is_some()) {
context.note_file_read(&entry.path);
}
// Resolve absolute paths for LSP diagnostics query.
let abs_paths: Vec<PathBuf> = pending.iter().map(|p| p.path.clone()).collect();
let diag_block = lsp_diagnostics_for_paths(context, &abs_paths).await;
@@ -484,6 +487,9 @@ impl ToolSpec for ApplyPatchTool {
stats.header_path_mismatch = preflight.summary.header_path_mismatch.clone();
normalize_pending_rust(&mut pending).await;
let pending = apply_pending_writes_blocking(pending).await?;
for entry in pending.iter().filter(|p| p.content.is_some()) {
context.note_file_read(&entry.path);
}
// Resolve absolute paths for LSP diagnostics query.
let abs_paths: Vec<PathBuf> = pending
.iter()
@@ -1251,6 +1257,9 @@ fn build_pending_writes_from_replace(
.ok_or_else(|| ToolError::missing_field(format!("{source_field}[].content")))?;
let resolved = context.resolve_path(path)?;
if resolved.exists() {
context.require_fresh_read_before_write(&resolved, path)?;
}
let original = if resolved.exists() {
Some(read_file_content(&resolved)?)
} else {
@@ -2407,6 +2416,7 @@ diff --git a/same.txt b/same.txt
let ctx = ToolContext::new(tmp.path().to_path_buf());
fs::write(tmp.path().join("one.txt"), "old\n").expect("write");
ctx.note_file_read(&ctx.resolve_path("one.txt").expect("resolve"));
let tool = ApplyPatchTool;
let result = tool
@@ -2463,6 +2473,7 @@ diff --git a/same.txt b/same.txt
let tmp = tempdir().expect("tempdir");
let ctx = ToolContext::new(tmp.path().to_path_buf());
fs::write(tmp.path().join("legacy.txt"), "old\n").expect("write");
ctx.note_file_read(&ctx.resolve_path("legacy.txt").expect("resolve"));
let result = ApplyPatchTool
.execute(
@@ -2537,6 +2548,7 @@ diff --git a/same.txt b/same.txt
let ctx = ToolContext::new(tmp.path().to_path_buf());
fs::write(tmp.path().join("one.txt"), "old\n").expect("write");
ctx.note_file_read(&ctx.resolve_path("one.txt").expect("resolve"));
fs::write(tmp.path().join("blocked"), "not a dir\n").expect("write blocker");
let tool = ApplyPatchTool;
+2
View File
@@ -1856,6 +1856,8 @@ impl ToolSpec for WriteFileTool {
)));
}
verify_expected_hash(Some(expected), prior_contents.as_bytes(), "write", path_str)?;
} else if existed_before {
context.require_fresh_read_before_write(&file_path, path_str)?;
}
// Create parent directories if needed
+2
View File
@@ -771,6 +771,7 @@ async fn write_file_tool_preserves_existing_mode() {
let path = tmp.path().join("shared.txt");
fs::write(&path, b"before").expect("initial write");
fs::set_permissions(&path, fs::Permissions::from_mode(0o664)).expect("set shared permissions");
ctx.note_file_read(&ctx.resolve_path("shared.txt").expect("resolve"));
WriteFileTool
.execute(json!({"path": "shared.txt", "content": "after"}), &ctx)
@@ -788,6 +789,7 @@ async fn write_file_over_crlf_file_preserves_crlf_line_endings() {
let ctx = ToolContext::new(tmp.path().to_path_buf());
let path = tmp.path().join("crlf.txt");
fs::write(&path, b"alpha\r\nbeta\r\n").expect("initial CRLF write");
ctx.note_file_read(&ctx.resolve_path("crlf.txt").expect("resolve"));
WriteFileTool
.execute(
+11 -1
View File
@@ -114,7 +114,7 @@ impl ToolSpec for FimEditTool {
let max_tokens = optional_u64(&input, "max_tokens", 1024)?;
// 1. Read the file
let resolved = context.resolve_path(path)?;
let resolved = crate::tools::file::resolve_guarded_read_path(context, path, "fim_edit")?;
let content = tokio::fs::read_to_string(&resolved).await.map_err(|e| {
ToolError::execution_failed(format!("Failed to read {}: {}", resolved.display(), e))
})?;
@@ -161,6 +161,15 @@ impl ToolSpec for FimEditTool {
}
};
let current = tokio::fs::read_to_string(&resolved).await.map_err(|e| {
ToolError::execution_failed(format!("Failed to re-read {}: {}", resolved.display(), e))
})?;
if current != content {
return Err(ToolError::execution_failed(format!(
"Refusing fim_edit for {path}: the file changed while the FIM completion was running. Nothing was written. Recovery: read the file again and retry."
)));
}
// 7. Build the new content and write it back
let generated_len = generated_text.len();
let new_content = format!("{fim_prompt}{generated_text}{fim_suffix}");
@@ -172,6 +181,7 @@ impl ToolSpec for FimEditTool {
crate::utils::write_atomic_workspace(&resolved, new_content.as_bytes()).map_err(|e| {
ToolError::execution_failed(format!("Failed to write {}: {}", resolved.display(), e))
})?;
context.note_file_read(&resolved);
let result = FimEditResult {
success: true,