diff --git a/internal/agent/prompt_budget_test.go b/internal/agent/prompt_budget_test.go
index 912b479a7..77c25072c 100644
--- a/internal/agent/prompt_budget_test.go
+++ b/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")
}
}
diff --git a/internal/agent/system_prompt.md b/internal/agent/system_prompt.md
index 1e3393088..8b3e9c46c 100644
--- a/internal/agent/system_prompt.md
+++ b/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
diff --git a/internal/agent/system_prompt_models.go b/internal/agent/system_prompt_models.go
index 7db41d247..c66773577 100644
--- a/internal/agent/system_prompt_models.go
+++ b/internal/agent/system_prompt_models.go
@@ -60,14 +60,18 @@ const openAIPromptAddendum = `
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.
`
const geminiPromptAddendum = `
-- 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.
diff --git a/internal/agent/system_prompt_test.go b/internal/agent/system_prompt_test.go
index 007642979..09f7c05db 100644
--- a/internal/agent/system_prompt_test.go
+++ b/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",
diff --git a/internal/sandbox/apply_patch_paths_test.go b/internal/sandbox/apply_patch_paths_test.go
new file mode 100644
index 000000000..3f9a2dc66
--- /dev/null
+++ b/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)
+ }
+ }
+}
diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go
index a37868c63..faa4e09bc 100644
--- a/internal/sandbox/risk.go
+++ b/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)
diff --git a/internal/tools/apply_patch.go b/internal/tools/apply_patch.go
index 2a74ae90a..4862b65f1 100644
--- a/internal/tools/apply_patch.go
+++ b/internal/tools/apply_patch.go
@@ -3,11 +3,11 @@ package tools
import (
"context"
"fmt"
- "os"
- "os/exec"
"path/filepath"
"strconv"
"strings"
+
+ "github.com/Gitlawb/zero/internal/sandbox"
)
type applyPatchTool struct {
@@ -20,11 +20,11 @@ func NewScopedApplyPatchTool(workspaceRoot string, scope PathScope) Tool {
return applyPatchTool{
baseTool: baseTool{
name: "apply_patch",
- description: "Apply a patch inside the workspace or an explicitly granted extra write root.",
+ description: "Apply a multi-hunk or multi-file patch inside the workspace (or a granted extra write root). Structured format:\n*** Begin Patch\n*** Update File: src/app.js\n@@\n unchanged context line\n-removed line\n+added line\n*** End Patch\nUse \"*** Add File: path\" with \"+\" lines to create a file and \"*** Delete File: path\" to remove one; several sections may follow each other. A unified diff (---/+++ headers with a/ b/ prefixes, @@ hunks) is also accepted and applied in-process. Paths are workspace-relative; absolute paths inside the workspace are fine. For a single targeted change, edit_file is simpler.",
parameters: Schema{
Type: "object",
Properties: map[string]PropertySchema{
- "patch": {Type: "string", Description: "A unified diff or a structured *** Begin Patch patch to apply."},
+ "patch": {Type: "string", Description: "The structured (*** Begin Patch) or unified-diff patch text."},
"cwd": {Type: "string", Description: "Directory where the patch should be applied. Relative paths stay in the workspace; use an absolute path to target a granted extra write root. Defaults to workspace root.", Default: "."},
},
Required: []string{"patch"},
@@ -59,177 +59,14 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a
if isStructuredPatch(patch) {
return tool.runStructuredPatch(applyRoot, relativeRoot, patch, options)
}
- if err := validatePatchPaths(applyRoot, patch); err != nil {
- return errorResult("Error applying patch: " + err.Error())
- }
-
- tempFile, err := os.CreateTemp("", "zero-patch-*.patch")
+ // Unified diffs are translated into the same operations and applied by
+ // the same os.Root engine, so neither format opens a target by pathname
+ // after validation (no check-to-use window) and git is not needed.
+ operations, err := parseUnifiedPatch(patch)
if err != nil {
return errorResult("Error applying patch: " + err.Error())
}
- patchPath := tempFile.Name()
- defer func() {
- _ = os.Remove(patchPath)
- }()
- if _, err := tempFile.WriteString(patch); err != nil {
- _ = tempFile.Close()
- return errorResult("Error applying patch: " + err.Error())
- }
- if err := tempFile.Close(); err != nil {
- return errorResult("Error applying patch: " + err.Error())
- }
-
- if err := recheckPatchWriteTargets(applyRoot, patch); err != nil {
- return errorResult("Error applying patch: " + err.Error())
- }
- var createdTargets []string
- var fullySuppliedTargets []string
- wholeBefore := map[string]bool{}
- if options.FileTracker != nil {
- createdTargets = missingPatchTargets(applyRoot, patch)
- fullySuppliedTargets = completeCreatedPatchTargets(applyRoot, patch)
- for _, path := range patchHeaderPaths(patch) {
- if path == "" || path == "/dev/null" {
- continue
- }
- if absolute, _, rerr := resolveWorkspaceTargetPath(applyRoot, path); rerr == nil {
- wholeBefore[absolute] = options.FileTracker.SeenWhole(absolute)
- }
- }
- }
-
- command := exec.CommandContext(ctx, "git", "apply", "--whitespace=nowarn", patchPath)
- command.Dir = applyRoot
- output, err := command.CombinedOutput()
- if err != nil {
- message := strings.TrimSpace(string(output))
- if message == "" {
- message = err.Error()
- }
- return errorResult("Error applying patch: " + message)
- }
-
- summary := "Patch applied successfully."
- if relativeRoot != "." {
- summary = "Patch applied successfully in " + relativeRoot + "."
- }
- result := okResult(summary)
- result.ChangedFiles = changedFilesFromPatch(relativeRoot, patch)
- result.Display = Display{Summary: summary, Kind: "diff", Preview: capPreviewDiff(patch)}
- fullySupplied := make(map[string]bool, len(fullySuppliedTargets))
- for _, absolute := range fullySuppliedTargets {
- fullySupplied[absolute] = true
- }
- // Re-baseline files changed by this tool. When the model had already seen the
- // whole input (or supplied a complete new file), the exact patch plus that
- // input determines the whole output, so a follow-up edit needs no wasted read.
- // Partial observations remain conservative and are cleared by Record.
- for _, changed := range result.ChangedFiles {
- if absolute, _, rerr := resolveScopedPath(tool.workspaceRoot, tool.scope, changed); rerr == nil {
- content, readErr := os.ReadFile(absolute)
- if readErr != nil {
- options.FileTracker.Forget(absolute)
- continue
- }
- info, _ := os.Stat(absolute)
- wasWhole := wholeBefore[absolute] || fullySupplied[absolute]
- options.FileTracker.Record(absolute, content, info)
- if wasWhole {
- lines := lineCount(string(content))
- options.FileTracker.RecordSeenRange(absolute, 1, lines, lines)
- }
- }
- }
- recordCreatedPatchTargets(options.FileTracker, createdTargets)
- return result
-}
-
-func missingPatchTargets(root string, patch string) []string {
- seen := map[string]bool{}
- var missing []string
- for _, path := range patchHeaderPaths(patch) {
- if path == "" || path == "/dev/null" {
- continue
- }
- absolute, _, err := resolveWorkspaceTargetPath(root, path)
- if err != nil || seen[absolute] {
- continue
- }
- seen[absolute] = true
- if _, err := os.Stat(absolute); os.IsNotExist(err) {
- missing = append(missing, absolute)
- }
- }
- return missing
-}
-
-// completeCreatedPatchTargets returns only files whose full contents are
-// supplied by a /dev/null creation patch. A missing rename/copy destination is
-// created by git too, but its bytes come from an unread source and must not gain
-// whole-file observation credit.
-func completeCreatedPatchTargets(root string, patch string) []string {
- seen := map[string]bool{}
- var created []string
- oldRemaining, newRemaining := 0, 0
- inHunk := false
- fromDevNull := false
- for _, line := range strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") {
- if inHunk && (oldRemaining > 0 || newRemaining > 0) {
- switch {
- case strings.HasPrefix(line, "-"):
- oldRemaining--
- case strings.HasPrefix(line, "+"):
- newRemaining--
- case strings.HasPrefix(line, "\\"):
- default:
- oldRemaining--
- newRemaining--
- }
- continue
- }
- inHunk = false
- switch {
- case strings.HasPrefix(line, "diff --git "):
- fromDevNull = false
- case strings.HasPrefix(line, "@@"):
- oldRemaining, newRemaining = parseHunkCounts(line)
- inHunk = oldRemaining > 0 || newRemaining > 0
- case strings.HasPrefix(line, "--- "):
- fromDevNull = patchFileHeaderPath(line) == "/dev/null"
- case strings.HasPrefix(line, "+++ "):
- path := patchFileHeaderPath(line)
- if !fromDevNull || path == "" || path == "/dev/null" {
- fromDevNull = false
- continue
- }
- fromDevNull = false
- absolute, _, err := resolveWorkspaceTargetPath(root, stripPatchPrefix(path))
- if err != nil || seen[absolute] {
- continue
- }
- if _, err := os.Stat(absolute); !os.IsNotExist(err) {
- continue
- }
- seen[absolute] = true
- created = append(created, absolute)
- }
- }
- return created
-}
-
-func recordCreatedPatchTargets(tracker *FileTracker, missingBefore []string) {
- if tracker == nil {
- return
- }
- for _, absolute := range missingBefore {
- if _, err := os.Stat(absolute); err != nil {
- continue
- }
- if resolved, err := filepath.EvalSymlinks(absolute); err == nil {
- absolute = resolved
- }
- tracker.RecordCreated(absolute)
- }
+ return applyPatchOperations(applyRoot, relativeRoot, operations, options)
}
// changedFilesFromPatch extracts the unique, WORKSPACE-relative paths a patch
@@ -260,27 +97,35 @@ func changedFilesFromPatch(relativeRoot string, patch string) []string {
return paths
}
-func validatePatchPaths(root string, patch string) error {
- for _, path := range patchHeaderPaths(patch) {
- if path == "" || path == "/dev/null" {
- continue
- }
- if filepath.IsAbs(path) || path == ".." || strings.HasPrefix(path, "../") {
- return fmt.Errorf("patch path %q must stay inside the workspace", path)
- }
- if _, _, err := resolveWorkspaceTargetPath(root, path); err != nil {
- return err
- }
+// normalizePatchPathForRoot resolves platform-level symlinks (macOS /var ->
+// /private/var) in the prefix of an absolute patch path that lies outside the
+// apply root, so a path the model copied from read_file compares equal to the
+// symlink-resolved root. Relative paths are returned unchanged.
+func normalizePatchPathForRoot(root string, path string) string {
+ if !filepath.IsAbs(path) {
+ return path
}
- return nil
+ resolvedRoot, err := filepath.Abs(root)
+ if err != nil {
+ return path
+ }
+ if evaluated, err := filepath.EvalSymlinks(resolvedRoot); err == nil {
+ resolvedRoot = evaluated
+ }
+ return sandbox.NormalizePrefixForRoot(path, resolvedRoot)
}
-func recheckPatchWriteTargets(root string, patch string) error {
+func validatePatchPaths(root string, patch string) error {
for _, path := range patchHeaderPaths(patch) {
if path == "" || path == "/dev/null" {
continue
}
- if err := recheckWorkspaceWriteTarget(root, path); err != nil {
+ // Relative traversal is rejected here; an absolute path is checked by
+ // resolveWorkspaceTargetPath, which only accepts one inside the root.
+ if path == ".." || strings.HasPrefix(path, "../") {
+ return fmt.Errorf("patch path %q must stay inside the workspace", path)
+ }
+ if _, _, err := resolveWorkspaceTargetPath(root, normalizePatchPathForRoot(root, path)); err != nil {
return err
}
}
@@ -351,7 +196,7 @@ func patchFileHeaderPath(line string) string {
// tokens. Scanning the whole line would let a crafted heading like
// "@@ -1,1 +1,1 @@ +1,999999" overwrite the real count, keep the parser stuck in
// hunk mode, and swallow later "--- "/"+++ " file headers so they escape
-// validatePatchPaths / recheckPatchWriteTargets — a workspace-confinement bypass.
+// validatePatchPaths — a workspace-confinement bypass.
func parseHunkCounts(line string) (int, int) {
_, rest, ok := strings.Cut(line, "@@")
if !ok {
diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go
new file mode 100644
index 000000000..2561d4eb7
--- /dev/null
+++ b/internal/tools/apply_patch_tolerance_test.go
@@ -0,0 +1,712 @@
+package tools
+
+import (
+ "context"
+ "os"
+ "path/filepath"
+ "strconv"
+ "strings"
+ "testing"
+)
+
+// Models routinely decorate the structured markers ("*** Begin Patch ***") and
+// write unified-diff ranges after "@@"; both used to fail on the first line and
+// pushed the model into whole-file rewrites.
+func TestApplyPatchToleratesDecoratedMarkersAndRangeHeaders(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "hello.txt"), "hello\nold\nbye\n")
+
+ patch := strings.Join([]string{
+ "*** Begin Patch ***",
+ "*** Update File: hello.txt",
+ "@@ -1,3 +1,3 @@",
+ " hello",
+ "-old",
+ "+new",
+ "*** End Patch ***",
+ "",
+ }, "\n")
+
+ if !isStructuredPatch(patch) {
+ t.Fatal("decorated begin marker must still be recognised as a structured patch")
+ }
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status != StatusOK {
+ t.Fatalf("decorated structured patch should apply, got %s: %s", result.Status, result.Output)
+ }
+ content, err := os.ReadFile(filepath.Join(root, "hello.txt"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if got := strings.ReplaceAll(string(content), "\r\n", "\n"); got != "hello\nnew\nbye\n" {
+ t.Fatalf("patched content = %q", got)
+ }
+}
+
+func TestStructuredHunkAnchorKeepsHeadingAndPlainContext(t *testing.T) {
+ cases := map[string]string{
+ "-12,4 +12,6": "",
+ "-12 +12": "",
+ "-12,4 +12,6 @@": "",
+ "-12,4 +12,6 @@ func main() {": "func main() {",
+ "func main() {": "func main() {",
+ "": "",
+ }
+ for input, want := range cases {
+ if got := structuredHunkAnchor(input); got != want {
+ t.Fatalf("structuredHunkAnchor(%q) = %q, want %q", input, got, want)
+ }
+ }
+}
+
+func TestStructuredPatchMarkerSpellings(t *testing.T) {
+ begin := []string{"*** Begin Patch", "*** Begin Patch ***", "***Begin Patch", " *** Begin Patch ", "*** Begin Patch **"}
+ for _, line := range begin {
+ if structuredPatchMarker(line) != "begin" {
+ t.Fatalf("%q must read as the begin marker", line)
+ }
+ }
+ if structuredPatchMarker("*** End Patch ***") != "end" {
+ t.Fatal("decorated end marker must read as the end marker")
+ }
+ for _, line := range []string{"*** Update File: x", "Begin Patch", "*** Begin Patchwork"} {
+ if structuredPatchMarker(line) != "" {
+ t.Fatalf("%q must not read as a marker", line)
+ }
+ }
+}
+
+func TestStructuredPatchHeaderErrorCarriesFormatHint(t *testing.T) {
+ _, err := parseStructuredPatch("--- a/x\n+++ b/x\n")
+ if err == nil || !strings.Contains(err.Error(), "*** Update File: path") {
+ t.Fatalf("header error should teach the format, got %v", err)
+ }
+}
+
+func TestApplyPatchAcceptsAbsoluteInWorkspacePaths(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "hello.txt"), "hello\nold\n")
+ absolute := filepath.Join(root, "hello.txt")
+
+ structured := strings.Join([]string{
+ "*** Begin Patch",
+ "*** Update File: " + absolute,
+ "@@",
+ " hello",
+ "-old",
+ "+structured",
+ "*** End Patch",
+ "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": structured})
+ if result.Status != StatusOK {
+ t.Fatalf("absolute in-workspace structured path should apply, got %s: %s", result.Status, result.Output)
+ }
+ content, _ := os.ReadFile(absolute)
+ if got := strings.ReplaceAll(string(content), "\r\n", "\n"); got != "hello\nstructured\n" {
+ t.Fatalf("structured content = %q", got)
+ }
+ if len(result.ChangedFiles) != 1 || result.ChangedFiles[0] != "hello.txt" {
+ t.Fatalf("changed files should be workspace-relative, got %v", result.ChangedFiles)
+ }
+
+ unified := strings.Join([]string{
+ "--- " + absolute,
+ "+++ " + absolute,
+ "@@ -1,2 +1,2 @@",
+ " hello",
+ "-structured",
+ "+unified",
+ "",
+ }, "\n")
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": unified})
+ if result.Status != StatusOK {
+ t.Fatalf("absolute in-workspace unified path should apply, got %s: %s", result.Status, result.Output)
+ }
+ content, _ = os.ReadFile(absolute)
+ if got := strings.ReplaceAll(string(content), "\r\n", "\n"); got != "hello\nunified\n" {
+ t.Fatalf("unified content = %q", got)
+ }
+}
+
+func TestApplyPatchStillRejectsAbsolutePathsOutsideWorkspace(t *testing.T) {
+ root := t.TempDir()
+ outside := filepath.Join(t.TempDir(), "escape.txt")
+ writeTestFile(t, outside, "old\n")
+
+ for name, patch := range map[string]string{
+ "structured": strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-old", "+new", "*** End Patch", ""}, "\n"),
+ "unified": strings.Join([]string{"--- " + outside, "+++ " + outside, "@@ -1 +1 @@", "-old", "+new", ""}, "\n"),
+ } {
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status == StatusOK {
+ t.Fatalf("%s patch outside the workspace must be rejected", name)
+ }
+ if content, _ := os.ReadFile(outside); string(content) != "old\n" {
+ t.Fatalf("%s patch must not touch a file outside the workspace", name)
+ }
+ }
+}
+
+// Unified diffs are applied in-process through the same os.Root engine as
+// structured patches: git C-quoted paths with whitespace resolve to one file,
+// and the same header outside the workspace is refused before anything is
+// written.
+func TestUnifiedPatchQuotedWhitespacePaths(t *testing.T) {
+ root := t.TempDir()
+ dir := filepath.Join(root, "work tree")
+ if err := os.MkdirAll(dir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ target := filepath.Join(dir, "file.txt")
+ writeTestFile(t, target, "hello\nold\n")
+
+ quoted := func(path string) string { return strconv.Quote(path) }
+ inside := strings.Join([]string{
+ "diff --git " + quoted("a/"+filepath.ToSlash(target)) + " " + quoted("b/"+filepath.ToSlash(target)),
+ "--- " + quoted(target),
+ "+++ " + quoted(target),
+ "@@ -1,2 +1,2 @@",
+ " hello",
+ "-old",
+ "+new",
+ "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": inside})
+ if result.Status != StatusOK {
+ t.Fatalf("quoted whitespace path inside the workspace should apply, got %s: %s", result.Status, result.Output)
+ }
+ content, _ := os.ReadFile(target)
+ if got := strings.ReplaceAll(string(content), "\r\n", "\n"); got != "hello\nnew\n" {
+ t.Fatalf("content = %q", got)
+ }
+ if len(result.ChangedFiles) != 1 || result.ChangedFiles[0] != "work tree/file.txt" {
+ t.Fatalf("changed files = %v", result.ChangedFiles)
+ }
+
+ outsideDir := filepath.Join(t.TempDir(), "other tree")
+ if err := os.MkdirAll(outsideDir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ outside := filepath.Join(outsideDir, "file.txt")
+ writeTestFile(t, outside, "hello\nold\n")
+ escape := strings.Join([]string{"--- " + quoted(outside), "+++ " + quoted(outside), "@@ -1,2 +1,2 @@", " hello", "-old", "+new", ""}, "\n")
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": escape})
+ if result.Status == StatusOK {
+ t.Fatal("quoted whitespace path outside the workspace must be rejected")
+ }
+ if content, _ := os.ReadFile(outside); string(content) != "hello\nold\n" {
+ t.Fatal("outside file must be untouched")
+ }
+}
+
+// A workspace path that is swapped for a symlink escaping the root — the
+// classic check-to-use attack — is refused by the os.Root engine for both
+// formats, and the outside file is never modified.
+func TestApplyPatchRefusesSymlinkEscapingWorkspace(t *testing.T) {
+ root := t.TempDir()
+ outside := filepath.Join(t.TempDir(), "secret.txt")
+ writeTestFile(t, outside, "hello\nold\n")
+ link := filepath.Join(root, "target.txt")
+ if err := os.Symlink(outside, link); err != nil {
+ t.Skipf("symlinks unavailable: %v", err)
+ }
+ for name, patch := range map[string]string{
+ "unified": strings.Join([]string{"--- a/target.txt", "+++ b/target.txt", "@@ -1,2 +1,2 @@", " hello", "-old", "+new", ""}, "\n"),
+ "structured": strings.Join([]string{"*** Begin Patch", "*** Update File: target.txt", "@@", " hello", "-old", "+new", "*** End Patch", ""}, "\n"),
+ } {
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status == StatusOK {
+ t.Fatalf("%s patch through an escaping symlink must be refused", name)
+ }
+ if content, _ := os.ReadFile(outside); string(content) != "hello\nold\n" {
+ t.Fatalf("%s patch must not modify the file outside the workspace", name)
+ }
+ }
+}
+
+func TestUnifiedPatchCreatesDeletesAndHonoursNoNewlineMarker(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "bye\n")
+ writeTestFile(t, filepath.Join(root, "keep.txt"), "a\nb\n")
+ patch := strings.Join([]string{
+ "diff --git a/new.txt b/new.txt",
+ "new file mode 100644",
+ "--- /dev/null",
+ "+++ b/new.txt",
+ "@@ -0,0 +1,2 @@",
+ "+first",
+ "+second",
+ "\\ No newline at end of file",
+ "diff --git a/gone.txt b/gone.txt",
+ "deleted file mode 100644",
+ "--- a/gone.txt",
+ "+++ /dev/null",
+ "@@ -1 +0,0 @@",
+ "-bye",
+ "--- a/keep.txt",
+ "+++ b/keep.txt",
+ "@@ -1,2 +1,2 @@",
+ " a",
+ "-b",
+ "+c",
+ "\\ No newline at end of file",
+ "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status != StatusOK {
+ t.Fatalf("multi-file unified patch should apply, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "new.txt")); string(content) != "first\nsecond" {
+ t.Fatalf("created file = %q", string(content))
+ }
+ if _, err := os.Stat(filepath.Join(root, "gone.txt")); !os.IsNotExist(err) {
+ t.Fatal("deleted file must be gone")
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "keep.txt")); string(content) != "a\nc" {
+ t.Fatalf("updated file = %q", string(content))
+ }
+ if len(result.ChangedFiles) != 3 {
+ t.Fatalf("changed files = %v", result.ChangedFiles)
+ }
+}
+
+func TestUnifiedPatchUsesRangeHintThenFallsBackToContext(t *testing.T) {
+ root := t.TempDir()
+ // Two identical blocks: the hunk's range picks the second one, which
+ // content search alone would call ambiguous.
+ writeTestFile(t, filepath.Join(root, "dup.txt"), "x\ny\nx\ny\n")
+ patch := strings.Join([]string{"--- a/dup.txt", "+++ b/dup.txt", "@@ -3,2 +3,2 @@", " x", "-y", "+z", ""}, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status != StatusOK {
+ t.Fatalf("range-hinted hunk should apply, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "dup.txt")); string(content) != "x\ny\nx\nz\n" {
+ t.Fatalf("content = %q", string(content))
+ }
+ // A stale range (file grew above the hunk) still applies by unique context.
+ writeTestFile(t, filepath.Join(root, "moved.txt"), "header\nhello\nold\n")
+ patch = strings.Join([]string{"--- a/moved.txt", "+++ b/moved.txt", "@@ -1,2 +1,2 @@", " hello", "-old", "+new", ""}, "\n")
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status != StatusOK {
+ t.Fatalf("offset hunk should apply by context, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "moved.txt")); string(content) != "header\nhello\nnew\n" {
+ t.Fatalf("content = %q", string(content))
+ }
+}
+
+func TestUnifiedPatchRenameWithHunkAndRejectsBinary(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "x.txt"), "hello\nold\n")
+ rename := strings.Join([]string{
+ "diff --git a/x.txt b/y.txt",
+ "similarity index 90%",
+ "rename from x.txt",
+ "rename to y.txt",
+ "--- a/x.txt",
+ "+++ b/y.txt",
+ "@@ -1,2 +1,2 @@",
+ " hello",
+ "-old",
+ "+new",
+ "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": rename})
+ if result.Status != StatusOK {
+ t.Fatalf("rename with hunk should apply, got %s: %s", result.Status, result.Output)
+ }
+ if _, err := os.Stat(filepath.Join(root, "x.txt")); !os.IsNotExist(err) {
+ t.Fatal("renamed source must be gone")
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "y.txt")); string(content) != "hello\nnew\n" {
+ t.Fatalf("renamed content = %q", string(content))
+ }
+ for name, patch := range map[string]string{
+ "binary": "diff --git a/x b/x\nBinary files a/x and b/x differ\n",
+ "empty": "diff --git a/x b/x\n",
+ } {
+ if result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch}); result.Status == StatusOK {
+ t.Fatalf("%s patch must be rejected", name)
+ }
+ }
+}
+
+func TestUnifiedPatchCopyOperation(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "src.txt"), "hello\nold\n")
+ copyPatch := func(from, to string, hunk bool) string {
+ lines := []string{"diff --git a/" + from + " b/" + to, "similarity index 90%", "copy from " + from, "copy to " + to}
+ if hunk {
+ lines = append(lines, "--- a/"+from, "+++ b/"+to, "@@ -1,2 +1,2 @@", " hello", "-old", "+new")
+ }
+ return strings.Join(append(lines, ""), "\n")
+ }
+
+ // Success: source kept, destination created with the hunk applied, and the
+ // destination inherits the source's tracker state (read whole -> whole).
+ tracker := NewFileTracker()
+ registry := NewRegistry()
+ registry.Register(NewScopedReadFileTool(root, nil))
+ registry.Register(NewScopedApplyPatchTool(root, nil))
+ opts := RunOptions{PermissionGranted: true, FileTracker: tracker}
+ if r := registry.RunWithOptions(context.Background(), "read_file", map[string]any{"path": "src.txt"}, opts); r.Status != StatusOK {
+ t.Fatalf("read: %s", r.Output)
+ }
+ result := registry.RunWithOptions(context.Background(), "apply_patch", map[string]any{"patch": copyPatch("src.txt", "dst.txt", true)}, opts)
+ if result.Status != StatusOK {
+ t.Fatalf("copy with hunk should apply, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "src.txt")); string(content) != "hello\nold\n" {
+ t.Fatalf("copy must keep the source, got %q", string(content))
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "dst.txt")); string(content) != "hello\nnew\n" {
+ t.Fatalf("copy destination = %q", string(content))
+ }
+ if len(result.ChangedFiles) != 2 || result.ChangedFiles[0] != "src.txt" || result.ChangedFiles[1] != "dst.txt" {
+ t.Fatalf("changed files = %v", result.ChangedFiles)
+ }
+ destination, err := filepath.EvalSymlinks(filepath.Join(root, "dst.txt"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !tracker.SeenWhole(destination) {
+ t.Fatal("destination copied from a fully read source must be tracked as seen whole")
+ }
+ unreadSource, _ := filepath.EvalSymlinks(filepath.Join(root, "src.txt"))
+ if _, tracked := tracker.Version(unreadSource); !tracked {
+ t.Fatal("source must stay tracked after a copy")
+ }
+
+ // Failure: destination already exists — nothing is written.
+ writeTestFile(t, filepath.Join(root, "taken.txt"), "keep\n")
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": copyPatch("src.txt", "taken.txt", false)})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "already exists") {
+ t.Fatalf("copy onto an existing destination must be refused, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "taken.txt")); string(content) != "keep\n" {
+ t.Fatal("existing destination must be untouched")
+ }
+
+ // Failure: copy onto itself.
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": copyPatch("src.txt", "src.txt", false)})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "onto itself") {
+ t.Fatalf("copy onto itself must be refused, got %s: %s", result.Status, result.Output)
+ }
+}
+
+func TestUnifiedPatchRejectsOverDeclaredHunkCounts(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "a.txt"), "hello\nold\n")
+ writeTestFile(t, filepath.Join(root, "b.txt"), "x\n")
+ // "-1,3 +1,3" over-declares a two-line hunk, so without a guard the next
+ // file's ---/+++ headers would be swallowed as "-"/"+" content.
+ patch := strings.Join([]string{
+ "--- a/a.txt", "+++ b/a.txt", "@@ -1,3 +1,3 @@", " hello", "-old", "+new",
+ "--- a/b.txt", "+++ b/b.txt", "@@ -1 +1 @@", "-x", "+y", "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "declared line counts") {
+ t.Fatalf("over-declared hunk must be reported as malformed, got %s: %s", result.Status, result.Output)
+ }
+ for name, want := range map[string]string{"a.txt": "hello\nold\n", "b.txt": "x\n"} {
+ if content, _ := os.ReadFile(filepath.Join(root, name)); string(content) != want {
+ t.Fatalf("%s must be untouched after a malformed patch, got %q", name, string(content))
+ }
+ }
+ // Truncated at end of input is reported the same way.
+ truncated := "--- a/a.txt\n+++ b/a.txt\n@@ -1,2 +1,2 @@\n hello\n-old\n"
+ if result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": truncated}); result.Status == StatusOK || !strings.Contains(result.Output, "declared line counts") {
+ t.Fatalf("truncated hunk must be reported as malformed, got %s: %s", result.Status, result.Output)
+ }
+}
+
+func TestUnifiedPatchNoNewlineMarkerOnlyAfterFinalHunk(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "two.txt"), "a\nb\nc\nd\ne\nf\n")
+ // A marker after the first of two hunks describes nothing real and must
+ // not leak into the second hunk's result: the patch is rejected untouched.
+ early := strings.Join([]string{
+ "--- a/two.txt", "+++ b/two.txt",
+ "@@ -1,2 +1,2 @@", " a", "-b", "+B", "\\ No newline at end of file",
+ "@@ -5,2 +5,2 @@", " e", "-f", "+F", "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": early})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "must follow the last hunk") {
+ t.Fatalf("marker before a later hunk must be rejected, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "two.txt")); string(content) != "a\nb\nc\nd\ne\nf\n" {
+ t.Fatal("rejected patch must not modify the file")
+ }
+ // The same marker after the final hunk applies and strips the newline.
+ final := strings.Join([]string{
+ "--- a/two.txt", "+++ b/two.txt",
+ "@@ -1,2 +1,2 @@", " a", "-b", "+B",
+ "@@ -5,2 +5,2 @@", " e", "-f", "+F", "\\ No newline at end of file", "",
+ }, "\n")
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": final})
+ if result.Status != StatusOK {
+ t.Fatalf("two-hunk patch with a final marker should apply, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "two.txt")); string(content) != "a\nB\nc\nd\ne\nF" {
+ t.Fatalf("content = %q", string(content))
+ }
+}
+
+// A removed "-- …" line directly followed by an added "++ …" line looks like a
+// ---/+++ header pair; the boundary check must not mistake it for one.
+func TestUnifiedPatchKeepsAdjacentDashPlusContentLines(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "notes.sql"), "select 1;\n-- old comment\nselect 2;\n")
+ patch := strings.Join([]string{
+ "--- a/notes.sql", "+++ b/notes.sql",
+ "@@ -1,3 +1,3 @@", " select 1;", "--- old comment", "+++ new comment", " select 2;", "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status != StatusOK {
+ t.Fatalf("adjacent -- / ++ content lines must apply, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "notes.sql")); string(content) != "select 1;\n++ new comment\nselect 2;\n" {
+ t.Fatalf("content = %q", string(content))
+ }
+}
+
+// A unified deletion states the content it removes; it must be verified
+// against the current file before anything is deleted.
+func TestUnifiedPatchDeletionVerifiesExpectedContent(t *testing.T) {
+ deletion := func(path string, hunks ...string) string {
+ lines := []string{"diff --git a/" + path + " b/" + path, "deleted file mode 100644", "--- a/" + path, "+++ /dev/null"}
+ return strings.Join(append(append(lines, hunks...), ""), "\n")
+ }
+ t.Run("matching deletion succeeds", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "current\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": deletion("gone.txt", "@@ -1 +0,0 @@", "-current")})
+ if result.Status != StatusOK {
+ t.Fatalf("matching deletion should apply, got %s: %s", result.Status, result.Output)
+ }
+ if _, err := os.Stat(filepath.Join(root, "gone.txt")); !os.IsNotExist(err) {
+ t.Fatal("file must be deleted")
+ }
+ })
+ t.Run("stale deletion is refused untouched", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "current\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": deletion("gone.txt", "@@ -1 +0,0 @@", "-stale")})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "does not match its current content") {
+ t.Fatalf("stale deletion must be refused, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "gone.txt")); string(content) != "current\n" {
+ t.Fatalf("file must be byte-for-byte unchanged, got %q", string(content))
+ }
+ })
+ t.Run("partial deletion is refused", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "one\ntwo\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": deletion("gone.txt", "@@ -1 +0,0 @@", "-one")})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "does not match its current content") {
+ t.Fatalf("deletion that does not cover the file must be refused, got %s: %s", result.Status, result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "gone.txt")); string(content) != "one\ntwo\n" {
+ t.Fatal("file must be unchanged")
+ }
+ })
+ t.Run("header-only deletion is refused", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "current\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": deletion("gone.txt")})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "must include the hunk") {
+ t.Fatalf("header-only deletion must be refused, got %s: %s", result.Status, result.Output)
+ }
+ if _, err := os.Stat(filepath.Join(root, "gone.txt")); err != nil {
+ t.Fatal("file must still exist")
+ }
+ })
+ t.Run("multi-hunk deletion verifies everything before removing", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "a\nb\nc\nd\n")
+ good := deletion("gone.txt", "@@ -1,2 +0,0 @@", "-a", "-b", "@@ -3,2 +0,0 @@", "-c", "-d")
+ bad := deletion("gone.txt", "@@ -1,2 +0,0 @@", "-a", "-b", "@@ -3,2 +0,0 @@", "-c", "-stale")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": bad})
+ if result.Status == StatusOK {
+ t.Fatalf("deletion with one stale hunk must be refused: %s", result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "gone.txt")); string(content) != "a\nb\nc\nd\n" {
+ t.Fatal("no hunk may be applied when any hunk is stale")
+ }
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": good})
+ if result.Status != StatusOK {
+ t.Fatalf("multi-hunk deletion should apply, got %s: %s", result.Status, result.Output)
+ }
+ if _, err := os.Stat(filepath.Join(root, "gone.txt")); !os.IsNotExist(err) {
+ t.Fatal("file must be deleted")
+ }
+ })
+ t.Run("structured delete stays unconditional", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "anything\n")
+ patch := "*** Begin Patch\n*** Delete File: gone.txt\n*** End Patch\n"
+ if result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch}); result.Status != StatusOK {
+ t.Fatalf("structured delete should apply, got %s: %s", result.Status, result.Output)
+ }
+ })
+}
+
+// The source is re-read through the root immediately before commit, so a
+// change made after planning is refused instead of overwritten or removed.
+func TestApplyStructuredPatchChangeRefusesSourceChangedAfterPlanning(t *testing.T) {
+ root := t.TempDir()
+ path := filepath.Join(root, "file.txt")
+ writeTestFile(t, path, "planned\n")
+ workspace, err := os.OpenRoot(root)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer workspace.Close()
+ target := structuredPatchTarget{requested: "file.txt", absolute: path, relative: "file.txt"}
+ destination := structuredPatchTarget{requested: "copy.txt", absolute: filepath.Join(root, "copy.txt"), relative: "copy.txt"}
+ for name, change := range map[string]structuredPatchChange{
+ "update": {kind: structuredPatchUpdate, from: target, to: target, before: "planned\n", after: "new\n", mode: 0o644},
+ "delete": {kind: structuredPatchDelete, from: target, to: target, before: "planned\n", mode: 0o644},
+ "copy": {kind: structuredPatchCopy, from: target, to: destination, before: "planned\n", after: "planned\n", mode: 0o644},
+ } {
+ writeTestFile(t, path, "changed after planning\n")
+ committed, err := applyStructuredPatchChange(workspace, change)
+ if err == nil || committed || !strings.Contains(err.Error(), "changed on disk between planning and commit") {
+ t.Fatalf("%s: expected a refusal, got committed=%v err=%v", name, committed, err)
+ }
+ if content, _ := os.ReadFile(path); string(content) != "changed after planning\n" {
+ t.Fatalf("%s: file must be untouched, got %q", name, string(content))
+ }
+ if _, statErr := os.Stat(filepath.Join(root, "copy.txt")); !os.IsNotExist(statErr) {
+ t.Fatalf("%s: refused change must not create copy.txt", name)
+ }
+ }
+}
+
+func TestUnifiedPatchDeletionIsByteExact(t *testing.T) {
+ deletion := func(hunks ...string) string {
+ return strings.Join(append(append([]string{"--- a/gone.txt", "+++ /dev/null"}, hunks...), ""), "\n")
+ }
+ for name, tc := range map[string]struct{ file, patch string }{
+ "trailing whitespace differs": {"current \n", deletion("@@ -1 +0,0 @@", "-current")},
+ "indentation differs": {" current\n", deletion("@@ -1 +0,0 @@", "-current")},
+ "blank line not covered": {"current\n\n", deletion("@@ -1 +0,0 @@", "-current")},
+ "space-only line not covered": {"current\n \n", deletion("@@ -1 +0,0 @@", "-current")},
+ "missing final newline": {"current", deletion("@@ -1 +0,0 @@", "-current")},
+ } {
+ t.Run(name, func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), tc.file)
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": tc.patch})
+ if result.Status == StatusOK {
+ t.Fatalf("deletion must be byte-exact; %q was deleted by a non-matching hunk", tc.file)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "gone.txt")); string(content) != tc.file {
+ t.Fatalf("file must be unchanged, got %q", string(content))
+ }
+ })
+ }
+ t.Run("no-newline marker on the removed side matches exactly", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "current")
+ patch := deletion("@@ -1 +0,0 @@", "-current", "\\ No newline at end of file")
+ if result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch}); result.Status != StatusOK {
+ t.Fatalf("deletion with a matching no-newline marker should apply, got %s: %s", result.Status, result.Output)
+ }
+ if _, err := os.Stat(filepath.Join(root, "gone.txt")); !os.IsNotExist(err) {
+ t.Fatal("file must be deleted")
+ }
+ })
+ t.Run("CRLF file matches an LF patch of the same content", func(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "gone.txt"), "a\r\nb\r\n")
+ if result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": deletion("@@ -1,2 +0,0 @@", "-a", "-b")}); result.Status != StatusOK {
+ t.Fatalf("CRLF deletion should apply, got %s: %s", result.Status, result.Output)
+ }
+ })
+}
+
+// git describes an empty file's creation or deletion with header lines only.
+func TestUnifiedPatchHeaderOnlyEmptyFileForms(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "empty.txt"), "")
+ writeTestFile(t, filepath.Join(root, "full.txt"), "content\n")
+ writeTestFile(t, filepath.Join(root, "keep.txt"), "a\n")
+ deleteHeader := func(path string) string {
+ return "diff --git a/" + path + " b/" + path + "\ndeleted file mode 100644\nindex e69de29..0000000\n"
+ }
+ // A non-empty file is not deleted by the header-only form.
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": deleteHeader("full.txt")})
+ if result.Status == StatusOK || !strings.Contains(result.Output, "expects an empty file") {
+ t.Fatalf("header-only deletion of a non-empty file must be refused, got %s: %s", result.Status, result.Output)
+ }
+ if _, err := os.Stat(filepath.Join(root, "full.txt")); err != nil {
+ t.Fatal("non-empty file must still exist")
+ }
+ // In a multi-file patch the empty-file deletion and creation are applied
+ // alongside a normal hunk.
+ patch := deleteHeader("empty.txt") +
+ "diff --git a/blank.txt b/blank.txt\nnew file mode 100644\nindex 0000000..e69de29\n" +
+ "diff --git a/keep.txt b/keep.txt\n--- a/keep.txt\n+++ b/keep.txt\n@@ -1 +1 @@\n-a\n+b\n"
+ result = NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status != StatusOK {
+ t.Fatalf("multi-file patch with header-only forms should apply, got %s: %s", result.Status, result.Output)
+ }
+ if _, err := os.Stat(filepath.Join(root, "empty.txt")); !os.IsNotExist(err) {
+ t.Fatal("empty file must be deleted")
+ }
+ if content, err := os.ReadFile(filepath.Join(root, "blank.txt")); err != nil || len(content) != 0 {
+ t.Fatalf("empty file must be created, got err=%v content=%q", err, string(content))
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "keep.txt")); string(content) != "b\n" {
+ t.Fatalf("hunk file = %q", string(content))
+ }
+ if len(result.ChangedFiles) != 3 {
+ t.Fatalf("changed files = %v", result.ChangedFiles)
+ }
+}
+
+// When a later change fails, the error names exactly which files were already
+// committed so the caller knows what changed and what did not.
+func TestApplyPatchOperationsReportsCommittedPrefixOnFailure(t *testing.T) {
+ root := t.TempDir()
+ writeTestFile(t, filepath.Join(root, "first.txt"), "one\n")
+ writeTestFile(t, filepath.Join(root, "second.txt"), "two\n")
+ writeTestFile(t, filepath.Join(root, "third.txt"), "three\n")
+ // Make second.txt change after planning so its commit is refused.
+ structuredPatchBeforeCommit = func(change structuredPatchChange) {
+ if change.to.relative == "second.txt" {
+ writeTestFile(t, filepath.Join(root, "second.txt"), "changed\n")
+ }
+ }
+ defer func() { structuredPatchBeforeCommit = nil }()
+ patch := strings.Join([]string{
+ "*** Begin Patch",
+ "*** Update File: first.txt", "@@", "-one", "+ONE",
+ "*** Update File: second.txt", "@@", "-two", "+TWO",
+ "*** Update File: third.txt", "@@", "-three", "+THREE",
+ "*** End Patch", "",
+ }, "\n")
+ result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch})
+ if result.Status == StatusOK {
+ t.Fatal("patch must fail when a later change is refused")
+ }
+ for _, want := range []string{"already committed: first.txt", "remaining files are unchanged"} {
+ if !strings.Contains(result.Output, want) {
+ t.Fatalf("error must report the committed prefix, got: %s", result.Output)
+ }
+ }
+ if strings.Contains(result.Output, "committed: first.txt, second.txt") || strings.Contains(result.Output, "third.txt") {
+ t.Fatalf("error must not list uncommitted files as committed: %s", result.Output)
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "first.txt")); string(content) != "ONE\n" {
+ t.Fatalf("first.txt must hold the committed change, got %q", string(content))
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "second.txt")); string(content) != "changed\n" {
+ t.Fatalf("second.txt must be untouched by the patch, got %q", string(content))
+ }
+ if content, _ := os.ReadFile(filepath.Join(root, "third.txt")); string(content) != "three\n" {
+ t.Fatalf("third.txt must be untouched, got %q", string(content))
+ }
+}
diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go
index a5b0ac308..db4a6f045 100644
--- a/internal/tools/edit_file.go
+++ b/internal/tools/edit_file.go
@@ -168,7 +168,7 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any
options.FileTracker.Record(absolutePath, []byte(updated), newInfo)
if updated == modelKnownContent {
if previouslySeenWhole {
- options.FileTracker.RecordSeenRange(absolutePath, 1, lineCount(updated), lineCount(updated))
+ options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(updated), trackedLineTotal(updated))
} else {
for _, span := range editedSpans {
options.FileTracker.RecordSeenBytes(absolutePath, span.start, span.end, len(updated))
diff --git a/internal/tools/file_tools_test.go b/internal/tools/file_tools_test.go
index df6b11edc..92dc7325a 100644
--- a/internal/tools/file_tools_test.go
+++ b/internal/tools/file_tools_test.go
@@ -26,8 +26,8 @@ func TestReadFileToolReadsLineRanges(t *testing.T) {
}
for _, want := range []string{
"File: notes.txt (lines 2-3 of 4)",
- "2 | beta",
- "3 | gamma",
+ "2→beta",
+ "3→gamma",
} {
if !strings.Contains(result.Output, want) {
t.Fatalf("expected output to contain %q, got %q", want, result.Output)
@@ -54,7 +54,7 @@ func TestReadFileToolReadsCanonicalLineRange(t *testing.T) {
if result.Truncated {
t.Fatalf("canonical limit defines the requested range and must not mark it truncated: %#v", result.Meta)
}
- if !strings.Contains(result.Output, "2 | beta") || !strings.Contains(result.Output, "3 | gamma") {
+ if !strings.Contains(result.Output, "2→beta") || !strings.Contains(result.Output, "3→gamma") {
t.Fatalf("canonical range returned the wrong lines: %q", result.Output)
}
if strings.Contains(result.Output, "alpha") || strings.Contains(result.Output, "delta") {
@@ -94,7 +94,7 @@ func TestReadFileToolMixedLegacyRangesPreferLines(t *testing.T) {
if result.Status != StatusOK {
t.Fatalf("mixed legacy range should recover, got %s: %s", result.Status, result.Output)
}
- if !strings.Contains(result.Output, "2 | beta") || !strings.Contains(result.Output, "3 | gamma") {
+ if !strings.Contains(result.Output, "2→beta") || !strings.Contains(result.Output, "3→gamma") {
t.Fatalf("mixed legacy range did not prefer lines: %q", result.Output)
}
}
@@ -110,7 +110,7 @@ func TestReadFileToolCombinesLegacyStartWithCanonicalLimit(t *testing.T) {
if result.Status != StatusOK || result.Truncated {
t.Fatalf("mixed compatible range should be exact, got status=%s truncated=%v: %s", result.Status, result.Truncated, result.Output)
}
- if !strings.Contains(result.Output, "2 | beta") || !strings.Contains(result.Output, "3 | gamma") {
+ if !strings.Contains(result.Output, "2→beta") || !strings.Contains(result.Output, "3→gamma") {
t.Fatalf("mixed compatible range returned the wrong lines: %q", result.Output)
}
if strings.Contains(result.Output, "alpha") || strings.Contains(result.Output, "delta") {
@@ -955,7 +955,7 @@ func TestReadFileToolRecoversBackwardsRange(t *testing.T) {
for _, want := range []string{
"end_line 2 was before start_line 3", // the note
"only line 3 was read",
- "3 | gamma",
+ "3→gamma",
} {
if !strings.Contains(result.Output, want) {
t.Fatalf("expected %q in output, got %q", want, result.Output)
@@ -984,7 +984,7 @@ func TestReadFileToolValidRangeUnchangedByRecovery(t *testing.T) {
// Byte-for-byte: header, blank separator, the exact selected lines, and no
// recovery note or trailing content. An altered header/separator or a stray
// out-of-range line would fail this where a substring check would not.
- const want = "File: notes.txt (lines 2-4 of 5)\n\n2 | beta\n3 | gamma\n4 | delta"
+ const want = "File: notes.txt (lines 2-4 of 5)\n\n2→beta\n3→gamma\n4→delta"
if result.Output != want {
t.Fatalf("valid-range output changed by the recovery path:\n got: %q\nwant: %q", result.Output, want)
}
diff --git a/internal/tools/model_visibility.go b/internal/tools/model_visibility.go
index e942e10b8..24820c05e 100644
--- a/internal/tools/model_visibility.go
+++ b/internal/tools/model_visibility.go
@@ -1,9 +1,11 @@
package tools
// ModelVisible reports whether a registered tool is advertised to an agent.
-// edit_file remains implemented for compatibility with existing integrations,
-// but its text-only replacement contract cannot safely disambiguate repeated
-// source fragments. Agents use apply_patch for contextual existing-file edits.
+// Every registered tool is visible. edit_file (exact search/replace with a
+// uniqueness guard, CRLF and fuzzy fallbacks) was hidden for a while in favour
+// of apply_patch; head-to-head benchmarking showed that when a model misses the
+// patch grammar it degrades to whole-file rewrites, so both edit tools are
+// advertised and the prompt steers between them.
func ModelVisible(tool Tool) bool {
- return tool != nil && tool.Name() != "edit_file"
+ return tool != nil
}
diff --git a/internal/tools/model_visibility_test.go b/internal/tools/model_visibility_test.go
index 22d79d8d3..7f2590dcf 100644
--- a/internal/tools/model_visibility_test.go
+++ b/internal/tools/model_visibility_test.go
@@ -2,17 +2,28 @@ package tools
import "testing"
-func TestToolSearchDoesNotSurfaceLegacyStringReplacement(t *testing.T) {
+func TestToolSearchSurfacesBothEditTools(t *testing.T) {
registry := NewRegistry()
registry.Register(NewScopedEditFileTool(t.TempDir(), nil))
registry.Register(NewScopedApplyPatchTool(t.TempDir(), nil))
search := NewToolSearchTool(registry).(toolSearchTool)
eager := search.visibleEagerToolNames(nil, nil, "ask")
- if eager["edit_file"] {
- t.Fatal("tool_search must not describe edit_file as available to the model")
+ if !eager["edit_file"] {
+ t.Fatal("tool_search must describe edit_file as available to the model")
}
if !eager["apply_patch"] {
- t.Fatal("tool_search must retain apply_patch as the available edit tool")
+ t.Fatal("tool_search must retain apply_patch as an available edit tool")
+ }
+}
+
+func TestModelVisibleAdvertisesEveryRegisteredTool(t *testing.T) {
+ if ModelVisible(nil) {
+ t.Fatal("a nil tool must not be visible")
+ }
+ for _, tool := range []Tool{NewScopedEditFileTool(t.TempDir(), nil), NewScopedApplyPatchTool(t.TempDir(), nil), NewScopedWriteFileTool(t.TempDir(), nil)} {
+ if !ModelVisible(tool) {
+ t.Fatalf("%s must be visible to the model", tool.Name())
+ }
}
}
diff --git a/internal/tools/read_file.go b/internal/tools/read_file.go
index b6ef1d9da..38339ad38 100644
--- a/internal/tools/read_file.go
+++ b/internal/tools/read_file.go
@@ -9,12 +9,14 @@ import (
"io"
"os"
"strconv"
- "strings"
"unicode/utf8"
)
const readFileByteChunkMax = 64 * 1024
+// readFileLinePrefixSeparator follows the line number on every read_file line.
+const readFileLinePrefixSeparator = "→"
+
type readFileTool struct {
baseTool
workspaceRoot string
@@ -31,7 +33,7 @@ func NewScopedReadFileTool(workspaceRoot string, scope PathScope) Tool {
return readFileTool{
baseTool: baseTool{
name: "read_file",
- description: "Read exact file text with line numbers. Use for comments, formatting, or edits; prefer read_minified_file for initial code understanding. Use offset and limit for a line range.",
+ description: "Read exact file text, each line prefixed with its line number and →. Use for comments, formatting, or edits; prefer read_minified_file for initial code understanding. Use offset and limit for a line range.",
parameters: Schema{
Type: "object",
Properties: map[string]PropertySchema{
@@ -221,7 +223,6 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
}
lastLine := startLine + selectedLines - 1
- width := len(strconv.Itoa(lastLine))
header := fmt.Sprintf("File: %s (%d lines)", relativePath, total)
if startLine != 1 || endLine != total || maxLines > 0 {
header = fmt.Sprintf("File: %s (lines %d-%d of %d)", relativePath, startLine, lastLine, total)
@@ -244,7 +245,7 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
budgetedOutput.WriteString("\n")
}
budgetedOutput.WriteString("\n")
- if err := appendReadFileRange(budgetedOutput, absolutePath, startLine, selectedLines, width); err != nil {
+ if err := appendReadFileRange(budgetedOutput, absolutePath, startLine, selectedLines); err != nil {
return errorResult("Error reading file " + relativePath + ": " + err.Error())
}
if truncated {
@@ -357,7 +358,7 @@ func renderReadFileBytes(path, relativePath string, total, requestedStart, limit
return Result{Status: StatusOK, Output: output, Meta: map[string]string{"next_byte_offset": strconv.Itoa(end)}}, start, end
}
-func appendReadFileRange(output *outputBudgetBuilder, path string, startLine int, selectedLines int, width int) error {
+func appendReadFileRange(output *outputBudgetBuilder, path string, startLine int, selectedLines int) error {
file, err := os.Open(path)
if err != nil {
return err
@@ -380,10 +381,11 @@ func appendReadFileRange(output *outputBudgetBuilder, path string, startLine int
if emitted > 0 {
output.WriteString("\n")
}
- number := strconv.Itoa(lineNumber)
- output.WriteString(strings.Repeat(" ", width-len(number)))
- output.WriteString(number)
- output.WriteString(" | ")
+ // Compact "N→" prefix: the padded " N | " form cost ~19% of every
+ // read (about 1.5k tokens per 800-line file) and was re-sent on each
+ // later call; no consumer parses it, and models read "N→" natively.
+ output.WriteString(strconv.Itoa(lineNumber))
+ output.WriteString(readFileLinePrefixSeparator)
output.WriteString(string(trimLineBreak(raw, ended)))
emitted++
}
diff --git a/internal/tools/read_minified_file.go b/internal/tools/read_minified_file.go
index a443acf84..8c9d9d15b 100644
--- a/internal/tools/read_minified_file.go
+++ b/internal/tools/read_minified_file.go
@@ -171,14 +171,9 @@ func selectSourceLines(content []byte, offset, limit int) sourceSelection {
}
func sourceLineCount(content []byte) int {
- if len(content) == 0 {
- return 1
- }
- lines := strings.Count(string(content), "\n")
- if content[len(content)-1] != '\n' {
- lines++
- }
- return lines
+ // One implementation for reader and writer: the tracker contract lives on
+ // trackedLineTotal, and this must never drift from it.
+ return trackedLineTotal(string(content))
}
// lineCount reports the number of newline-separated lines in s (an empty string
@@ -189,3 +184,20 @@ func lineCount(s string) int {
}
return strings.Count(s, "\n") + 1
}
+
+// trackedLineTotal reports a file's line count the way read_file reports it
+// to the FileTracker (newline-terminated lines, plus an unterminated last
+// line; an empty file is one line). Writers must record this same number:
+// RecordSeenRange resets every observation when the total changes, so a
+// writer that recorded lineCount (one higher for a trailing newline) made the
+// next partial read_file wipe whole-file knowledge and refuse the next edit.
+func trackedLineTotal(s string) int {
+ if s == "" {
+ return 1
+ }
+ total := strings.Count(s, "\n")
+ if !strings.HasSuffix(s, "\n") {
+ total++
+ }
+ return total
+}
diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go
index c4ca17359..653f0cbb5 100644
--- a/internal/tools/structured_patch.go
+++ b/internal/tools/structured_patch.go
@@ -6,9 +6,11 @@ import (
"io"
"os"
"path/filepath"
+ "regexp"
"strings"
"github.com/Gitlawb/zero/internal/pathjail"
+ "github.com/Gitlawb/zero/internal/sandbox"
)
const (
@@ -28,6 +30,9 @@ const (
structuredPatchAdd structuredPatchKind = iota
structuredPatchDelete
structuredPatchUpdate
+ // structuredPatchCopy creates movePath from path's content (plus any
+ // hunks) and keeps path; produced by a unified diff's "copy from/to".
+ structuredPatchCopy
)
type structuredPatchOperation struct {
@@ -37,7 +42,28 @@ type structuredPatchOperation struct {
contents string
chunks []structuredPatchChunk
line int
-}
+ // eofNewline lets a unified diff's "\ No newline at end of file" marker
+ // force the trailing-newline state of the result; structured patches keep
+ // the file's existing state.
+ eofNewline eofNewlineMode
+ // verifyDelete marks a deletion that carries the expected old content
+ // (a unified diff's "+++ /dev/null" hunks, or git's header-only form for
+ // an empty file): the removed lines must equal the current file byte for
+ // byte, otherwise the deletion is stale and refused. A structured
+ // "*** Delete File" means "delete this path" and has no chunks.
+ verifyDelete bool
+ // oldNoNewline records a unified diff's "\ No newline at end of file"
+ // on the removed side, i.e. the old content did not end with a newline.
+ oldNoNewline bool
+}
+
+type eofNewlineMode uint8
+
+const (
+ eofNewlineKeep eofNewlineMode = iota
+ eofNewlinePresent
+ eofNewlineAbsent
+)
type structuredPatchChunk struct {
context string
@@ -46,6 +72,11 @@ type structuredPatchChunk struct {
new []string
newSourceOffsets []int
endOfFile bool
+ // hint is the 0-based line the hunk is expected at (from a unified diff's
+ // "@@ -a,b" range). When the expected lines match there it is used as-is;
+ // otherwise the hunk is located by content like a structured hunk.
+ hint int
+ hasHint bool
}
type structuredPatchTarget struct {
@@ -63,8 +94,36 @@ type structuredPatchChange struct {
mode os.FileMode
}
+// unifiedHunkRangePattern recognises a unified-diff range header ("-12,4 +12,6",
+// optionally followed by "@@ heading") written inside a structured hunk marker.
+// The line numbers carry no information for the context-anchored grammar, so
+// the range is dropped and only an explicit heading is kept as the anchor.
+var unifiedHunkRangePattern = regexp.MustCompile(`^-\d+(?:,\d+)?\s+\+\d+(?:,\d+)?\s*(?:@@\s*(.*))?$`)
+
+const structuredPatchFormatHint = ` (format: "*** Begin Patch", then "*** Update File: path" / "*** Add File: path" / "*** Delete File: path" sections whose hunks start with "@@" and use " " context, "-" removed and "+" added lines, then "*** End Patch")`
+
+// structuredPatchMarker classifies a line as the "begin" or "end" marker of a
+// structured patch, or "" when it is neither. The classifier is owned by the
+// sandbox package so the boundary check and the tool accept exactly the same
+// spellings; a strict byte-equal match here once made every patch from some
+// models fail on line 1 and pushed them into whole-file rewrites.
+func structuredPatchMarker(line string) string {
+ return sandbox.StructuredPatchMarker(line)
+}
+
+// structuredHunkAnchor normalises the text after a hunk's "@@ " marker. A
+// unified-diff range is dropped (its optional heading survives as the anchor);
+// anything else is used verbatim.
+func structuredHunkAnchor(context string) string {
+ context = strings.TrimSpace(context)
+ if match := unifiedHunkRangePattern.FindStringSubmatch(context); match != nil {
+ return strings.TrimSpace(match[1])
+ }
+ return context
+}
+
func isStructuredPatch(patch string) bool {
- return strings.HasPrefix(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), structuredPatchBegin)
+ return sandbox.IsStructuredPatch(patch)
}
func (tool applyPatchTool) runStructuredPatch(applyRoot, relativeRoot, patch string, options RunOptions) Result {
@@ -72,6 +131,14 @@ func (tool applyPatchTool) runStructuredPatch(applyRoot, relativeRoot, patch str
if err != nil {
return errorResult("Error applying patch: " + err.Error())
}
+ return applyPatchOperations(applyRoot, relativeRoot, operations, options)
+}
+
+// applyPatchOperations applies parsed operations (from either patch format)
+// through an opened workspace root: every stat, read, create and write is
+// descriptor-relative and refuses to follow a link out of the root, so there
+// is no pathname check-to-use window between validation and write.
+func applyPatchOperations(applyRoot, relativeRoot string, operations []structuredPatchOperation, options RunOptions) Result {
workspace, err := os.OpenRoot(applyRoot)
if err != nil {
return errorResult("Error applying patch: " + err.Error())
@@ -84,7 +151,7 @@ func (tool applyPatchTool) runStructuredPatch(applyRoot, relativeRoot, patch str
wholeBefore := make(map[string]bool, len(changes))
if options.FileTracker != nil {
for _, change := range changes {
- if change.kind == structuredPatchUpdate {
+ if change.kind == structuredPatchUpdate || change.kind == structuredPatchCopy {
wholeBefore[change.from.absolute] = options.FileTracker.SeenWhole(change.from.absolute)
}
}
@@ -103,6 +170,10 @@ func (tool applyPatchTool) runStructuredPatch(applyRoot, relativeRoot, patch str
wasWhole := wholeBefore[change.from.absolute]
options.FileTracker.Forget(change.from.absolute)
recordStructuredPatchFile(options.FileTracker, change.to.absolute, false, wasWhole)
+ case structuredPatchCopy:
+ // The source is untouched; the destination inherits only what the
+ // model had actually seen of the source.
+ recordStructuredPatchFile(options.FileTracker, change.to.absolute, true, wholeBefore[change.from.absolute])
}
}
@@ -122,11 +193,11 @@ func parseStructuredPatch(patch string) ([]structuredPatchOperation, error) {
return nil, fmt.Errorf("structured patch is empty")
}
lines := strings.Split(normalized, "\n")
- if strings.TrimSpace(lines[0]) != structuredPatchBegin {
- return nil, fmt.Errorf("the first line of a structured patch must be %q", structuredPatchBegin)
+ if structuredPatchMarker(lines[0]) != "begin" {
+ return nil, fmt.Errorf("the first line of a structured patch must be %q%s", structuredPatchBegin, structuredPatchFormatHint)
}
- if strings.TrimSpace(lines[len(lines)-1]) != structuredPatchEnd {
- return nil, fmt.Errorf("the last line of a structured patch must be %q", structuredPatchEnd)
+ if structuredPatchMarker(lines[len(lines)-1]) != "end" {
+ return nil, fmt.Errorf("the last line of a structured patch must be %q%s", structuredPatchEnd, structuredPatchFormatHint)
}
var operations []structuredPatchOperation
@@ -191,8 +262,12 @@ func parseStructuredPatch(patch string) ([]structuredPatchOperation, error) {
}
if trimmed == "@@" || strings.HasPrefix(trimmed, structuredHunkContext) {
chunk := structuredPatchChunk{}
+ rest := ""
if trimmed != "@@" {
- chunk.context = strings.TrimPrefix(trimmed, structuredHunkContext)
+ rest = strings.TrimPrefix(trimmed, structuredHunkContext)
+ }
+ if anchor := structuredHunkAnchor(rest); anchor != "" {
+ chunk.context = anchor
chunk.hasContext = true
}
op.chunks = append(op.chunks, chunk)
@@ -248,7 +323,7 @@ func structuredPatchPath(header, prefix string, line int) (string, error) {
}
func isStructuredPatchHeader(line string) bool {
- return line == structuredPatchEnd || strings.HasPrefix(line, structuredAddFile) || strings.HasPrefix(line, structuredDeleteFile) || strings.HasPrefix(line, structuredUpdateFile)
+ return structuredPatchMarker(line) == "end" || strings.HasPrefix(line, structuredAddFile) || strings.HasPrefix(line, structuredDeleteFile) || strings.HasPrefix(line, structuredUpdateFile)
}
func structuredPatchLineError(line int, message string) error {
@@ -318,7 +393,7 @@ func planStructuredPatch(root *os.Root, operations []structuredPatchOperation, t
return nil, err
}
change.after = operation.contents
- case structuredPatchDelete, structuredPatchUpdate:
+ case structuredPatchDelete, structuredPatchUpdate, structuredPatchCopy:
info, err := root.Stat(from.relative)
if err != nil {
return nil, fmt.Errorf("stating %s: %w", from.relative, err)
@@ -332,12 +407,20 @@ func planStructuredPatch(root *os.Root, operations []structuredPatchOperation, t
return nil, fmt.Errorf("%s", fileConflictMessage(from.relative))
}
change.before = string(content)
- if operation.kind == structuredPatchUpdate {
+ if operation.kind == structuredPatchDelete && operation.verifyDelete {
+ if err := verifyUnifiedDeletion(change.before, from.relative, operation); err != nil {
+ return nil, err
+ }
+ }
+ if operation.kind == structuredPatchUpdate || operation.kind == structuredPatchCopy {
updated, err := applyStructuredPatchUpdate(change.before, from.relative, operation.chunks)
if err != nil {
return nil, err
}
- change.after = updated
+ change.after = applyEOFNewline(updated, operation.eofNewline)
+ if operation.kind == structuredPatchCopy && from.absolute == to.absolute {
+ return nil, fmt.Errorf("cannot copy %s onto itself", from.relative)
+ }
if from.absolute != to.absolute {
if _, err := root.Lstat(to.relative); err == nil {
return nil, fmt.Errorf("cannot move %s to %s because the destination already exists", from.relative, to.relative)
@@ -353,10 +436,14 @@ func planStructuredPatch(root *os.Root, operations []structuredPatchOperation, t
}
func resolveStructuredPatchTarget(root, path string) (structuredPatchTarget, error) {
- if filepath.IsAbs(path) || path == ".." || strings.HasPrefix(filepath.ToSlash(path), "../") {
+ // Relative traversal is rejected outright. An absolute path is allowed when
+ // it resolves inside the apply root (models routinely echo the absolute path
+ // they were shown by read_file); resolveWorkspaceTargetPath rejects anything
+ // that lands outside.
+ if path == ".." || strings.HasPrefix(filepath.ToSlash(path), "../") {
return structuredPatchTarget{}, fmt.Errorf("patch path %q must stay inside the workspace", path)
}
- absolute, relative, err := resolveWorkspaceTargetPath(root, path)
+ absolute, relative, err := resolveWorkspaceTargetPath(root, normalizePatchPathForRoot(root, path))
if err != nil {
return structuredPatchTarget{}, err
}
@@ -389,9 +476,15 @@ func applyStructuredPatchUpdate(content, path string, chunks []structuredPatchCh
if len(chunk.old) == 0 {
// A context anchor identifies the line immediately before a pure
// insertion. Only an explicit end-of-file hunk (or an unanchored
- // insertion) belongs at the file end.
+ // insertion) belongs at the file end. A unified diff names the
+ // insertion point in its range instead; a zero-context hunk (as
+ // from `git diff -U0`) has nothing to verify the position against,
+ // so it is position-trusting by design — the same semantics git
+ // apply uses without fuzz. Hunks that carry context are verified.
start := lineIndex
- if chunk.endOfFile || !chunk.hasContext {
+ if chunk.hasHint {
+ start = max(chunk.hint, lineIndex)
+ } else if chunk.endOfFile || !chunk.hasContext {
start = len(lines)
}
if start > len(lines) {
@@ -405,7 +498,13 @@ func applyStructuredPatchUpdate(content, path string, chunks []structuredPatchCh
lineIndex = start
continue
}
- index, ambiguous := findStructuredPatchSequence(lines, chunk.old, lineIndex, chunk.endOfFile)
+ var index int
+ ambiguous := false
+ if chunk.hasHint && chunk.hint >= lineIndex && sequenceMatchesAt(lines, chunk.old, chunk.hint) {
+ index = chunk.hint
+ } else {
+ index, ambiguous = findStructuredPatchSequence(lines, chunk.old, lineIndex, chunk.endOfFile)
+ }
if ambiguous {
return "", fmt.Errorf("expected lines are ambiguous in %s; provide more surrounding context:\n%s", path, strings.Join(chunk.old, "\n"))
}
@@ -440,6 +539,64 @@ func applyStructuredPatchUpdate(content, path string, chunks []structuredPatchCh
return updated, nil
}
+// verifyUnifiedDeletion checks, byte for byte, that the lines a unified
+// deletion removes are exactly the file's current content. A deletion is not
+// recoverable, so none of the whitespace tolerance used to locate update
+// hunks applies here; only the file's own line-ending style is normalised.
+// With no hunks (git's header-only form) the file must be empty.
+func verifyUnifiedDeletion(current, path string, operation structuredPatchOperation) error {
+ var removed []string
+ for _, chunk := range operation.chunks {
+ if len(chunk.new) > 0 {
+ return fmt.Errorf("deletion of %s must not add lines", path)
+ }
+ removed = append(removed, chunk.old...)
+ }
+ expected := strings.Join(removed, "\n")
+ if len(removed) > 0 && !operation.oldNoNewline {
+ expected += "\n"
+ }
+ actual := current
+ if structuredPatchLineEnding(current) == "\r\n" {
+ actual = strings.ReplaceAll(current, "\r\n", "\n")
+ }
+ if actual != expected {
+ if len(removed) == 0 {
+ return fmt.Errorf("deletion of %s expects an empty file but it has content; include the removed lines in the patch", path)
+ }
+ return fmt.Errorf("deletion of %s does not match its current content; the removed lines must equal the whole file", path)
+ }
+ return nil
+}
+
+// sequenceMatchesAt reports whether wanted appears verbatim at lines[start:].
+func sequenceMatchesAt(lines, wanted []string, start int) bool {
+ if start < 0 || start+len(wanted) > len(lines) {
+ return false
+ }
+ for offset, line := range wanted {
+ if lines[start+offset] != line {
+ return false
+ }
+ }
+ return true
+}
+
+// applyEOFNewline forces or strips a single trailing line ending when a
+// unified diff's end-of-file marker asked for it.
+func applyEOFNewline(content string, mode eofNewlineMode) string {
+ switch mode {
+ case eofNewlinePresent:
+ if content != "" && !strings.HasSuffix(content, "\n") {
+ return content + structuredPatchLineEnding(content)
+ }
+ case eofNewlineAbsent:
+ content = strings.TrimSuffix(content, "\n")
+ content = strings.TrimSuffix(content, "\r")
+ }
+ return content
+}
+
func structuredPatchReplacement(chunk structuredPatchChunk, source []string, start int) ([]string, error) {
if len(chunk.newSourceOffsets) != len(chunk.new) {
return nil, fmt.Errorf("context mapping has %d entries for %d output lines", len(chunk.newSourceOffsets), len(chunk.new))
@@ -516,14 +673,19 @@ func findStructuredPatchSequence(lines, wanted []string, start int, endOfFile bo
}
func applyStructuredPatchChanges(root *os.Root, changes []structuredPatchChange, tracker *FileTracker) error {
- mutated := false
+ // committed lists, in order, the paths whose change reached disk before a
+ // later change failed, so the caller (and the model) knows exactly which
+ // files now hold the patched content and which were never touched.
+ var committed []string
for _, change := range changes {
- committed, err := applyStructuredPatchChange(root, change)
- mutated = mutated || committed
+ done, err := applyStructuredPatchChange(root, change)
+ if done {
+ committed = append(committed, structuredPatchChangePaths(change)...)
+ }
if err != nil {
forgetStructuredPatchFiles(tracker, changes)
- if mutated {
- return fmt.Errorf("%w; structured patch was partially applied; re-read affected files before retrying", err)
+ if len(committed) > 0 {
+ return fmt.Errorf("%w; patch was partially applied — already committed: %s; the remaining files are unchanged; re-read the committed files before retrying", err, strings.Join(committed, ", "))
}
return err
}
@@ -531,6 +693,19 @@ func applyStructuredPatchChanges(root *os.Root, changes []structuredPatchChange,
return nil
}
+// structuredPatchChangePaths names the workspace-relative paths a committed
+// change touched: the destination, plus the source of a move or copy.
+func structuredPatchChangePaths(change structuredPatchChange) []string {
+ if change.kind == structuredPatchDelete {
+ return []string{change.from.relative}
+ }
+ paths := []string{change.to.relative}
+ if change.from.absolute != change.to.absolute && change.from.relative != "" {
+ paths = append([]string{change.from.relative}, paths...)
+ }
+ return paths
+}
+
func forgetStructuredPatchFiles(tracker *FileTracker, changes []structuredPatchChange) {
if tracker == nil {
return
@@ -543,14 +718,35 @@ func forgetStructuredPatchFiles(tracker *FileTracker, changes []structuredPatchC
}
}
+// structuredPatchBeforeCommit, when set, runs before each change's pre-commit
+// recheck. Tests use it to alter the filesystem between planning and commit
+// deterministically; it is nil in production.
+var structuredPatchBeforeCommit func(change structuredPatchChange)
+
func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bool, error) {
+ if structuredPatchBeforeCommit != nil {
+ structuredPatchBeforeCommit(change)
+ }
+ // The planner validated the source against the bytes it read; re-read
+ // through the same root immediately before committing so a change made
+ // by another process in between is refused rather than overwritten or
+ // removed.
+ if change.kind != structuredPatchAdd {
+ current, err := root.ReadFile(change.from.relative)
+ if err != nil {
+ return false, fmt.Errorf("re-reading %s before commit: %w", change.from.relative, err)
+ }
+ if string(current) != change.before {
+ return false, fmt.Errorf("%s changed on disk between planning and commit; re-read it and retry", change.from.relative)
+ }
+ }
switch change.kind {
case structuredPatchDelete:
if err := root.Remove(change.from.relative); err != nil {
return false, fmt.Errorf("deleting %s: %w", change.from.relative, err)
}
return true, nil
- case structuredPatchAdd:
+ case structuredPatchAdd, structuredPatchCopy:
return writeStructuredPatchFile(root, change.to, change.after, change.mode, true)
case structuredPatchUpdate:
moving := change.from.absolute != change.to.absolute
@@ -706,7 +902,7 @@ func recordStructuredPatchFile(tracker *FileTracker, absolute string, created, s
info, _ := os.Stat(absolute)
tracker.Record(absolute, content, info)
if seenWhole {
- lines := lineCount(string(content))
+ lines := trackedLineTotal(string(content))
tracker.RecordSeenRange(absolute, 1, lines, lines)
}
if created {
diff --git a/internal/tools/tracked_line_total_test.go b/internal/tools/tracked_line_total_test.go
new file mode 100644
index 000000000..59491b17e
--- /dev/null
+++ b/internal/tools/tracked_line_total_test.go
@@ -0,0 +1,59 @@
+package tools
+
+import (
+ "context"
+ "fmt"
+ "path/filepath"
+ "testing"
+)
+
+func TestTrackedLineTotalMatchesReadFileStats(t *testing.T) {
+ root := t.TempDir()
+ for name, content := range map[string]string{"trailing": "a\nb\nc\n", "unterminated": "a\nb\nc", "single": "x\n", "empty": "", "crlf": "a\r\nb\r\n"} {
+ path := filepath.Join(root, name+".txt")
+ writeTestFile(t, path, content)
+ stats, err := scanReadFileStats(path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if got := trackedLineTotal(content); got != stats.lines {
+ t.Fatalf("%s: trackedLineTotal = %d, read_file reports %d", name, got, stats.lines)
+ }
+ }
+}
+
+// A whole-file read, an edit, then a partial re-read must keep the file "seen
+// whole": the writer and reader now agree on the line total, so the partial
+// read accumulates instead of resetting and a later edit far from the re-read
+// window is not refused.
+func TestEditThenPartialReadKeepsWholeFileObservation(t *testing.T) {
+ root := t.TempDir()
+ path := filepath.Join(root, "notes.txt")
+ content := ""
+ for i := 1; i <= 40; i++ {
+ content += fmt.Sprintf("line %02d\n", i)
+ }
+ writeTestFile(t, path, content)
+ tracker := NewFileTracker()
+ registry := NewRegistry()
+ registry.Register(NewScopedReadFileTool(root, nil))
+ registry.Register(NewScopedEditFileTool(root, nil))
+ opts := RunOptions{PermissionGranted: true, FileTracker: tracker}
+
+ if r := registry.RunWithOptions(context.Background(), "read_file", map[string]any{"path": "notes.txt"}, opts); r.Status != StatusOK {
+ t.Fatalf("read: %s", r.Output)
+ }
+ if r := registry.RunWithOptions(context.Background(), "edit_file", map[string]any{"path": "notes.txt", "old_string": "line 01\nline 02\n", "new_string": "line 01\nline two\n"}, opts); r.Status != StatusOK {
+ t.Fatalf("first edit: %s", r.Output)
+ }
+ if r := registry.RunWithOptions(context.Background(), "read_file", map[string]any{"path": "notes.txt", "limit": 5}, opts); r.Status != StatusOK {
+ t.Fatalf("partial read: %s", r.Output)
+ }
+ absolute, _ := filepath.EvalSymlinks(path)
+ if !tracker.SeenWhole(absolute) {
+ t.Fatal("partial re-read of an unchanged file must not discard whole-file knowledge")
+ }
+ if r := registry.RunWithOptions(context.Background(), "edit_file", map[string]any{"path": "notes.txt", "old_string": "line 39\nline 40\n", "new_string": "line 39\nline forty\n"}, opts); r.Status != StatusOK {
+ t.Fatalf("edit far from the re-read window must succeed: %s", r.Output)
+ }
+}
diff --git a/internal/tools/unified_patch.go b/internal/tools/unified_patch.go
new file mode 100644
index 000000000..ab09b3f1d
--- /dev/null
+++ b/internal/tools/unified_patch.go
@@ -0,0 +1,393 @@
+package tools
+
+import (
+ "fmt"
+ "strings"
+)
+
+// parseUnifiedPatch converts a unified diff into the same operations a
+// structured patch produces, so both formats are applied by Zero itself
+// through an opened workspace root (descriptor-relative, no-follow writes)
+// instead of handing pathnames to git apply after validation.
+//
+// Supported: file modifications, creations (--- /dev/null) and deletions
+// (+++ /dev/null, verified against the content the hunks expect to remove),
+// multiple hunks per file, "\ No newline at end of file"
+// markers, a/ and b/ prefixes, git C-quoted paths, CRLF patches, and git's
+// "rename from/to" and "copy from/to" headers (with or without hunks). Headers
+// git emits around a hunk (diff --git, index, mode lines) are skipped; binary
+// patches are rejected.
+func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) {
+ normalized := strings.TrimPrefix(strings.ReplaceAll(patch, "\r\n", "\n"), "\ufeff")
+ lines := strings.Split(normalized, "\n")
+ // The element after a final "\n" is the line terminator, not an empty
+ // context line; counting it would let a truncated hunk look complete.
+ if len(lines) > 0 && lines[len(lines)-1] == "" {
+ lines = lines[:len(lines)-1]
+ }
+
+ var operations []structuredPatchOperation
+ var current *structuredPatchOperation
+ var chunk *structuredPatchChunk
+ var added []string // collected "+" lines for a file creation
+ oldPath, newPath := "", ""
+ pendingFrom, pendingKind := "", structuredPatchUpdate
+ // git's header-only forms: "deleted file mode" / "new file mode" after a
+ // "diff --git" line with no ---/+++ pair describe an empty file.
+ diffPath, headerOnly := "", byte(0) // headerOnly is 'd' (deleted) or 'n' (new)
+ oldRemaining, newRemaining := 0, 0
+ inHunk := false
+ lastSide := byte(0) // '+' or '-' for the most recent content line
+
+ finish := func() error {
+ if current == nil {
+ return nil
+ }
+ switch current.kind {
+ case structuredPatchAdd:
+ if len(added) > 0 {
+ current.contents = strings.Join(added, "\n")
+ if current.eofNewline != eofNewlineAbsent {
+ current.contents += "\n"
+ }
+ }
+ case structuredPatchUpdate, structuredPatchCopy:
+ if !allStructuredPatchChunksHaveContent(current.chunks) {
+ return fmt.Errorf("unified diff for %s has an empty hunk", current.path)
+ }
+ if len(current.chunks) == 0 && current.movePath == "" {
+ return fmt.Errorf("unified diff for %s has no hunk lines", current.path)
+ }
+ case structuredPatchDelete:
+ if len(current.chunks) == 0 || !allStructuredPatchChunksHaveContent(current.chunks) {
+ return fmt.Errorf("unified deletion of %s must include the hunk with the content being removed", current.path)
+ }
+ }
+ operations = append(operations, *current)
+ current, chunk, added = nil, nil, nil
+ return nil
+ }
+ flushHeaderOnly := func(line int) error {
+ if headerOnly == 0 {
+ return nil
+ }
+ if diffPath == "" {
+ return fmt.Errorf("invalid unified diff at line %d: file mode header without a diff --git path", line)
+ }
+ if err := finish(); err != nil {
+ return err
+ }
+ switch headerOnly {
+ case 'd':
+ operations = append(operations, structuredPatchOperation{kind: structuredPatchDelete, path: diffPath, line: line, verifyDelete: true})
+ case 'n':
+ operations = append(operations, structuredPatchOperation{kind: structuredPatchAdd, path: diffPath, line: line})
+ }
+ headerOnly, diffPath = 0, ""
+ return nil
+ }
+ startFile := func(line int) error {
+ // A ---/+++ pair means the file has hunks; it is not header-only.
+ headerOnly, diffPath = 0, ""
+ if oldPath == "" || newPath == "" {
+ return fmt.Errorf("invalid unified diff at line %d: hunk before a ---/+++ header pair", line)
+ }
+ // A ---/+++ pair after a rename/copy header names the same files; keep
+ // accumulating that operation's hunks instead of starting a new one.
+ if current != nil && current.movePath != "" && current.path == oldPath && (current.movePath == newPath || oldPath == newPath) {
+ return nil
+ }
+ if err := finish(); err != nil {
+ return err
+ }
+ op := structuredPatchOperation{line: line}
+ switch {
+ case oldPath == "/dev/null" && newPath == "/dev/null":
+ return fmt.Errorf("invalid unified diff at line %d: both sides are /dev/null", line)
+ case oldPath == "/dev/null":
+ op.kind, op.path = structuredPatchAdd, newPath
+ case newPath == "/dev/null":
+ // A unified deletion states the content it expects to remove; keep
+ // its hunks so the planner verifies them before deleting.
+ op.kind, op.path, op.verifyDelete = structuredPatchDelete, oldPath, true
+ default:
+ op.kind, op.path = structuredPatchUpdate, oldPath
+ if newPath != oldPath {
+ op.movePath = newPath
+ }
+ }
+ current = &op
+ return nil
+ }
+
+ for index, raw := range lines {
+ lineNumber := index + 1
+ if inHunk && (oldRemaining > 0 || newRemaining > 0) {
+ // A "--- " line followed by "+++ " and then a "@@" hunk header is
+ // the next file's header pair, not a removed "-- x" line followed by
+ // an added "++ y" line: an over-declared count would otherwise
+ // swallow both and the hunk would later fail against the wrong
+ // file. The "@@" requirement keeps a genuine adjacent pair intact.
+ if strings.HasPrefix(raw, "--- ") && index+2 < len(lines) && strings.HasPrefix(lines[index+1], "+++ ") && strings.HasPrefix(lines[index+2], "@@") {
+ return nil, fmt.Errorf("invalid unified diff at line %d: hunk ended before its declared line counts", lineNumber)
+ }
+ switch {
+ case strings.HasPrefix(raw, "\\"):
+ // "\ No newline at end of file" qualifies the previous line's side.
+ if current != nil && lastSide == '+' {
+ current.eofNewline = eofNewlineAbsent
+ } else if current != nil && lastSide == '-' {
+ current.oldNoNewline = true
+ if current.eofNewline == eofNewlineKeep {
+ current.eofNewline = eofNewlinePresent
+ }
+ }
+ continue
+ case strings.HasPrefix(raw, "-"):
+ oldRemaining--
+ lastSide = '-'
+ if chunk != nil {
+ chunk.old = append(chunk.old, raw[1:])
+ }
+ case strings.HasPrefix(raw, "+"):
+ newRemaining--
+ lastSide = '+'
+ if current != nil && current.kind == structuredPatchAdd {
+ added = append(added, raw[1:])
+ } else if chunk != nil {
+ chunk.new = append(chunk.new, raw[1:])
+ chunk.newSourceOffsets = append(chunk.newSourceOffsets, -1)
+ }
+ default:
+ content := raw
+ if strings.HasPrefix(raw, " ") {
+ content = raw[1:]
+ } else if raw != "" {
+ return nil, fmt.Errorf("invalid unified diff at line %d: hunk lines must start with ' ', '+', '-' or '\\'", lineNumber)
+ }
+ oldRemaining--
+ newRemaining--
+ lastSide = ' '
+ if chunk != nil {
+ offset := len(chunk.old)
+ chunk.old = append(chunk.old, content)
+ chunk.new = append(chunk.new, content)
+ chunk.newSourceOffsets = append(chunk.newSourceOffsets, offset)
+ }
+ }
+ continue
+ }
+ inHunk = false
+ trimmed := strings.TrimSpace(raw)
+ if oldRemaining > 0 || newRemaining > 0 {
+ // The hunk ended before its declared counts were consumed. Report
+ // it here rather than absorbing the next file's ---/+++ headers as
+ // "-"/"+" content and failing later against the wrong file.
+ return nil, fmt.Errorf("invalid unified diff at line %d: hunk ended before its declared line counts", lineNumber)
+ }
+ switch {
+ case trimmed == "":
+ continue
+ case strings.HasPrefix(raw, "\\"):
+ // "\ No newline at end of file" directly after a hunk's last line.
+ if current != nil && lastSide == '+' {
+ current.eofNewline = eofNewlineAbsent
+ } else if current != nil && lastSide == '-' {
+ current.oldNoNewline = true
+ if current.eofNewline == eofNewlineKeep {
+ current.eofNewline = eofNewlinePresent
+ }
+ }
+ continue
+ case strings.HasPrefix(raw, "diff --git "):
+ if err := flushHeaderOnly(lineNumber); err != nil {
+ return nil, err
+ }
+ diffPath = diffGitNewPath(raw)
+ continue
+ case strings.HasPrefix(raw, "deleted file mode "):
+ headerOnly = 'd'
+ continue
+ case strings.HasPrefix(raw, "new file mode "):
+ headerOnly = 'n'
+ continue
+ case strings.HasPrefix(raw, "index "), strings.HasPrefix(raw, "old mode "), strings.HasPrefix(raw, "new mode "):
+ continue
+ case strings.HasPrefix(raw, "similarity index "), strings.HasPrefix(raw, "dissimilarity index "):
+ continue
+ case strings.HasPrefix(raw, "rename from "), strings.HasPrefix(raw, "copy from "):
+ pendingFrom = strings.TrimSpace(unquoteGitPath(strings.TrimPrefix(strings.TrimPrefix(raw, "rename from "), "copy from ")))
+ pendingKind = structuredPatchUpdate
+ if strings.HasPrefix(raw, "copy from ") {
+ pendingKind = structuredPatchCopy
+ }
+ if pendingFrom == "" {
+ return nil, fmt.Errorf("invalid unified diff at line %d: missing source path", lineNumber)
+ }
+ case strings.HasPrefix(raw, "rename to "), strings.HasPrefix(raw, "copy to "):
+ to := strings.TrimSpace(unquoteGitPath(strings.TrimPrefix(strings.TrimPrefix(raw, "rename to "), "copy to ")))
+ if pendingFrom == "" || to == "" {
+ return nil, fmt.Errorf("invalid unified diff at line %d: rename/copy destination without a source", lineNumber)
+ }
+ if err := finish(); err != nil {
+ return nil, err
+ }
+ current = &structuredPatchOperation{kind: pendingKind, path: pendingFrom, movePath: to, line: lineNumber}
+ oldPath, newPath, pendingFrom = pendingFrom, to, ""
+ case strings.HasPrefix(raw, "Binary files "), strings.HasPrefix(raw, "GIT binary patch"):
+ return nil, fmt.Errorf("invalid unified diff at line %d: binary patches are not supported", lineNumber)
+ case strings.HasPrefix(raw, "--- "):
+ oldPath = stripPatchPrefix(patchFileHeaderPath(raw))
+ newPath = ""
+ if oldPath == "" {
+ return nil, fmt.Errorf("invalid unified diff at line %d: missing path in --- header", lineNumber)
+ }
+ case strings.HasPrefix(raw, "+++ "):
+ newPath = stripPatchPrefix(patchFileHeaderPath(raw))
+ if newPath == "" {
+ return nil, fmt.Errorf("invalid unified diff at line %d: missing path in +++ header", lineNumber)
+ }
+ if err := startFile(lineNumber); err != nil {
+ return nil, err
+ }
+ case strings.HasPrefix(raw, "@@"):
+ if current == nil {
+ return nil, fmt.Errorf("invalid unified diff at line %d: hunk before a ---/+++ header pair", lineNumber)
+ }
+ oldStart, oldCount, newCount, ok := parseHunkRange(raw)
+ if !ok {
+ return nil, fmt.Errorf("invalid unified diff at line %d: malformed hunk header", lineNumber)
+ }
+ // "\ No newline at end of file" describes the end of the file, so it
+ // may only follow a file's final hunk; a marker before another hunk
+ // would otherwise leak into that later hunk's result.
+ if current.eofNewline != eofNewlineKeep {
+ return nil, fmt.Errorf("invalid unified diff at line %d: \"\\ No newline at end of file\" must follow the last hunk of a file", lineNumber)
+ }
+ oldRemaining, newRemaining = oldCount, newCount
+ inHunk = oldRemaining > 0 || newRemaining > 0
+ lastSide = 0
+ if current.kind != structuredPatchAdd {
+ next := structuredPatchChunk{hasHint: true, hint: oldStart - 1}
+ if oldCount == 0 {
+ // A pure insertion's range names the line after which the
+ // new lines go, so the insertion index is oldStart itself.
+ next.hint = oldStart
+ }
+ if next.hint < 0 {
+ next.hint = 0
+ }
+ current.chunks = append(current.chunks, next)
+ chunk = ¤t.chunks[len(current.chunks)-1]
+ }
+ default:
+ return nil, fmt.Errorf("invalid unified diff at line %d: unexpected %q", lineNumber, trimmed)
+ }
+ }
+ // Strict at end of input on purpose: a hunk cut short (typically a model
+ // response that stopped mid-patch) would otherwise apply as a partial
+ // change — removals without their replacements — which is worse than a
+ // clear error and a retry. Miscounted ranges with complete content are
+ // still tolerated at every other boundary.
+ if inHunk && (oldRemaining > 0 || newRemaining > 0) {
+ return nil, fmt.Errorf("invalid unified diff at line %d: hunk ended before its declared line counts", len(lines))
+ }
+ if err := flushHeaderOnly(len(lines)); err != nil {
+ return nil, err
+ }
+ if err := finish(); err != nil {
+ return nil, err
+ }
+ if len(operations) == 0 {
+ return nil, fmt.Errorf("unified diff contains no file changes")
+ }
+ return operations, nil
+}
+
+// diffGitNewPath returns the post-image path named by a "diff --git a/X b/Y"
+// line, handling git's C-quoted form; "" when it cannot be determined.
+func diffGitNewPath(line string) string {
+ rest := strings.TrimSpace(strings.TrimPrefix(line, "diff --git "))
+ if strings.HasPrefix(rest, "\"") {
+ // Two quoted tokens: skip the first, unquote the second.
+ end := strings.Index(rest[1:], "\"")
+ for end > 0 && rest[end] == '\\' {
+ next := strings.Index(rest[end+2:], "\"")
+ if next < 0 {
+ return ""
+ }
+ end += next + 2
+ }
+ if end < 0 || end+2 > len(rest) {
+ return ""
+ }
+ rest = strings.TrimSpace(rest[end+2:])
+ return stripPatchPrefix(unquoteGitPath(rest))
+ }
+ fields := strings.Fields(rest)
+ if len(fields) < 2 {
+ return ""
+ }
+ last := fields[len(fields)-1]
+ if strings.HasPrefix(last, "\"") {
+ return stripPatchPrefix(unquoteGitPath(last))
+ }
+ return stripPatchPrefix(last)
+}
+
+// parseHunkRange reads "@@ -a[,b] +c[,d] @@" and returns a, b and d; a missing
+// count means 1 per unified-diff convention.
+func parseHunkRange(line string) (oldStart, oldCount, newCount int, ok bool) {
+ _, rest, found := strings.Cut(line, "@@")
+ if !found {
+ return 0, 0, 0, false
+ }
+ rangeSection := rest
+ if before, _, found := strings.Cut(rest, "@@"); found {
+ rangeSection = before
+ }
+ fields := strings.Fields(rangeSection)
+ if len(fields) != 2 || !strings.HasPrefix(fields[0], "-") || !strings.HasPrefix(fields[1], "+") {
+ return 0, 0, 0, false
+ }
+ parse := func(spec string) (int, int, bool) {
+ startText, countText, hasCount := strings.Cut(spec, ",")
+ start, err := parseNonNegativeInt(startText)
+ if err != nil {
+ return 0, 0, false
+ }
+ count := 1
+ if hasCount {
+ if count, err = parseNonNegativeInt(countText); err != nil {
+ return 0, 0, false
+ }
+ }
+ return start, count, true
+ }
+ oldStart, oldCount, ok = parse(fields[0][1:])
+ if !ok {
+ return 0, 0, 0, false
+ }
+ _, newCount, ok = parse(fields[1][1:])
+ if !ok {
+ return 0, 0, 0, false
+ }
+ return oldStart, oldCount, newCount, true
+}
+
+func parseNonNegativeInt(text string) (int, error) {
+ if text == "" {
+ return 0, fmt.Errorf("empty number")
+ }
+ value := 0
+ for _, r := range text {
+ if r < '0' || r > '9' {
+ return 0, fmt.Errorf("not a number: %q", text)
+ }
+ value = value*10 + int(r-'0')
+ if value > 1<<30 {
+ return 0, fmt.Errorf("number too large: %q", text)
+ }
+ }
+ return value, nil
+}
diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go
index 558530211..76f5f1baa 100644
--- a/internal/tools/write_file.go
+++ b/internal/tools/write_file.go
@@ -120,7 +120,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
newInfo, _ := os.Stat(absolutePath)
options.FileTracker.Record(absolutePath, []byte(content), newInfo)
if content == modelKnownContent {
- options.FileTracker.RecordSeenRange(absolutePath, 1, lineCount(content), lineCount(content))
+ options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(content), trackedLineTotal(content))
}
if !existed {
options.FileTracker.RecordCreated(absolutePath)
diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go
index fc8c0c771..87849e859 100644
--- a/internal/tools/write_tools_test.go
+++ b/internal/tools/write_tools_test.go
@@ -866,7 +866,9 @@ func TestStructuredPatchFailedDeleteDoesNotRecreateMissingFile(t *testing.T) {
}
}
-func TestStructuredPatchMoveFailureLeavesPublishedDestination(t *testing.T) {
+// A move whose source vanishes after planning is refused by the pre-commit
+// recheck before the destination is published, so nothing is left half-done.
+func TestStructuredPatchMoveWithMissingSourceIsRefusedBeforePublishing(t *testing.T) {
root := t.TempDir()
workspace, err := os.OpenRoot(root)
if err != nil {
@@ -875,22 +877,35 @@ func TestStructuredPatchMoveFailureLeavesPublishedDestination(t *testing.T) {
defer workspace.Close()
sourcePath := filepath.Join(root, "source.txt")
destinationPath := filepath.Join(root, "destination.txt")
+ writeTestFile(t, sourcePath, "removed source\n")
change := structuredPatchChange{
kind: structuredPatchUpdate,
from: structuredPatchTarget{absolute: sourcePath, relative: "source.txt"},
to: structuredPatchTarget{absolute: destinationPath, relative: "destination.txt"},
before: "removed source\n", after: "moved content\n", mode: 0o644,
}
+ // The source exists at planning time and disappears just before commit.
+ removed := false
+ structuredPatchBeforeCommit = func(structuredPatchChange) {
+ if err := os.Remove(sourcePath); err != nil {
+ t.Fatal(err)
+ }
+ removed = true
+ }
+ defer func() { structuredPatchBeforeCommit = nil }()
err = applyStructuredPatchChanges(workspace, []structuredPatchChange{change}, nil)
- if err == nil || !strings.Contains(err.Error(), "partially applied") {
- t.Fatalf("move with a missing source = %v, want partial-application error", err)
+ if !removed {
+ t.Fatal("pre-commit hook did not run")
+ }
+ if err == nil || !strings.Contains(err.Error(), "before commit") || strings.Contains(err.Error(), "partially applied") {
+ t.Fatalf("move with a missing source = %v, want a pre-commit refusal with nothing applied", err)
}
if _, statErr := os.Stat(sourcePath); !os.IsNotExist(statErr) {
t.Fatalf("failed move recreated a missing source: %v", statErr)
}
- if got := mustReadTestFile(t, destinationPath); got != "moved content\n" {
- t.Fatalf("published move destination = %q", got)
+ if _, statErr := os.Stat(destinationPath); !os.IsNotExist(statErr) {
+ t.Fatalf("refused move must not publish the destination: %v", statErr)
}
}
diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go
index d456c8473..ad59494b3 100644
--- a/internal/tui/model_test.go
+++ b/internal/tui/model_test.go
@@ -2439,7 +2439,7 @@ func TestAssistantNarrationBeforeToolCardGetsBlankSeparator(t *testing.T) {
m.transcript = append(m.transcript,
transcriptRow{kind: rowUser, text: "run it"},
transcriptRow{kind: rowAssistant, text: "I'll inspect the existing file, then run it."},
- transcriptRow{kind: rowToolResult, id: "t1", tool: "read_file", status: tools.StatusOK, detail: "File: time_test.py\n\n 1 | print('x')"},
+ transcriptRow{kind: rowToolResult, id: "t1", tool: "read_file", status: tools.StatusOK, detail: "File: time_test.py\n\n1→print('x')"},
)
items := m.transcriptBodyItems(m.chatColumnWidth(), "", false)
toolIdx := -1
diff --git a/internal/tui/rendering_lime_test.go b/internal/tui/rendering_lime_test.go
index 07d8382da..de95257fc 100644
--- a/internal/tui/rendering_lime_test.go
+++ b/internal/tui/rendering_lime_test.go
@@ -1075,8 +1075,8 @@ func TestEditedDiffCardHandlesBareBlankHunkContext(t *testing.T) {
func TestReadCardBodyShowsExploredSummary(t *testing.T) {
m := limeTestModel()
- // Mirrors the real read_file output shape: " | ".
- detail := "File: internal/agent/loop.go\n\n 12 | func Run() {\n 13 | }\n"
+ // Mirrors the real read_file output shape: "N→".
+ detail := "File: internal/agent/loop.go\n\n12→func Run() {\n13→}\n"
row := transcriptRow{kind: rowToolResult, id: "call_1", tool: "read_file", status: tools.StatusOK, detail: detail}
rc := buildRowContext([]transcriptRow{{kind: rowToolCall, id: "call_1", tool: "read_file", detail: "internal/agent/loop.go"}})
got := plainRender(t, m.renderRow(row, 80, rc))
@@ -1097,7 +1097,7 @@ func TestReadCardBodyShowsExploredSummary(t *testing.T) {
row.expanded = true
expanded := plainRender(t, m.renderRow(row, 80, rc))
- for _, want := range []string{"Explored", "└ Read", "internal/agent/loop.go", "func Run()", "13 |"} {
+ for _, want := range []string{"Explored", "└ Read", "internal/agent/loop.go", "func Run()", "13→"} {
if !strings.Contains(expanded, want) {
t.Fatalf("expanded read card = %q, missing %q", expanded, want)
}
diff --git a/internal/tui/tool_render_registry_test.go b/internal/tui/tool_render_registry_test.go
index 8b9cb15b9..ef0cbbd6e 100644
--- a/internal/tui/tool_render_registry_test.go
+++ b/internal/tui/tool_render_registry_test.go
@@ -41,7 +41,7 @@ func TestDefaultToolBodyRegistrySelectsCoreRenderers(t *testing.T) {
{
name: "read_file",
hint: "README.md",
- detail: "File: README.md\n\n 7 | # Zero",
+ detail: "File: README.md\n\n7→# Zero",
want: []string{"Read", "README.md"},
},
{