fix(tools): make apply_patch tolerant and re-expose edit_file

internal/agent/prompt_budget_test.go

@@ -68,7 +68,7 @@ func TestEagerToolSchemaTokenBudget(t *testing.T) {
 	}
 }
 
-func TestAgentAdvertisesPatchInsteadOfAmbiguousStringReplacement(t *testing.T) {
+func TestAgentAdvertisesBothEditTools(t *testing.T) {
 	registry := tools.NewRegistry()
 	for _, tool := range tools.CoreToolsScoped(t.TempDir(), nil) {
 		registry.Register(tool)
@@ -79,9 +79,9 @@ func TestAgentAdvertisesPatchInsteadOfAmbiguousStringReplacement(t *testing.T) {
 		names[definition.Name] = true
 	}
 	if !names["apply_patch"] {
-		t.Fatal("agent must retain apply_patch for existing-file changes")
+		t.Fatal("agent must retain apply_patch for multi-hunk changes")
 	}
-	if names["edit_file"] {
-		t.Fatal("agent must not receive the ambiguous string-replacement tool")
+	if !names["edit_file"] {
+		t.Fatal("agent must receive edit_file for targeted exact replacements")
 	}
 }

internal/agent/system_prompt.md

@@ -52,7 +52,7 @@ work.
 
 - Choose the narrowest tool that safely accomplishes the step. Prefer native
   file tools - read_file, read_minified_file, list_directory, glob, grep,
-  write_file, apply_patch - over shelling out to
+  edit_file, apply_patch, write_file - over shelling out to
   cat/sed/awk/python for file operations.
   They are safer, reviewable, and produce clean diffs.
 - Prefer read_minified_file when initially exploring source code; it preserves
@@ -61,11 +61,18 @@ work.
 - Keep edits focused and reviewable. A single patch may update several related
   files when they form one coherent change; do not hide unrelated edits in a
   bulk shell or script rewrite.
-- For edits to existing files, prefer apply_patch with minimal, targeted hunks
-  and enough unchanged context to identify the intended location. Match the
-  existing indentation, imports, and idioms. Match the file's comment density:
-  do not add explanatory comments unless the user asks or the code is already
+- For edits to existing files, use edit_file for a targeted change (old_string
+  must match the file exactly and be unique, so include a few surrounding
+  lines) and apply_patch when one coherent change spans several hunks or
+  files. Use write_file only to create a file or when most of it changes; do
+  not rewrite a whole file to change a few lines. Match the existing
+  indentation, imports, and idioms. Match the file's comment density: do not
+  add explanatory comments unless the user asks or the code is already
   comment-dense.
+- A successful edit result already confirms the change; do not re-read a file
+  just to verify an edit that succeeded. If an edit fails, read the error, fix
+  the old_string or hunk, and retry the same tool rather than switching to a
+  full rewrite.
 - Solve the problem as posed, not a more general version of it. Add no
   speculative abstraction, configurability, or handling for cases that cannot
   occur, and nothing the user did not ask for. A small diff can still be
@@ -105,7 +112,7 @@ work.
   you need to clean up a running foreground command yourself, use write_stdin.
 - write_stdin's session_id is only ever an id returned by a still-running
   exec_command; never guess or probe ids. If you have no such session, start one
-  with exec_command, or use write_file/apply_patch for file changes.
+  with exec_command, or use edit_file/apply_patch/write_file for file changes.
 - write_stdin with empty input polls an existing exec_command session, and
   `\u0003` interrupts it. Sending other stdin bytes may require approval because
   it can drive the running process beyond the original command. Non-tty sessions

internal/agent/system_prompt_models.go

@@ -60,14 +60,18 @@ const openAIPromptAddendum = `<model_guidance>
   longer answers, fenced code blocks for code, and ` + "`inline code`" + ` for paths,
   commands, and symbols.
 - Strongly prefer the native file tools (read_file, list_directory, grep, glob,
-  write_file, apply_patch) over shelling out to cat/sed/awk/python for
-  file work. Make one tool call per file; do not batch file writes into a script.
+  edit_file, apply_patch, write_file) over shelling out to cat/sed/awk/python
+  for file work. read_file, edit_file and write_file take one file per call;
+  a single apply_patch may span several files when they form one coherent
+  change. Do not batch file writes into a script. Independent tool calls
+  (several reads, several edit_file calls to different files) belong in the
+  same turn.
 - Persist until the task is fully handled this turn: gather context, implement,
   run the validators, and report — do not stop at a partial result.
 </model_guidance>`
 
 const geminiPromptAddendum = `<model_guidance>
-- Prefer the dedicated tools (read_file, grep, glob, apply_patch) over
+- Prefer the dedicated tools (read_file, grep, glob, edit_file, apply_patch) over
   equivalent shell commands; they are safer and produce cleaner diffs.
 - Be concise and concrete. When you run a shell command with side effects, state
   in one short clause why it is needed.

internal/agent/system_prompt_test.go

@@ -36,7 +36,7 @@ func TestCoreSystemPromptIncludesCodingQualityRules(t *testing.T) {
 		"inspect the target file",
 		"plan then act",
 		"choose the narrowest tool",
-		"for edits to existing files, prefer apply_patch",
+		"for edits to existing files, use edit_file",
 		"verify after edits",
 		"honor the active permission mode",
 		"avoid broad refactors",

internal/sandbox/apply_patch_paths_test.go

@@ -0,0 +1,96 @@
+package sandbox
+
+import (
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestApplyPatchPathBlockOnlyRejectsRelativeTraversal(t *testing.T) {
+	root := t.TempDir()
+	inside := filepath.Join(root, "main.js")
+	structured := func(path string) string {
+		return strings.Join([]string{"*** Begin Patch", "*** Update File: " + path, "@@", "-a", "+b", "*** End Patch"}, "\n")
+	}
+	for name, patch := range map[string]string{
+		"absolute inside workspace": structured(inside),
+		"relative":                  structured("main.js"),
+		"decorated markers":         "*** Begin Patch ***\n*** Update File: main.js\n@@\n-a\n+b\n*** End Patch ***",
+		"no-space marker":           "***Begin Patch\n*** Update File: main.js\n@@\n-a\n+b\n***End Patch",
+	} {
+		request := Request{ToolName: "apply_patch", WorkspaceRoot: root, SideEffect: SideEffectWrite, Args: map[string]any{"patch": patch}}
+		if block := applyPatchPathBlock(request); block != nil {
+			t.Fatalf("%s: unexpected block %+v", name, block)
+		}
+	}
+	for _, path := range []string{"../escape.js", ".."} {
+		for name, patch := range map[string]string{"canonical": structured(path), "no-space": "***Begin Patch\n*** Update File: " + path + "\n@@\n-a\n+b\n***End Patch"} {
+			request := Request{ToolName: "apply_patch", WorkspaceRoot: root, SideEffect: SideEffectWrite, Args: map[string]any{"patch": patch}}
+			block := applyPatchPathBlock(request)
+			if block == nil || block.Code != BlockOutsideWorkspace {
+				t.Fatalf("%s %q must be blocked as traversal, got %+v", name, path, block)
+			}
+		}
+	}
+}
+
+// Every marker spelling the tool applies must be classified as structured at
+// the sandbox boundary too; otherwise the boundary scans the patch as a unified
+// diff, extracts no targets, and validates nothing (fail-open).
+func TestStructuredPatchClassifierMatchesToolSpellings(t *testing.T) {
+	for _, header := range []string{"*** Begin Patch", "*** Begin Patch ***", "***Begin Patch", "  *** Begin Patch  ", "\ufeff*** Begin Patch"} {
+		patch := header + "\n*** Update File: main.js\n@@\n-a\n+b\n*** End Patch"
+		if !IsStructuredPatch(patch) {
+			t.Fatalf("%q must classify as a structured patch", header)
+		}
+		if paths := applyPatchPaths(patch); len(paths) != 1 || paths[0] != "main.js" {
+			t.Fatalf("%q: sandbox must extract the structured target, got %v", header, paths)
+		}
+	}
+	for _, header := range []string{"--- a/x", "Begin Patch", "*** Begin Patchwork", "*** Update File: x"} {
+		if IsStructuredPatch(header + "\n-a\n+b") {
+			t.Fatalf("%q must not classify as a structured patch", header)
+		}
+	}
+	if StructuredPatchMarker("*** End Patch ***") != "end" || StructuredPatchMarker("***End Patch") != "end" {
+		t.Fatal("decorated end markers must classify as end")
+	}
+}
+
+func TestApplyPatchRequestPathsCarryAbsolutePathsToScopeValidation(t *testing.T) {
+	root := t.TempDir()
+	// NewScope also grants the system temp dir, so a sibling t.TempDir() is
+	// legitimately in scope; pick a path under the filesystem root instead.
+	outside, err := filepath.Abs(filepath.Join(string(filepath.Separator), "zero-outside-workspace-test", "escape.js"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	scope, err := NewScope(root, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	inside := filepath.Join(root, "main.js")
+	structured := func(header, footer, path string) string {
+		return strings.Join([]string{header, "*** Update File: " + path, "@@", "-a", "+b", footer}, "\n")
+	}
+	for name, spelling := range map[string][2]string{"canonical": {"*** Begin Patch", "*** End Patch"}, "no-space": {"***Begin Patch", "***End Patch"}} {
+		// Failure path: the exact path the boundary parsed must be denied by the
+		// scope. structuredPatchHeaderPaths normalises separators to "/", so the
+		// parsed form is compared in slash form and then validated as-is.
+		paths := applyPatchRequestPaths(map[string]any{"patch": structured(spelling[0], spelling[1], outside)})
+		if len(paths) != 1 || paths[0] != filepath.ToSlash(outside) {
+			t.Fatalf("%s: absolute patch path must reach scope validation unchanged, got %v", name, paths)
+		}
+		if block := scope.validate(paths[0]); block == nil || block.Code != BlockOutsideWorkspace {
+			t.Fatalf("%s: scope must deny the parsed outside path %q, got %+v", name, paths[0], block)
+		}
+		// Success path: the parsed inside path must be accepted by the scope.
+		paths = applyPatchRequestPaths(map[string]any{"patch": structured(spelling[0], spelling[1], inside)})
+		if len(paths) != 1 || paths[0] != filepath.ToSlash(inside) {
+			t.Fatalf("%s: inside patch path must reach scope validation unchanged, got %v", name, paths)
+		}
+		if block := scope.validate(paths[0]); block != nil {
+			t.Fatalf("%s: scope must accept the parsed inside path %q, got %+v", name, paths[0], block)
+		}
+	}
+}

internal/sandbox/risk.go

@@ -281,11 +281,15 @@ func applyPatchPathBlock(request Request) *pathBlock {
 	if patch == "" {
 		return nil
 	}
+	// Only relative traversal is rejected up front. Absolute paths flow through
+	// the regular workspace-scope validation below (requestPaths), which accepts
+	// one inside the workspace and denies one outside — a model that echoes the
+	// absolute path read_file showed it must not be blocked for that alone.
 	for _, path := range applyPatchPaths(patch) {
 		if path == "" || path == "/dev/null" {
 			continue
 		}
-		if filepath.IsAbs(path) || path == ".." || strings.HasPrefix(path, "../") {
+		if path == ".." || strings.HasPrefix(path, "../") {
 			return &pathBlock{
 				Code:   BlockOutsideWorkspace,
 				Path:   path,
@@ -296,8 +300,35 @@ func applyPatchPathBlock(request Request) *pathBlock {
 	return nil
 }
 
+// structuredPatchMarkerPattern is the single classifier for structured-patch
+// markers, shared by the sandbox boundary and the apply_patch tool (which
+// imports this package). It accepts the canonical "*** Begin Patch" /
+// "*** End Patch" and the decorated spellings models emit ("*** Begin Patch ***",
+// "***Begin Patch", trailing whitespace). Both sides must agree: a spelling the
+// tool would apply but the sandbox did not recognise would make the sandbox
+// scan the patch as a unified diff, extract no targets, and validate nothing.
+var structuredPatchMarkerPattern = regexp.MustCompile(`^\*{3}\s*(Begin|End) Patch\s*\**\s*$`)
+
+// StructuredPatchMarker classifies a line as the "begin" or "end" marker of a
+// structured patch, or "" when it is neither.
+func StructuredPatchMarker(line string) string {
+	match := structuredPatchMarkerPattern.FindStringSubmatch(strings.TrimSpace(line))
+	if match == nil {
+		return ""
+	}
+	return strings.ToLower(match[1])
+}
+
+// IsStructuredPatch reports whether patch opens with a structured begin marker.
+// The tool applies exactly the patches this returns true for, so the sandbox
+// extracts structured header paths for exactly the same set.
+func IsStructuredPatch(patch string) bool {
+	first, _, _ := strings.Cut(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), "\n")
+	return StructuredPatchMarker(first) == "begin"
+}
+
 func applyPatchPaths(patch string) []string {
-	if strings.HasPrefix(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), "*** Begin Patch") {
+	if IsStructuredPatch(patch) {
 		return structuredPatchHeaderPaths(patch)
 	}
 	return patchHeaderPaths(patch)