mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-07 17:09:13 -10:00
fix(tools): every whole-file writer checks the read is fresh
92f900edf0guarded 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 in864d0db02a(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:
1 parent
864d0db02a
commit
74214911aa
6 files changed
+41
-1
No files matched your search
@@ -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,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
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in new issue
Block a user