fix(tools): clarify edit_file replacement boundaries (#1516)
This commit is contained in:
@@ -170,10 +170,10 @@ Multiple `tool_calls` in one turn run in parallel. `web_search` returns `ref_id`
|
||||
## Tool Selection Guide
|
||||
|
||||
### `apply_patch`
|
||||
Use `apply_patch` for structural edits, coordinated changes, or cases where line context matters. Use `write_file` for brand-new files or full-file rewrites. Use `edit_file` for a single unambiguous replacement.
|
||||
Use `apply_patch` for structural edits, coordinated changes, or cases where line context matters. Use `write_file` for brand-new files, full-file rewrites, or large existing-file changes where several intertwined edits make local replacement fragile. Use `edit_file` for a single unambiguous replacement.
|
||||
|
||||
### `edit_file`
|
||||
Use `edit_file` for one clear replacement in one file. Use `apply_patch` when the edit changes whole blocks, touches multiple files, or needs surrounding line context.
|
||||
Use `edit_file` for one clear replacement in one file. Do not use it for multi-block deletions, cross-cutting refactors, or changes that touch more than one logical unit; use `apply_patch` or `write_file` for those.
|
||||
|
||||
### `exec_shell`
|
||||
Use `exec_shell` for shell-native diagnostics, pipelines, and bounded commands. Use structured tools for structured operations when they map directly (`grep_files`, `git_diff`, `read_file`). For long commands, servers, full test suites, or release computations, start background work with `task_shell_start` or `exec_shell` using `background: true`, then poll with `task_shell_wait` or `exec_shell_wait`.
|
||||
|
||||
@@ -473,7 +473,7 @@ impl ToolSpec for EditFileTool {
|
||||
}
|
||||
|
||||
fn description(&self) -> &'static str {
|
||||
"Replace text in a single file via exact search/replace. Use this instead of `sed -i` in `exec_shell` for in-place edits. For multi-hunk or cross-file changes, use `apply_patch` instead. Required: 'path', 'search' (exact text to find), 'replace' (text to substitute)."
|
||||
"Replace text in a single file via exact search/replace. Use this instead of `sed -i` in `exec_shell` for one unambiguous in-place edit. `search` must match exactly, including whitespace and indentation. Returns a compact unified diff, not the full file. For structural, multi-block, or cross-file changes, use `apply_patch` or `write_file` instead."
|
||||
}
|
||||
|
||||
fn input_schema(&self) -> Value {
|
||||
@@ -486,7 +486,7 @@ impl ToolSpec for EditFileTool {
|
||||
},
|
||||
"search": {
|
||||
"type": "string",
|
||||
"description": "Text to search for"
|
||||
"description": "Exact text to search for, including whitespace, indentation, and newlines"
|
||||
},
|
||||
"replace": {
|
||||
"type": "string",
|
||||
@@ -542,7 +542,15 @@ impl ToolSpec for EditFileTool {
|
||||
|
||||
let display = file_path.display().to_string();
|
||||
let diff = make_unified_diff(&display, &contents, &updated);
|
||||
let summary = format!("Replaced {count} occurrence(s) in {display}");
|
||||
let summary = if count > 1 {
|
||||
format!(
|
||||
"Replaced {count} occurrence(s) in {display}\n\
|
||||
Warning: multiple matches were replaced with the same substitution. \
|
||||
Verify the result with read_file before proceeding."
|
||||
)
|
||||
} else {
|
||||
format!("Replaced 1 occurrence in {display}")
|
||||
};
|
||||
let body = if diff.is_empty() {
|
||||
format!("{summary}\n(no textual changes)")
|
||||
} else {
|
||||
@@ -1023,7 +1031,7 @@ mod tests {
|
||||
// Two concerns in one test: with `prefer_external_pdftotext = true`
|
||||
// the dispatch must (a) call pdftotext when present, and (b) return
|
||||
// the structured `binary_unavailable` response when pdftotext is
|
||||
// missing — both branches were covered by the pre-v0.8.32 default.
|
||||
// missing.
|
||||
// Sync test (calls `read_pdf` directly, not the async ReadFileTool
|
||||
// wrapper) so the env-var lock is never held across an `.await`.
|
||||
let _lock = DS_CONFIG_PATH_LOCK.lock().unwrap();
|
||||
@@ -1151,6 +1159,11 @@ mod tests {
|
||||
|
||||
assert!(result.success);
|
||||
assert!(result.content.contains("2 occurrence(s)"));
|
||||
assert!(
|
||||
result.content.contains("multiple matches were replaced"),
|
||||
"{}",
|
||||
result.content
|
||||
);
|
||||
// Inline diff (#505) — the unified diff lands above the summary
|
||||
// line so the TUI's diff-aware renderer kicks in.
|
||||
assert!(result.content.contains("--- a/"), "{}", result.content);
|
||||
@@ -1170,6 +1183,28 @@ mod tests {
|
||||
assert_eq!(edited, "hi world hi");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_edit_file_single_match_has_no_multi_match_warning() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let ctx = ToolContext::new(tmp.path().to_path_buf());
|
||||
|
||||
let test_file = tmp.path().join("single.txt");
|
||||
fs::write(&test_file, "hello world").expect("write");
|
||||
|
||||
let tool = EditFileTool;
|
||||
let result = tool
|
||||
.execute(
|
||||
json!({"path": "single.txt", "search": "hello", "replace": "hi"}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.expect("execute");
|
||||
|
||||
assert!(result.success);
|
||||
assert!(result.content.contains("Replaced 1 occurrence"));
|
||||
assert!(!result.content.contains("multiple matches were replaced"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_edit_file_not_found() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
@@ -1320,6 +1355,8 @@ mod tests {
|
||||
assert!(!tool.is_read_only());
|
||||
assert!(tool.is_sandboxable());
|
||||
assert_eq!(tool.approval_requirement(), ApprovalRequirement::Suggest);
|
||||
assert!(tool.description().contains("exact search/replace"));
|
||||
assert!(tool.description().contains("structural"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1363,6 +1400,11 @@ mod tests {
|
||||
.and_then(|value| value.as_array())
|
||||
.expect("edit schema should include required array");
|
||||
assert_eq!(required.len(), 3);
|
||||
let search_desc = edit_schema["properties"]["search"]["description"]
|
||||
.as_str()
|
||||
.expect("search description");
|
||||
assert!(search_desc.contains("Exact text"));
|
||||
assert!(search_desc.contains("whitespace"));
|
||||
|
||||
let list_schema = ListDirTool.input_schema();
|
||||
let required = list_schema
|
||||
|
||||
Reference in New Issue
Block a user