From e9698ee5c229984ee8361b212e90a37edc724dbc Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 00:58:00 +0800 Subject: [PATCH 01/15] fix(tools): make apply_patch tolerant and re-expose edit_file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Head-to-head benchmarking against Grok Build on grok-4.6 (3 tasks × 3 runs) showed Zero solving every task but spending 2.7× the tokens and 2.3× the time on the multi-file feature task. Every one of the 20 apply_patch calls in those runs failed, after which the model degraded to whole-file write_file rewrites and read→write verification loops. Root causes and fixes: - The sandbox and the tool both rejected an absolute path that lies inside the workspace ("must stay inside the workspace"). Models routinely echo the absolute path read_file showed them. Absolute paths now flow through the normal workspace-scope validation (inside → allowed, outside → still denied); unified-diff headers are rewritten to a/ b/ relative form before git apply, and platform symlink prefixes (/var → /private/var) are normalised the same way the other file tools do. - The structured parser demanded a byte-exact "*** Begin Patch" / "*** End Patch"; grok-4.6 writes "*** Begin Patch ***". Decorated marker spellings are accepted, and a unified range after "@@" ("-12,4 +12,6", optionally with a heading) no longer becomes a literal context search. - Parse errors now carry a one-line format reminder so a failed call is recoverable in one retry instead of a format-guessing loop. - edit_file (exact search/replace with uniqueness guard, CRLF and fuzzy fallbacks) is advertised to the model again alongside apply_patch, and the apply_patch description shows the structured format. - Writers (edit_file, write_file, apply_patch) recorded a file's line total with lineCount, which is one higher than read_file's count for a file ending in a newline. RecordSeenRange resets every observation when the total changes, so a partial read_file after a successful edit silently discarded whole-file knowledge and the next edit was refused with "content that has not been read exactly in this session". All writers now record the same total read_file reports (trackedLineTotal). - read_file's padded " N | " line prefix was ~19% of every read (about 1.5k tokens per 800-line file) and was re-sent on every later call. It is now the compact "N→" prefix models already know; nothing parsed the old form. - The editing-discipline prompt steers: edit_file for a targeted change, apply_patch for multi-hunk/multi-file changes, write_file only for new files or full rewrites; a successful edit result is confirmation, so the file is not re-read to verify it. Tests: regression tests for decorated markers, writer/reader line-total agreement (edit → partial read → edit far away still succeeds), range hunk headers, absolute in-workspace paths (structured + unified), outside-workspace rejection, header-line-only relativisation, and tool visibility; the prompt and tool-budget tests updated to the new contract. Benchmark (grok-4.6, 3 tasks x 3 runs, harness in demo/bench-zero-vs-grok): Zero 0.8.0 617 s / 1,527k tokens -> with this change 346 s / 493k tokens; Grok Build 407 s / 713k. Every run of every configuration solved its task. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/agent/prompt_budget_test.go | 8 +- internal/agent/system_prompt.md | 19 +- internal/agent/system_prompt_models.go | 8 +- internal/agent/system_prompt_test.go | 2 +- internal/sandbox/apply_patch_paths_test.go | 57 ++++++ internal/sandbox/risk.go | 6 +- internal/tools/apply_patch.go | 105 ++++++++++- internal/tools/apply_patch_tolerance_test.go | 180 +++++++++++++++++++ internal/tools/edit_file.go | 2 +- internal/tools/file_tools_test.go | 14 +- internal/tools/model_visibility.go | 10 +- internal/tools/model_visibility_test.go | 19 +- internal/tools/read_file.go | 20 ++- internal/tools/read_minified_file.go | 17 ++ internal/tools/structured_patch.go | 66 +++++-- internal/tools/tracked_line_total_test.go | 59 ++++++ internal/tools/write_file.go | 2 +- 17 files changed, 538 insertions(+), 56 deletions(-) create mode 100644 internal/sandbox/apply_patch_paths_test.go create mode 100644 internal/tools/apply_patch_tolerance_test.go create mode 100644 internal/tools/tracked_line_total_test.go 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..3c168dba9 100644 --- a/internal/agent/system_prompt_models.go +++ b/internal/agent/system_prompt_models.go @@ -60,14 +60,16 @@ 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. Make one tool call per file; 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..b6ded23bc --- /dev/null +++ b/internal/sandbox/apply_patch_paths_test.go @@ -0,0 +1,57 @@ +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 ***", + } { + 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", ".."} { + request := Request{ToolName: "apply_patch", WorkspaceRoot: root, SideEffect: SideEffectWrite, Args: map[string]any{"patch": structured(path)}} + block := applyPatchPathBlock(request) + if block == nil || block.Code != BlockOutsideWorkspace { + t.Fatalf("%q must be blocked as traversal, got %+v", path, block) + } + } +} + +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) + } + patch := strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "*** End Patch"}, "\n") + paths := applyPatchRequestPaths(map[string]any{"patch": patch}) + if len(paths) != 1 || paths[0] != outside { + t.Fatalf("absolute patch path must reach scope validation unchanged, got %v", paths) + } + scope, err := NewScope(root, nil) + if err != nil { + t.Fatal(err) + } + if block := scope.validate(outside); block == nil || block.Code != BlockOutsideWorkspace { + t.Fatalf("scope must still deny an absolute path outside the workspace, got %+v", block) + } + if block := scope.validate(filepath.Join(root, "main.js")); block != nil { + t.Fatalf("scope must accept an absolute path inside the workspace, got %+v", block) + } +} diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index a37868c63..802e8d46f 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, diff --git a/internal/tools/apply_patch.go b/internal/tools/apply_patch.go index 2a74ae90a..5c08f41da 100644 --- a/internal/tools/apply_patch.go +++ b/internal/tools/apply_patch.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strconv" "strings" + + "github.com/Gitlawb/zero/internal/sandbox" ) type applyPatchTool struct { @@ -20,11 +22,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 (git apply -p1) is also accepted. 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,6 +61,7 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a if isStructuredPatch(patch) { return tool.runStructuredPatch(applyRoot, relativeRoot, patch, options) } + patch = relativizeUnifiedPatchPaths(applyRoot, patch) if err := validatePatchPaths(applyRoot, patch); err != nil { return errorResult("Error applying patch: " + err.Error()) } @@ -135,7 +138,7 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a wasWhole := wholeBefore[absolute] || fullySupplied[absolute] options.FileTracker.Record(absolute, content, info) if wasWhole { - lines := lineCount(string(content)) + lines := trackedLineTotal(string(content)) options.FileTracker.RecordSeenRange(absolute, 1, lines, lines) } } @@ -260,15 +263,107 @@ func changedFilesFromPatch(relativeRoot string, patch string) []string { return paths } +// relativizeUnifiedPatchPaths rewrites absolute file paths in a unified diff's +// headers (`diff --git`, `---`, `+++`) to apply-root-relative "a/…"/"b/…" paths +// when they resolve inside the root, so `git apply -p1` can consume a patch +// whose author echoed the absolute paths it was shown. Paths outside the root +// and every hunk body line are left untouched; validatePatchPaths still rejects +// anything that escapes the workspace. +// 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 + } + 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 relativizeUnifiedPatchPaths(root string, patch string) string { + relativize := func(path string) (string, bool) { + if path == "" || path == "/dev/null" || !filepath.IsAbs(path) { + return path, false + } + if _, relative, err := resolveWorkspaceTargetPath(root, normalizePatchPathForRoot(root, path)); err == nil && relative != "." { + return relative, true + } + return path, false + } + lines := strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") + oldRemaining, newRemaining := 0, 0 + inHunk := false + changed := false + for index, line := range lines { + 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 "): + fields := strings.Fields(line) + if len(fields) >= 4 { + a, aok := relativize(fields[2]) + b, bok := relativize(fields[3]) + if aok || bok { + if aok { + fields[2] = "a/" + a + } + if bok { + fields[3] = "b/" + b + } + lines[index] = strings.Join(fields, " ") + changed = true + } + } + case strings.HasPrefix(line, "@@"): + oldRemaining, newRemaining = parseHunkCounts(line) + inHunk = oldRemaining > 0 || newRemaining > 0 + case strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "): + if relative, ok := relativize(patchFileHeaderPath(line)); ok { + prefix := "a/" + if strings.HasPrefix(line, "+++ ") { + prefix = "b/" + } + lines[index] = line[:4] + prefix + relative + changed = true + } + } + } + if !changed { + return patch + } + return strings.Join(lines, "\n") +} + 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, "../") { + // 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, path); err != nil { + if _, _, err := resolveWorkspaceTargetPath(root, normalizePatchPathForRoot(root, path)); err != nil { return err } } diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go new file mode 100644 index 000000000..2574f898d --- /dev/null +++ b/internal/tools/apply_patch_tolerance_test.go @@ -0,0 +1,180 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "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) + } + } +} + +func TestRelativizeUnifiedPatchPathsLeavesHunkBodiesAlone(t *testing.T) { + root := t.TempDir() + absolute := filepath.Join(root, "notes.txt") + patch := strings.Join([]string{ + "diff --git " + absolute + " " + absolute, + "--- " + absolute, + "+++ " + absolute, + "@@ -1,2 +1,2 @@", + "-see " + absolute, + "+see " + absolute + " (kept)", + " --- not a header", + "", + }, "\n") + got := relativizeUnifiedPatchPaths(root, patch) + want := strings.Join([]string{ + "diff --git a/notes.txt b/notes.txt", + "--- a/notes.txt", + "+++ b/notes.txt", + "@@ -1,2 +1,2 @@", + "-see " + absolute, + "+see " + absolute + " (kept)", + " --- not a header", + "", + }, "\n") + if got != want { + t.Fatalf("relativized patch mismatch:\n%s\n--- want ---\n%s", got, want) + } + if plain := "--- a/x.go\n+++ b/x.go\n@@ -1 +1 @@\n-a\n+b\n"; relativizeUnifiedPatchPaths(root, plain) != plain { + t.Fatal("a patch without absolute paths must be returned unchanged") + } +} 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..57cca1b56 100644 --- a/internal/tools/read_minified_file.go +++ b/internal/tools/read_minified_file.go @@ -189,3 +189,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..6a46c68a0 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" "github.com/Gitlawb/zero/internal/pathjail" @@ -63,8 +64,45 @@ type structuredPatchChange struct { mode os.FileMode } +// structuredPatchMarkerPattern accepts the canonical "*** Begin Patch" / +// "*** End Patch" markers plus the decorated spellings models commonly emit +// ("*** Begin Patch ***", "***Begin Patch", trailing whitespace). A strict +// byte-equal match here made every patch from some models fail on line 1 and +// pushed them into whole-file rewrites. +var structuredPatchMarkerPattern = regexp.MustCompile(`^\*{3}\s*(Begin|End) Patch\s*\**\s*$`) + +// 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. +func structuredPatchMarker(line string) string { + match := structuredPatchMarkerPattern.FindStringSubmatch(strings.TrimSpace(line)) + if match == nil { + return "" + } + return strings.ToLower(match[1]) +} + +// 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) + first, _, _ := strings.Cut(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), "\n") + return structuredPatchMarker(first) == "begin" } func (tool applyPatchTool) runStructuredPatch(applyRoot, relativeRoot, patch string, options RunOptions) Result { @@ -122,11 +160,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 +229,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 +290,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 { @@ -353,10 +395,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 } @@ -706,7 +752,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/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) From ff9b0de9e5695f50a44db4472af3df1a0c44b167 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 09:11:39 +0800 Subject: [PATCH 02/15] fix(tools): keep CRLF line endings when relativizing patch headers relativizeUnifiedPatchPaths split on a normalized "\n" and rejoined with "\n", so any CRLF patch that needed a header rewrite lost its line endings everywhere; git apply could then fail to match a CRLF file. Split on "\n" only and carry each line's trailing "\r" through, including on rewritten headers. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/apply_patch.go | 12 ++++++++---- internal/tools/apply_patch_tolerance_test.go | 4 ++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/tools/apply_patch.go b/internal/tools/apply_patch.go index 5c08f41da..ea233d031 100644 --- a/internal/tools/apply_patch.go +++ b/internal/tools/apply_patch.go @@ -297,11 +297,15 @@ func relativizeUnifiedPatchPaths(root string, patch string) string { } return path, false } - lines := strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") + // Split on "\n" only and keep any trailing "\r" on each line, so a CRLF + // patch keeps its line endings everywhere except the rewritten headers. + lines := strings.Split(patch, "\n") oldRemaining, newRemaining := 0, 0 inHunk := false changed := false - for index, line := range lines { + for index, rawLine := range lines { + line := strings.TrimSuffix(rawLine, "\r") + eol := rawLine[len(line):] if inHunk && (oldRemaining > 0 || newRemaining > 0) { switch { case strings.HasPrefix(line, "-"): @@ -329,7 +333,7 @@ func relativizeUnifiedPatchPaths(root string, patch string) string { if bok { fields[3] = "b/" + b } - lines[index] = strings.Join(fields, " ") + lines[index] = strings.Join(fields, " ") + eol changed = true } } @@ -342,7 +346,7 @@ func relativizeUnifiedPatchPaths(root string, patch string) string { if strings.HasPrefix(line, "+++ ") { prefix = "b/" } - lines[index] = line[:4] + prefix + relative + lines[index] = line[:4] + prefix + relative + eol changed = true } } diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index 2574f898d..0351e94c2 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -177,4 +177,8 @@ func TestRelativizeUnifiedPatchPathsLeavesHunkBodiesAlone(t *testing.T) { if plain := "--- a/x.go\n+++ b/x.go\n@@ -1 +1 @@\n-a\n+b\n"; relativizeUnifiedPatchPaths(root, plain) != plain { t.Fatal("a patch without absolute paths must be returned unchanged") } + crlf := "--- " + absolute + "\r\n+++ " + absolute + "\r\n@@ -1 +1 @@\r\n-a\r\n+b\r\n" + if got, want := relativizeUnifiedPatchPaths(root, crlf), "--- a/notes.txt\r\n+++ b/notes.txt\r\n@@ -1 +1 @@\r\n-a\r\n+b\r\n"; got != want { + t.Fatalf("CRLF patch must keep its line endings:\n%q\nwant\n%q", got, want) + } } From 9483000873259efdb8619f42a9146bceeefce29a Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 09:31:31 +0800 Subject: [PATCH 03/15] fix(sandbox): share the structured-patch classifier with apply_patch Review follow-ups for #956: - The tool accepted decorated structured markers ("***Begin Patch") that the sandbox's applyPatchPaths did not recognise, so for that spelling the boundary scanned the patch as a unified diff, extracted no targets and validated nothing. The marker classifier now lives in the sandbox package (StructuredPatchMarker / IsStructuredPatch) and the tool delegates to it, so both sides accept exactly the same set; tests cover inside, traversal and outside targets for the no-space spelling at the boundary. - Document the containment model at the apply site: structured patches are applied through an opened os.Root (descriptor-relative, no-follow); unified diffs keep the pre-existing git-apply pathname flow, which the absolute-path rewrite does not widen (they become the same root-relative form relative paths always used). - OpenAI addendum: the one-file-per-call rule names read_file, edit_file and write_file explicitly and allows a single multi-file apply_patch. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/agent/system_prompt_models.go | 8 ++-- internal/sandbox/apply_patch_paths_test.go | 46 ++++++++++++++++++---- internal/sandbox/risk.go | 29 +++++++++++++- internal/tools/apply_patch.go | 10 +++++ internal/tools/structured_patch.go | 22 ++++------- 5 files changed, 88 insertions(+), 27 deletions(-) diff --git a/internal/agent/system_prompt_models.go b/internal/agent/system_prompt_models.go index 3c168dba9..c66773577 100644 --- a/internal/agent/system_prompt_models.go +++ b/internal/agent/system_prompt_models.go @@ -61,9 +61,11 @@ const openAIPromptAddendum = ` commands, and symbols. - Strongly prefer the native file tools (read_file, list_directory, grep, glob, edit_file, apply_patch, write_file) 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. Independent tool calls (several reads, several edit_file calls to - different files) belong in the same turn. + 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. ` diff --git a/internal/sandbox/apply_patch_paths_test.go b/internal/sandbox/apply_patch_paths_test.go index b6ded23bc..5ae89681f 100644 --- a/internal/sandbox/apply_patch_paths_test.go +++ b/internal/sandbox/apply_patch_paths_test.go @@ -16,6 +16,7 @@ func TestApplyPatchPathBlockOnlyRejectsRelativeTraversal(t *testing.T) { "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 { @@ -23,14 +24,39 @@ func TestApplyPatchPathBlockOnlyRejectsRelativeTraversal(t *testing.T) { } } for _, path := range []string{"../escape.js", ".."} { - request := Request{ToolName: "apply_patch", WorkspaceRoot: root, SideEffect: SideEffectWrite, Args: map[string]any{"patch": structured(path)}} - block := applyPatchPathBlock(request) - if block == nil || block.Code != BlockOutsideWorkspace { - t.Fatalf("%q must be blocked as traversal, got %+v", path, block) + 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 @@ -39,10 +65,14 @@ func TestApplyPatchRequestPathsCarryAbsolutePathsToScopeValidation(t *testing.T) if err != nil { t.Fatal(err) } - patch := strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "*** End Patch"}, "\n") - paths := applyPatchRequestPaths(map[string]any{"patch": patch}) - if len(paths) != 1 || paths[0] != outside { - t.Fatalf("absolute patch path must reach scope validation unchanged, got %v", paths) + for name, patch := range map[string]string{ + "canonical": strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "*** End Patch"}, "\n"), + "no-space": strings.Join([]string{"***Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "***End Patch"}, "\n"), + } { + paths := applyPatchRequestPaths(map[string]any{"patch": patch}) + if len(paths) != 1 || paths[0] != outside { + t.Fatalf("%s: absolute patch path must reach scope validation unchanged, got %v", name, paths) + } } scope, err := NewScope(root, nil) if err != nil { diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 802e8d46f..faa4e09bc 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -300,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 ea233d031..ef167c340 100644 --- a/internal/tools/apply_patch.go +++ b/internal/tools/apply_patch.go @@ -59,8 +59,18 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a return errorResult("Error applying patch: " + err.Error()) } if isStructuredPatch(patch) { + // Structured patches (the format models use) are applied by Zero itself + // through an opened workspace root (os.Root): every read, create and + // write below is descriptor-relative and no-follow, so there is no + // pathname check-to-use window. return tool.runStructuredPatch(applyRoot, relativeRoot, patch, options) } + // Unified diffs are handed to git apply, which opens targets by name after + // the pathname checks below; that pre-existing check-to-use window is the + // same one the relative-path flow always had. Absolute paths are only ever + // rewritten to the same root-relative form relative paths already use, so + // this does not widen it. Moving unified diffs onto the os.Root path is a + // separate change. patch = relativizeUnifiedPatchPaths(applyRoot, patch) if err := validatePatchPaths(applyRoot, patch); err != nil { return errorResult("Error applying patch: " + err.Error()) diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 6a46c68a0..c03c39d9f 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/Gitlawb/zero/internal/pathjail" + "github.com/Gitlawb/zero/internal/sandbox" ) const ( @@ -64,13 +65,6 @@ type structuredPatchChange struct { mode os.FileMode } -// structuredPatchMarkerPattern accepts the canonical "*** Begin Patch" / -// "*** End Patch" markers plus the decorated spellings models commonly emit -// ("*** Begin Patch ***", "***Begin Patch", trailing whitespace). A strict -// byte-equal match here made every patch from some models fail on line 1 and -// pushed them into whole-file rewrites. -var structuredPatchMarkerPattern = regexp.MustCompile(`^\*{3}\s*(Begin|End) Patch\s*\**\s*$`) - // 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 @@ -80,13 +74,12 @@ var unifiedHunkRangePattern = regexp.MustCompile(`^-\d+(?:,\d+)?\s+\+\d+(?:,\d+) 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. +// 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 { - match := structuredPatchMarkerPattern.FindStringSubmatch(strings.TrimSpace(line)) - if match == nil { - return "" - } - return strings.ToLower(match[1]) + return sandbox.StructuredPatchMarker(line) } // structuredHunkAnchor normalises the text after a hunk's "@@ " marker. A @@ -101,8 +94,7 @@ func structuredHunkAnchor(context string) string { } func isStructuredPatch(patch string) bool { - first, _, _ := strings.Cut(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), "\n") - return structuredPatchMarker(first) == "begin" + return sandbox.IsStructuredPatch(patch) } func (tool applyPatchTool) runStructuredPatch(applyRoot, relativeRoot, patch string, options RunOptions) Result { From 29e5fdf4ed8f29e64a168c97eabe24c965f8b36b Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 10:40:48 +0800 Subject: [PATCH 04/15] test(sandbox): compare slash-normalised patch paths so the check passes on Windows structuredPatchHeaderPaths normalises separators to "/", so the absolute outside-workspace path in the boundary test must be compared in its slash form; on Windows the backslash original never matched and the Smoke job failed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/sandbox/apply_patch_paths_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/apply_patch_paths_test.go b/internal/sandbox/apply_patch_paths_test.go index 5ae89681f..dc6f1d5fc 100644 --- a/internal/sandbox/apply_patch_paths_test.go +++ b/internal/sandbox/apply_patch_paths_test.go @@ -69,8 +69,10 @@ func TestApplyPatchRequestPathsCarryAbsolutePathsToScopeValidation(t *testing.T) "canonical": strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "*** End Patch"}, "\n"), "no-space": strings.Join([]string{"***Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "***End Patch"}, "\n"), } { + // structuredPatchHeaderPaths normalises separators to "/", so compare the + // slash form; the absolute path itself must survive untouched. paths := applyPatchRequestPaths(map[string]any{"patch": patch}) - if len(paths) != 1 || paths[0] != 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) } } From 5a1d9663194c8b27fd066d809643199496caaacb Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 10:46:42 +0800 Subject: [PATCH 05/15] test(sandbox): validate the parsed patch path at the scope boundary For each accepted marker spelling, pass the exact path applyPatchRequestPaths produced to scope.validate: an outside path must be denied and an inside path accepted, so a separator-handling regression between the parser and the scope cannot slip past the test. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/sandbox/apply_patch_paths_test.go | 37 +++++++++++++--------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/internal/sandbox/apply_patch_paths_test.go b/internal/sandbox/apply_patch_paths_test.go index dc6f1d5fc..3f9a2dc66 100644 --- a/internal/sandbox/apply_patch_paths_test.go +++ b/internal/sandbox/apply_patch_paths_test.go @@ -65,25 +65,32 @@ func TestApplyPatchRequestPathsCarryAbsolutePathsToScopeValidation(t *testing.T) if err != nil { t.Fatal(err) } - for name, patch := range map[string]string{ - "canonical": strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "*** End Patch"}, "\n"), - "no-space": strings.Join([]string{"***Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "***End Patch"}, "\n"), - } { - // structuredPatchHeaderPaths normalises separators to "/", so compare the - // slash form; the absolute path itself must survive untouched. - paths := applyPatchRequestPaths(map[string]any{"patch": patch}) - if len(paths) != 1 || paths[0] != filepath.ToSlash(outside) { - t.Fatalf("%s: absolute patch path must reach scope validation unchanged, got %v", name, paths) - } - } scope, err := NewScope(root, nil) if err != nil { t.Fatal(err) } - if block := scope.validate(outside); block == nil || block.Code != BlockOutsideWorkspace { - t.Fatalf("scope must still deny an absolute path outside the workspace, got %+v", block) + 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") } - if block := scope.validate(filepath.Join(root, "main.js")); block != nil { - t.Fatalf("scope must accept an absolute path inside the workspace, got %+v", block) + 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) + } } } From e32047745de60b5551f5273b29ca6a0f939056e0 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 13:56:43 +0800 Subject: [PATCH 06/15] fix(tools): apply unified diffs in-process through the workspace root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up for #956. Unified diffs were validated by pathname and then handed to git apply, which reopened the targets by name — a check-to-use window a symlink swap could exploit — and the absolute-path rewrite added in this PR normalised `diff --git` headers with strings.Fields, which splits a git C-quoted path containing whitespace. Both go away by not shelling out at all: parseUnifiedPatch translates a unified diff into the same operations a structured patch produces, and both formats are applied by applyPatchOperations through os.OpenRoot — every stat, read, create and write is descriptor-relative and refuses to follow a link out of the root. git is no longer required by apply_patch. Unified-diff support: modifications, creations (--- /dev/null), deletions (+++ /dev/null), multiple hunks per file, git "rename from/to" (a move, hunks optional) and "copy from/to" (a new copy operation whose destination inherits only the tracker credit the source had), "\ No newline at end of file" on either side, a/ b/ prefixes, C-quoted paths, CRLF patches, and git's diff/index/mode lines. Hunks are placed at their "@@ -a,b" range when the expected lines match there and located by context otherwise (so a stale range still applies and duplicate blocks are disambiguated by the range). Binary patches are rejected. Removed: the git apply flow, relativizeUnifiedPatchPaths, recheckPatchWriteTargets, missingPatchTargets, completeCreatedPatchTargets and recordCreatedPatchTargets. validatePatchPaths/changedFilesFromPatch stay for mutation-target listing. Tests: the existing unified-diff suite now runs through the in-process applier unchanged; new coverage for quoted whitespace paths (inside applies, outside is refused untouched), a workspace path swapped for a symlink that escapes the root (both formats refused, outside file unmodified), create + delete + no-newline markers, range hint vs context fallback, rename with a hunk, and binary/empty rejection. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/apply_patch.go | 278 +----------------- internal/tools/apply_patch_tolerance_test.go | 195 +++++++++++-- internal/tools/structured_patch.go | 88 +++++- internal/tools/unified_patch.go | 279 +++++++++++++++++++ 4 files changed, 538 insertions(+), 302 deletions(-) create mode 100644 internal/tools/unified_patch.go diff --git a/internal/tools/apply_patch.go b/internal/tools/apply_patch.go index ef167c340..4862b65f1 100644 --- a/internal/tools/apply_patch.go +++ b/internal/tools/apply_patch.go @@ -3,8 +3,6 @@ package tools import ( "context" "fmt" - "os" - "os/exec" "path/filepath" "strconv" "strings" @@ -22,7 +20,7 @@ func NewScopedApplyPatchTool(workspaceRoot string, scope PathScope) Tool { return applyPatchTool{ baseTool: baseTool{ name: "apply_patch", - 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 (git apply -p1) is also accepted. Paths are workspace-relative; absolute paths inside the workspace are fine. For a single targeted change, edit_file is simpler.", + 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{ @@ -59,190 +57,16 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a return errorResult("Error applying patch: " + err.Error()) } if isStructuredPatch(patch) { - // Structured patches (the format models use) are applied by Zero itself - // through an opened workspace root (os.Root): every read, create and - // write below is descriptor-relative and no-follow, so there is no - // pathname check-to-use window. return tool.runStructuredPatch(applyRoot, relativeRoot, patch, options) } - // Unified diffs are handed to git apply, which opens targets by name after - // the pathname checks below; that pre-existing check-to-use window is the - // same one the relative-path flow always had. Absolute paths are only ever - // rewritten to the same root-relative form relative paths already use, so - // this does not widen it. Moving unified diffs onto the os.Root path is a - // separate change. - patch = relativizeUnifiedPatchPaths(applyRoot, patch) - 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 := trackedLineTotal(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 @@ -273,12 +97,6 @@ func changedFilesFromPatch(relativeRoot string, patch string) []string { return paths } -// relativizeUnifiedPatchPaths rewrites absolute file paths in a unified diff's -// headers (`diff --git`, `---`, `+++`) to apply-root-relative "a/…"/"b/…" paths -// when they resolve inside the root, so `git apply -p1` can consume a patch -// whose author echoed the absolute paths it was shown. Paths outside the root -// and every hunk body line are left untouched; validatePatchPaths still rejects -// anything that escapes the workspace. // 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 @@ -297,76 +115,6 @@ func normalizePatchPathForRoot(root string, path string) string { return sandbox.NormalizePrefixForRoot(path, resolvedRoot) } -func relativizeUnifiedPatchPaths(root string, patch string) string { - relativize := func(path string) (string, bool) { - if path == "" || path == "/dev/null" || !filepath.IsAbs(path) { - return path, false - } - if _, relative, err := resolveWorkspaceTargetPath(root, normalizePatchPathForRoot(root, path)); err == nil && relative != "." { - return relative, true - } - return path, false - } - // Split on "\n" only and keep any trailing "\r" on each line, so a CRLF - // patch keeps its line endings everywhere except the rewritten headers. - lines := strings.Split(patch, "\n") - oldRemaining, newRemaining := 0, 0 - inHunk := false - changed := false - for index, rawLine := range lines { - line := strings.TrimSuffix(rawLine, "\r") - eol := rawLine[len(line):] - 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 "): - fields := strings.Fields(line) - if len(fields) >= 4 { - a, aok := relativize(fields[2]) - b, bok := relativize(fields[3]) - if aok || bok { - if aok { - fields[2] = "a/" + a - } - if bok { - fields[3] = "b/" + b - } - lines[index] = strings.Join(fields, " ") + eol - changed = true - } - } - case strings.HasPrefix(line, "@@"): - oldRemaining, newRemaining = parseHunkCounts(line) - inHunk = oldRemaining > 0 || newRemaining > 0 - case strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "): - if relative, ok := relativize(patchFileHeaderPath(line)); ok { - prefix := "a/" - if strings.HasPrefix(line, "+++ ") { - prefix = "b/" - } - lines[index] = line[:4] + prefix + relative + eol - changed = true - } - } - } - if !changed { - return patch - } - return strings.Join(lines, "\n") -} - func validatePatchPaths(root string, patch string) error { for _, path := range patchHeaderPaths(patch) { if path == "" || path == "/dev/null" { @@ -384,18 +132,6 @@ func validatePatchPaths(root string, patch string) error { return nil } -func recheckPatchWriteTargets(root string, patch string) error { - for _, path := range patchHeaderPaths(patch) { - if path == "" || path == "/dev/null" { - continue - } - if err := recheckWorkspaceWriteTarget(root, path); err != nil { - return err - } - } - return nil -} - // patchHeaderPaths returns the file paths declared in a unified diff's headers // (`diff --git` and `---`/`+++` lines). It tracks hunk state by counting body // lines from each `@@ -a,b +c,d @@` header, so a removed/added content line that @@ -460,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 index 0351e94c2..d7f338360 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strconv" "strings" "testing" ) @@ -147,38 +148,186 @@ func TestApplyPatchStillRejectsAbsolutePathsOutsideWorkspace(t *testing.T) { } } -func TestRelativizeUnifiedPatchPathsLeavesHunkBodiesAlone(t *testing.T) { +// 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() - absolute := filepath.Join(root, "notes.txt") + 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 " + absolute + " " + absolute, - "--- " + absolute, - "+++ " + absolute, + "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 @@", - "-see " + absolute, - "+see " + absolute + " (kept)", - " --- not a header", + " a", + "-b", + "+c", + "\\ No newline at end of file", "", }, "\n") - got := relativizeUnifiedPatchPaths(root, patch) - want := strings.Join([]string{ - "diff --git a/notes.txt b/notes.txt", - "--- a/notes.txt", - "+++ b/notes.txt", + 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 @@", - "-see " + absolute, - "+see " + absolute + " (kept)", - " --- not a header", + " hello", + "-old", + "+new", "", }, "\n") - if got != want { - t.Fatalf("relativized patch mismatch:\n%s\n--- want ---\n%s", got, want) + 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 plain := "--- a/x.go\n+++ b/x.go\n@@ -1 +1 @@\n-a\n+b\n"; relativizeUnifiedPatchPaths(root, plain) != plain { - t.Fatal("a patch without absolute paths must be returned unchanged") + if content, _ := os.ReadFile(filepath.Join(root, "y.txt")); string(content) != "hello\nnew\n" { + t.Fatalf("renamed content = %q", string(content)) } - crlf := "--- " + absolute + "\r\n+++ " + absolute + "\r\n@@ -1 +1 @@\r\n-a\r\n+b\r\n" - if got, want := relativizeUnifiedPatchPaths(root, crlf), "--- a/notes.txt\r\n+++ b/notes.txt\r\n@@ -1 +1 @@\r\n-a\r\n+b\r\n"; got != want { - t.Fatalf("CRLF patch must keep its line endings:\n%q\nwant\n%q", got, want) + 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) + } } } diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index c03c39d9f..9a139b6b0 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -30,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 { @@ -39,8 +42,20 @@ 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 } +type eofNewlineMode uint8 + +const ( + eofNewlineKeep eofNewlineMode = iota + eofNewlinePresent + eofNewlineAbsent +) + type structuredPatchChunk struct { context string hasContext bool @@ -48,6 +63,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 { @@ -102,6 +122,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()) @@ -114,7 +142,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) } } @@ -133,6 +161,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]) } } @@ -352,7 +384,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) @@ -366,12 +398,15 @@ 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 == 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) @@ -427,9 +462,12 @@ 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. 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) { @@ -443,7 +481,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")) } @@ -478,6 +522,34 @@ func applyStructuredPatchUpdate(content, path string, chunks []structuredPatchCh return updated, 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)) @@ -588,7 +660,7 @@ func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bo 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 diff --git a/internal/tools/unified_patch.go b/internal/tools/unified_patch.go new file mode 100644 index 000000000..1036434db --- /dev/null +++ b/internal/tools/unified_patch.go @@ -0,0 +1,279 @@ +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), 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") + + var operations []structuredPatchOperation + var current *structuredPatchOperation + var chunk *structuredPatchChunk + var added []string // collected "+" lines for a file creation + oldPath, newPath := "", "" + pendingFrom, pendingKind := "", structuredPatchUpdate + 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) + } + } + operations = append(operations, *current) + current, chunk, added = nil, nil, nil + return nil + } + startFile := func(line int) error { + 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": + op.kind, op.path = structuredPatchDelete, oldPath + 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) { + 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.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) + 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.eofNewline == eofNewlineKeep { + current.eofNewline = eofNewlinePresent + } + continue + case strings.HasPrefix(raw, "diff --git "), strings.HasPrefix(raw, "index "), + strings.HasPrefix(raw, "new file mode "), strings.HasPrefix(raw, "deleted file mode "), + 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) + } + oldRemaining, newRemaining = oldCount, newCount + inHunk = oldRemaining > 0 || newRemaining > 0 + lastSide = 0 + if current.kind == structuredPatchUpdate || current.kind == structuredPatchCopy { + 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) + } + } + 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 +} + +// 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 +} From 236620177e14072e493310187fb5ae7073921be6 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 14:13:13 +0800 Subject: [PATCH 07/15] fix(tools): harden unified-diff parsing and cover the copy operation Review follow-ups for #956 (in-process unified applier): - A hunk that ends before its declared line counts is reported as malformed instead of silently absorbing the next file's ---/+++ header pair (which then failed later against the wrong file). Both shapes are caught: leftover counts at a header or at end of input, and a --- line directly followed by +++ inside a hunk. The element after a patch's final newline is no longer counted as an empty context line, so a truncated hunk cannot look complete. - "\ No newline at end of file" may only follow a file's final hunk; a marker before another hunk is rejected rather than leaking into that hunk's result. - Zero-context insertions (git diff -U0) are documented as position-trusting by design, matching git apply without fuzz; hunks with context are verified at the range before it is used. Tests: copy from/to (source kept, destination created with the hunk applied and tracked with the source's whole-file state; existing destination refused untouched; copy onto itself refused), over-declared and truncated hunk counts, and the no-newline marker before vs after the final hunk. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/apply_patch_tolerance_test.go | 121 +++++++++++++++++++ internal/tools/structured_patch.go | 5 +- internal/tools/unified_patch.go | 27 +++++ 3 files changed, 152 insertions(+), 1 deletion(-) diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index d7f338360..a8e29d1f6 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -331,3 +331,124 @@ func TestUnifiedPatchRenameWithHunkAndRejectsBinary(t *testing.T) { } } } + +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)) + } +} diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 9a139b6b0..692e97dca 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -463,7 +463,10 @@ func applyStructuredPatchUpdate(content, path string, chunks []structuredPatchCh // 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. A unified diff names the - // insertion point in its range instead. + // 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.hasHint { start = max(chunk.hint, lineIndex) diff --git a/internal/tools/unified_patch.go b/internal/tools/unified_patch.go index 1036434db..0e58b568b 100644 --- a/internal/tools/unified_patch.go +++ b/internal/tools/unified_patch.go @@ -19,6 +19,11 @@ import ( 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 @@ -87,6 +92,13 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { for index, raw := range lines { lineNumber := index + 1 if inHunk && (oldRemaining > 0 || newRemaining > 0) { + // A "--- " line directly followed by "+++ " is the next file's + // header pair, not a removed line followed by an added one: an + // over-declared count would otherwise swallow both and the hunk + // would later fail against the wrong file. + if strings.HasPrefix(raw, "--- ") && index+1 < len(lines) && strings.HasPrefix(lines[index+1], "+++ ") { + 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. @@ -132,6 +144,12 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { } 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 @@ -192,6 +210,12 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { 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 @@ -212,6 +236,9 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { return nil, fmt.Errorf("invalid unified diff at line %d: unexpected %q", lineNumber, trimmed) } } + 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 := finish(); err != nil { return nil, err } From 08100a38b049a7916b0d83d6f92f8d103b76cd98 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 14:30:20 +0800 Subject: [PATCH 08/15] fix(tools): require an @@ line before treating ---/+++ inside a hunk as a file boundary A removed "-- x" line directly followed by an added "++ y" line is valid hunk content that also matches the ---/+++ header shape; the boundary check now additionally requires the following line to be a hunk header, and a regression test keeps such a pair intact. End-of-input strictness for a hunk cut short is documented as deliberate: applying a truncated hunk would make a partial change (removals without their replacements). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/apply_patch_tolerance_test.go | 18 ++++++++++++++++++ internal/tools/unified_patch.go | 16 +++++++++++----- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index a8e29d1f6..90683e2b8 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -452,3 +452,21 @@ func TestUnifiedPatchNoNewlineMarkerOnlyAfterFinalHunk(t *testing.T) { 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)) + } +} diff --git a/internal/tools/unified_patch.go b/internal/tools/unified_patch.go index 0e58b568b..b4a8e881e 100644 --- a/internal/tools/unified_patch.go +++ b/internal/tools/unified_patch.go @@ -92,11 +92,12 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { for index, raw := range lines { lineNumber := index + 1 if inHunk && (oldRemaining > 0 || newRemaining > 0) { - // A "--- " line directly followed by "+++ " is the next file's - // header pair, not a removed line followed by an added one: an - // over-declared count would otherwise swallow both and the hunk - // would later fail against the wrong file. - if strings.HasPrefix(raw, "--- ") && index+1 < len(lines) && strings.HasPrefix(lines[index+1], "+++ ") { + // 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 { @@ -236,6 +237,11 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { 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)) } From 3f6544772efe10ed247d83e29218c9913cbae90c Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 15:04:46 +0800 Subject: [PATCH 09/15] refactor(tools): share one line-count implementation between reader and writer sourceLineCount and trackedLineTotal computed the same count; keeping two copies invites exactly the reader/writer drift this PR fixed. The reader now delegates to trackedLineTotal, which carries the tracker contract. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/read_minified_file.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/internal/tools/read_minified_file.go b/internal/tools/read_minified_file.go index 57cca1b56..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 From 3e01c218543414250d475c44856587778c421704 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 16:16:20 +0800 Subject: [PATCH 10/15] fix(tools): verify a unified deletion's expected content before removing the file A unified diff's "+++ /dev/null" hunks state the content being removed, but the in-process applier dropped them and deleted unconditionally, so a stale deletion removed a file whose current content the patch never described. Unified deletions now keep their hunks, require at least one, and run them through the same verifier as updates: the hunks must match the current file and leave nothing behind, otherwise the deletion is refused and the file is untouched. A structured "*** Delete File" keeps its unconditional meaning. Hardening from the same review: every non-create change re-reads its source through the opened root immediately before commit and refuses to proceed if the bytes differ from what was planned. Tests: matching deletion, stale deletion (file byte-for-byte unchanged), partial deletion, header-only deletion, multi-hunk deletion with one stale hunk (nothing removed) and all-good hunks, structured delete unchanged, and a source changed between planning and commit for update and delete. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/apply_patch_tolerance_test.go | 108 +++++++++++++++++++ internal/tools/structured_patch.go | 27 +++++ internal/tools/unified_patch.go | 13 ++- 3 files changed, 145 insertions(+), 3 deletions(-) diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index 90683e2b8..9864e4390 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -470,3 +470,111 @@ func TestUnifiedPatchKeepsAdjacentDashPlusContentLines(t *testing.T) { 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, "leaves content behind") { + 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"} + 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}, + } { + 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)) + } + } +} diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 692e97dca..2cabf9fb6 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -46,6 +46,11 @@ type structuredPatchOperation struct { // 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): the chunks must remove the + // whole current file, otherwise the deletion is stale and refused. A + // structured "*** Delete File" means "delete this path" and has no chunks. + verifyDelete bool } type eofNewlineMode uint8 @@ -398,6 +403,15 @@ func planStructuredPatch(root *os.Root, operations []structuredPatchOperation, t return nil, fmt.Errorf("%s", fileConflictMessage(from.relative)) } change.before = string(content) + if operation.kind == structuredPatchDelete && operation.verifyDelete { + remaining, err := applyStructuredPatchUpdate(change.before, from.relative, operation.chunks) + if err != nil { + return nil, fmt.Errorf("deletion of %s does not match its current content: %w", from.relative, err) + } + if strings.TrimSpace(remaining) != "" { + return nil, fmt.Errorf("deletion of %s leaves content behind; the hunks must remove the whole file", from.relative) + } + } if operation.kind == structuredPatchUpdate || operation.kind == structuredPatchCopy { updated, err := applyStructuredPatchUpdate(change.before, from.relative, operation.chunks) if err != nil { @@ -657,6 +671,19 @@ func forgetStructuredPatchFiles(tracker *FileTracker, changes []structuredPatchC } func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bool, error) { + // 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 { diff --git a/internal/tools/unified_patch.go b/internal/tools/unified_patch.go index b4a8e881e..a145b06ba 100644 --- a/internal/tools/unified_patch.go +++ b/internal/tools/unified_patch.go @@ -11,7 +11,8 @@ import ( // instead of handing pathnames to git apply after validation. // // Supported: file modifications, creations (--- /dev/null) and deletions -// (+++ /dev/null), multiple hunks per file, "\ No newline at end of file" +// (+++ /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 @@ -54,6 +55,10 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { 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 @@ -78,7 +83,9 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { case oldPath == "/dev/null": op.kind, op.path = structuredPatchAdd, newPath case newPath == "/dev/null": - op.kind, op.path = structuredPatchDelete, oldPath + // 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 { @@ -220,7 +227,7 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { oldRemaining, newRemaining = oldCount, newCount inHunk = oldRemaining > 0 || newRemaining > 0 lastSide = 0 - if current.kind == structuredPatchUpdate || current.kind == structuredPatchCopy { + 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 From d0f1394b2b38d41447276e7bcb30d6255d42126f Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 16:18:09 +0800 Subject: [PATCH 11/15] test(tools): a move with a missing source is refused before publishing The pre-commit recheck now refuses a move whose source vanished after planning instead of publishing the destination and then failing; update the test to the safer contract (error names the recheck, nothing is created). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/write_tools_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index fc8c0c771..98d92d721 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 vanished 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 { @@ -883,14 +885,14 @@ func TestStructuredPatchMoveFailureLeavesPublishedDestination(t *testing.T) { } 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 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) } } From 03e2da108199c65e94d5f1599696ccbfeb9ce5e2 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 16:27:12 +0800 Subject: [PATCH 12/15] fix(tools): byte-exact unified deletions and git's header-only empty-file forms Review follow-ups for #956: - A unified deletion is verified byte for byte: the removed lines (with the removed side's "\ No newline at end of file" honoured) must equal the current file exactly, with only the file's own line-ending style normalised. None of the whitespace tolerance used to locate update hunks applies, and a whitespace-only remainder no longer passes; a deletion is not recoverable, so it fails closed. - git describes an empty file's deletion or creation with header lines only ("diff --git", "deleted file mode"/"new file mode", "index") and no hunks. Both forms are recognised: the deletion is applied only when the current file is empty, the creation writes an empty file, and they compose with hunk-carrying files in the same patch. - The pre-commit recheck test also covers the copy operation and asserts no destination is created on refusal. Tests: whitespace/indentation/blank-line/space-only/missing-newline mismatches all refused with the file unchanged, matching no-newline marker and CRLF file accepted, header-only deletion of a non-empty file refused, multi-file patch with header-only delete + create + a normal hunk. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/apply_patch_tolerance_test.go | 90 +++++++++++++++++++- internal/tools/structured_patch.go | 48 +++++++++-- internal/tools/unified_patch.go | 88 +++++++++++++++++-- 3 files changed, 209 insertions(+), 17 deletions(-) diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index 9864e4390..ad1e54df8 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -504,7 +504,7 @@ func TestUnifiedPatchDeletionVerifiesExpectedContent(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, "leaves content behind") { + 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" { @@ -564,9 +564,11 @@ func TestApplyStructuredPatchChangeRefusesSourceChangedAfterPlanning(t *testing. } 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) @@ -576,5 +578,91 @@ func TestApplyStructuredPatchChangeRefusesSourceChangedAfterPlanning(t *testing. 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) } } diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 2cabf9fb6..82402c653 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -47,10 +47,14 @@ type structuredPatchOperation struct { // the file's existing state. eofNewline eofNewlineMode // verifyDelete marks a deletion that carries the expected old content - // (a unified diff's "+++ /dev/null" hunks): the chunks must remove the - // whole current file, otherwise the deletion is stale and refused. A - // structured "*** Delete File" means "delete this path" and has no chunks. + // (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 @@ -404,12 +408,8 @@ func planStructuredPatch(root *os.Root, operations []structuredPatchOperation, t } change.before = string(content) if operation.kind == structuredPatchDelete && operation.verifyDelete { - remaining, err := applyStructuredPatchUpdate(change.before, from.relative, operation.chunks) - if err != nil { - return nil, fmt.Errorf("deletion of %s does not match its current content: %w", from.relative, err) - } - if strings.TrimSpace(remaining) != "" { - return nil, fmt.Errorf("deletion of %s leaves content behind; the hunks must remove the whole file", from.relative) + if err := verifyUnifiedDeletion(change.before, from.relative, operation); err != nil { + return nil, err } } if operation.kind == structuredPatchUpdate || operation.kind == structuredPatchCopy { @@ -539,6 +539,36 @@ 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) { diff --git a/internal/tools/unified_patch.go b/internal/tools/unified_patch.go index a145b06ba..ab09b3f1d 100644 --- a/internal/tools/unified_patch.go +++ b/internal/tools/unified_patch.go @@ -32,6 +32,9 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { 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 @@ -64,7 +67,28 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { 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) } @@ -112,8 +136,11 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { // "\ 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.eofNewline == eofNewlineKeep { - current.eofNewline = eofNewlinePresent + } else if current != nil && lastSide == '-' { + current.oldNoNewline = true + if current.eofNewline == eofNewlineKeep { + current.eofNewline = eofNewlinePresent + } } continue case strings.HasPrefix(raw, "-"): @@ -165,13 +192,26 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { // "\ 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.eofNewline == eofNewlineKeep { - current.eofNewline = eofNewlinePresent + } else if current != nil && lastSide == '-' { + current.oldNoNewline = true + if current.eofNewline == eofNewlineKeep { + current.eofNewline = eofNewlinePresent + } } continue - case strings.HasPrefix(raw, "diff --git "), strings.HasPrefix(raw, "index "), - strings.HasPrefix(raw, "new file mode "), strings.HasPrefix(raw, "deleted file mode "), - strings.HasPrefix(raw, "old mode "), strings.HasPrefix(raw, "new mode "): + 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 @@ -252,6 +292,9 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { 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 } @@ -261,6 +304,37 @@ func parseUnifiedPatch(patch string) ([]structuredPatchOperation, error) { 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) { From 5dd78adc4f702d2bd756f92aa85032c0d5afb7ae Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 16:29:44 +0800 Subject: [PATCH 13/15] test(tools): remove the move source after planning via a deterministic hook The refused-move test now creates source.txt, plans the move, and removes the file through a test-only pre-commit hook (structuredPatchBeforeCommit) so it exercises the post-planning disappearance path rather than a source that was never there. Assertions unchanged: refused before publishing, no destination created. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/structured_patch.go | 8 ++++++++ internal/tools/write_tools_test.go | 15 ++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 82402c653..2ac9d75af 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -700,7 +700,15 @@ 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 diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 98d92d721..87849e859 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -866,7 +866,7 @@ func TestStructuredPatchFailedDeleteDoesNotRecreateMissingFile(t *testing.T) { } } -// A move whose source vanished after planning is refused by the pre-commit +// 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() @@ -877,14 +877,27 @@ func TestStructuredPatchMoveWithMissingSourceIsRefusedBeforePublishing(t *testin 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 !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) } From 3454f1a33fdfc076f2a4eb3357f1b1c6a4a7358a Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 17:14:45 +0800 Subject: [PATCH 14/15] =?UTF-8?q?test(tui):=20read=5Ffile=20fixtures=20use?= =?UTF-8?q?=20the=20compact=20N=E2=86=92=20prefix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three TUI fixtures still built fake read_file output in the legacy " N | text" shape; they pass either way but pinned an outdated contract. They now mirror what appendReadFileRange emits, and the shape comment and the expanded-card assertion are updated. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tui/model_test.go | 2 +- internal/tui/rendering_lime_test.go | 6 +++--- internal/tui/tool_render_registry_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) 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"}, }, { From 4f016510076a441fa4a916430ce90353c004bc01 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Tue, 25 Aug 2026 19:46:16 +0800 Subject: [PATCH 15/15] fix(tools): name the committed files when a patch is partially applied Review follow-up for #956. When a later change in a multi-file patch fails (for example the pre-commit recheck refuses a file that changed after planning), the error now lists exactly which files were already committed and states that the rest are unchanged, instead of a generic "partially applied" warning. The model can then re-read only what actually changed. Test: a three-file structured patch whose second file is altered after planning fails with "already committed: first.txt", first.txt holds the patched content, and the other two files are untouched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq --- internal/tools/apply_patch_tolerance_test.go | 44 ++++++++++++++++++++ internal/tools/structured_patch.go | 28 ++++++++++--- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index ad1e54df8..2561d4eb7 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -666,3 +666,47 @@ func TestUnifiedPatchHeaderOnlyEmptyFileForms(t *testing.T) { 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/structured_patch.go b/internal/tools/structured_patch.go index 2ac9d75af..653f0cbb5 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -673,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 } @@ -688,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