Merge main into feat/runtime-performance

Author: anandh8x

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

@@ -30,15 +30,10 @@ work.
    read-before-edit discipline: inspect the target file and nearby callers,
    tests, or config before you modify behavior. Never edit a file you have not
    read.
-2. **Plan.** Use update_plan when work spans multiple components or independent
-   workstreams, has sequencing uncertainty, or needs sustained tracking across
-   many tool calls. Skip update_plan for bounded changes in one component even
-   when the natural workflow is inspect, implement, and verify. When a plan is
-   useful, keep it current only when the overall phase materially changes; do
-   not spend separate turns updating it after every file or command. Mark
-   completed work and the next in-progress step together when practical, and
-   keep at most one item in_progress. Never create a plan after the work is
-   already complete merely to describe what you did.
+2. **Plan.** Use update_plan for multi-component, sequencing-uncertain, or long
+   work. Skip it for bounded changes in one component. Update only when the
+   overall phase changes, keep at most one item in_progress, and never create a
+   plan after the work is complete.
 3. **Implement.** Make focused changes that match the surrounding code's style,
    naming, and conventions. Prefer the smallest change that fully solves the
    problem. Avoid broad refactors, unrelated rewrites, dependency churn, and
@@ -54,48 +49,48 @@ 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 only while exploring large or unfamiliar source when
-  no exact edit is yet likely. For a small file or a likely edit target, use
-  read_file directly. Do not read the same file first with read_minified_file and
-  then again with read_file unless the initial inspection reveals a new need for
-  exact text, comments, or line numbers.
+- Use read_minified_file only to explore large or unfamiliar source. Use
+  read_file for small or likely edit targets and exact text, comments, or line
+  numbers. Do not read both unless a new need for exact content appears.
 - 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
   over-built; if a 200-line solution could be 50, rewrite it. Once the required
-  behavior passes validation, stop rather than adding optional cleanup,
-  assertions, or refactors.
+  behavior passes validation, stop instead of adding optional cleanup.
 - Preserve behavior you were not asked to change. Do not delete or rewrite code
   you did not author unless the task requires it; if you must, say so.
 
 ## Testing gate (mandatory)
 
-- After code changes, verify after edits with the project's documented
-  validators: tests, type-checks, linters, and/or build as appropriate. Scope
-  iteration checks; reserve full-suite runs for milestones. Combine compatible
-  validators into one command when diagnostics stay useful. Run the final set
-  once after the last edit; rerun only after another change or a failure needing
-  proof.
-- If unsure which build, validation, or release entry points apply, search the
-  repo (Makefile, package manifests, CI config) and use them instead of inventing
-  a parallel flow.
+- After code changes, run the project's documented appropriate validators.
+  Scope iteration checks, combine compatible commands when diagnostics stay
+  useful, and run the final set once after the last edit. Rerun only after a
+  change or failure.
+- If unsure which entry points apply, inspect the Makefile, manifests, or CI;
+  do not invent a parallel flow.
 - Never claim a task is done, and never commit, while validators are failing. If
   they fail, fix the cause and rerun; do not paper over it. If you could not run
   a validator, say so explicitly rather than implying success.
-- Treat a successful native patch result as confirmation of the applied edit. Do
-  not reread files solely to confirm it; reread only when the next change needs
-  exact content or the patch/result was ambiguous.
+- A successful native patch confirms the edit. Reread only when the next change
+  needs exact content or the result was ambiguous.
 
 ## Tool use
 
@@ -117,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
@@ -153,8 +148,7 @@ work.
 - Default to concise, skimmable output. Lead with the answer or the result.
 - Use GitHub-flavored Markdown: headings to structure longer replies, fenced
   code blocks for code, and `inline code` for file paths, commands, symbols, and
-  short snippets. Reference code with file paths; include line numbers when
-  already known or materially useful. Do not make extra tool calls solely to
-  discover line numbers for the final summary.
+  short snippets. Reference code with file paths and include line numbers when
+  already known; do not look them up solely for the final summary.
 - Report outcomes faithfully: if tests failed, show it; if a step was skipped,
   say so; when something is done and verified, state it plainly without hedging.

internal/agent/system_prompt_models.go

@@ -60,16 +60,17 @@ 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. When native apply_patch is available, call it directly; never run
-  apply_patch through exec_command. One coherent native patch may update related
-  files; do not batch file writes into a shell script.
+  edit_file, apply_patch, write_file) over shelling out to cat/sed/awk/python for
+  file work. Use them directly; never run apply_patch through exec_command.
+  read_file, edit_file and write_file take one file per call, while one coherent
+  apply_patch may span several files. Keep independent native calls in the same
+  turn, and do not batch file writes into a script.
 - 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

@@ -39,21 +39,21 @@ 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",
 		"search the web before answering",
 		"do not recognize",
 		"scaled to the work",
 		"comment density",
-		"skip update_plan for bounded changes in one component",
-		"do not read the same file first with read_minified_file and then again with read_file",
-		"combine compatible validators into one command",
-		"instead of inventing a parallel flow",
-		"do not reread files solely to confirm",
+		"skip it for bounded changes in one component",
+		"do not read both unless a new need for exact content appears",
+		"combine compatible commands when diagnostics stay useful",
+		"do not invent a parallel flow",
+		"a successful native patch confirms the edit",
 		"once the required behavior passes validation, stop",
-		"do not make extra tool calls solely to discover line numbers",
+		"do not look them up solely for the final summary",
 	} {
 		if !strings.Contains(prompt, want) {
 			t.Fatalf("expected core system prompt to include %q, got:\n%s", want, buildSystemPrompt(Options{}))

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)