diff --git a/pr-body-1650.md b/pr-body-1650.md new file mode 100644 index 000000000..99c696165 --- /dev/null +++ b/pr-body-1650.md @@ -0,0 +1,27 @@ +## Summary + +Fixes #1650 + +The Edit tool caused extreme token amplification (957K input tokens) and a 1m23s hang on a 1185-line file. Two root causes: + +### Fix 1: Remove redundant dry-run from `validate_input` + +`validate_input()` in `file_edit_tool.rs` ran a full dry-run `apply_edit_to_content()` — reading the file and generating all whitespace-normalization candidates — and then `call_impl` did the exact same work again when actually applying the edit. This doubled the file reads and candidate generation on every edit. + +**Fix:** Removed the dry-run block from `validate_input`. The edit is already validated during `call_impl` via `apply_edit_to_content`, so the dry-run was purely redundant work. + +### Fix 2: Add fast path before candidate generation in `apply_edit_to_content` + +`edit_string_candidates()` generates whitespace-normalization candidates (tabs↔spaces at width 2 and 4), each calling `find_actual_string()` which does O(n*m) char-by-char scanning. ALL candidates were generated upfront before any matching, even when the exact `old_string` already matched the file content. + +**Fix:** Added a fast path that tries the exact `old_string`/`new_string` match via `apply_match_and_replace()` before calling `edit_string_candidates()`. If the exact match succeeds (the common case), it returns immediately — skipping all candidate generation and expensive scanning. If the exact match fails with "not found", it falls through to the existing candidate loop (slow path unchanged for edge cases). + +## Validation + +- `cargo check -p tool-runtime -p bitfun-core` — passed +- `cargo test -p tool-runtime -- fs::edit_file` — 21/21 passed +- `cargo test -p bitfun-core -- file_edit_tool` — 3/3 passed + +## Impact + +For the reported 1185-line file edit, the common case (exact match) now completes in a single `apply_match_and_replace` call instead of generating and scanning multiple whitespace-normalization candidates through `find_actual_string`. Combined with removing the redundant dry-run, this eliminates the token amplification and hang. diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs index 42eab8281..2c643e7f2 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs @@ -1,7 +1,7 @@ use crate::agentic::tools::file_permissions::file_permission_intents_allowing_managed_plan_edits; use crate::agentic::tools::file_read_state_runtime::{ assert_file_not_unexpectedly_modified, file_mutation_timestamp_ms, get_stored_file_read_state, - local_file_modification_time_ms, read_current_file_content, read_state_tracking_enabled, + local_file_modification_time_ms, read_state_tracking_enabled, update_file_read_state_after_mutation, validate_edit_against_read_state, validate_edit_has_prior_read, FILE_UNEXPECTEDLY_MODIFIED_ERROR, }; @@ -223,7 +223,7 @@ impl Tool for FileEditTool { .get("new_string") .and_then(|v| v.as_str()) .unwrap_or(""); - let replace_all = input + let _replace_all = input .get("replace_all") .and_then(|v| v.as_bool()) .unwrap_or(false); @@ -270,36 +270,6 @@ impl Tool for FileEditTool { if let Some(message) = Self::edit_read_state_guardrail_error(ctx, &resolved).await { return Self::guidance_failure(message); } - - let file_content = match read_current_file_content(ctx, &resolved).await { - Ok(content) => content, - Err(error) => { - return ValidationResult { - result: false, - message: Some(format!( - "Failed to read file {}: {}", - resolved.logical_path, error - )), - error_code: Some(400), - meta: None, - }; - } - }; - - if let Err(error) = - apply_edit_to_content(&file_content, old_string, new_string, replace_all) - { - if is_edit_content_guardrail_error(&error) { - return Self::guidance_failure(error); - } - - return ValidationResult { - result: false, - message: Some(error), - error_code: Some(400), - meta: None, - }; - } } ValidationResult::default() diff --git a/src/crates/execution/tool-execution/src/fs/edit_file.rs b/src/crates/execution/tool-execution/src/fs/edit_file.rs index 0a3bc5a5d..7d4722d4b 100644 --- a/src/crates/execution/tool-execution/src/fs/edit_file.rs +++ b/src/crates/execution/tool-execution/src/fs/edit_file.rs @@ -429,6 +429,22 @@ pub fn apply_edit_to_content( let uses_crlf = content.contains("\r\n"); let normalized_content = normalize_string(content); + // Fast path: try the exact old_string before generating fallback + // candidates. This avoids expensive whitespace-normalization and + // find_actual_string character-by-character scanning when the exact + // match succeeds — the common case. + match apply_match_and_replace( + &normalized_content, + uses_crlf, + old_string, + new_string, + replace_all, + ) { + Ok(result) => return Ok(result), + Err(error) if error == "old_string not found in file." => last_error = error, + Err(error) => return Err(error), + } + for (candidate_old, candidate_new) in edit_string_candidates(content, old_string, new_string) { match apply_match_and_replace( &normalized_content,