From b74676ebe5dbd82cc03ed31061d5070720fcb466 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 00:51:57 +0700 Subject: [PATCH 01/20] Fix the format, not the matching edit_file grew 500 lines of similarity scoring, edit distance and prefix stripping to recover from anchors that do not match. Driven through the real tool, three of those recovery paths write to disk and report success: a space-indented anchor against a tab-indented file produces Python that will not compile, prefix stripping deletes real data out of new_string, and an anchor matching zero times rewrites every row under replace_all. All of it descends from one line in a commit about CRLF, which changed read_file's separator from a tab to a pipe. Both collide with real file content. This spec removes the separator instead. Co-authored-by: Cursor --- ...6-08-14-verbatim-read-exact-edit-design.md | 203 ++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-14-verbatim-read-exact-edit-design.md diff --git a/docs/superpowers/specs/2026-08-14-verbatim-read-exact-edit-design.md b/docs/superpowers/specs/2026-08-14-verbatim-read-exact-edit-design.md new file mode 100644 index 0000000..83cf2c2 --- /dev/null +++ b/docs/superpowers/specs/2026-08-14-verbatim-read-exact-edit-design.md @@ -0,0 +1,203 @@ +# Verbatim Read, Exact Edit + +## Summary + +`read_file` will return the file's bytes verbatim, with the line range stated +in a header rather than stamped onto every line. `edit_file` will go back to +writing only when `old_string` is present in the file exactly, and writing +exactly `new_string`. + +This removes roughly 500 lines of matching machinery that exists only to undo +the damage the line-number prefix causes. + +## The defect this fixes + +At `v0.2.0` `edit_file` was 49 lines with no helpers: count the occurrences, +refuse zero, refuse more than one without `replace_all`, replace, write. It +held two properties that everything since has eroded: + +1. It never wrote unless `old_string` was in the file byte for byte. +2. What it wrote was exactly `new_string`. + +Both are now violated, and the violations are silent. Driven through the real +tool against real files: + +``` +old_string = " if enabled:" against a tab-indented Python file + tool said : Edited app.py (1 replacement(s)) [matched unique near line for adjacent insertion] + on disk : "def main():\n\tif enabled:\n setup()\n\t\trun()\n\t\treturn 0\n" + python3 : TabError: inconsistent use of tabs and spaces in indentation, line 3 +``` + +`old_string` appears nowhere in that file. The tool wrote anyway, produced a +file that will not compile, and reported success. + +``` +old_string = "2|2|bob|user", new_string = "2|bob|admin" + tool said : Edited rows.psv (1 replacement(s)) [stripped read_file NUMBER| prefixes] + on disk : "1|alice|admin\nbob|admin\n" +``` + +The row's id was deleted, because prefix stripping is applied to `new_string` +— which the model authors and never copies from `read_file`. + +``` +old_string = "9|pending", replace_all = true, against three rows numbered 1..3 + tool said : Edited queue.psv (3 replacement(s)) [stripped read_file NUMBER| prefixes] + on disk : "1|done\n2|done\n3|done\n" +``` + +An anchor matching zero times rewrote every row. + +``` +old_string = "9|pending", replace_all = false + tool said : old_string appears 3 times in queue.psv; add unique surrounding + context ... Current match line(s): 1, 2, 3. +``` + +That string appears zero times. The advice cannot succeed, because the problem +is the anchor, not the context — which is what a retry loop looks like from the +inside. + +## Root cause + +Commit `d6762c7`, titled "fix(tools): preserve exact file editing", changed one +line: + +```diff +- fmt.Fprintf(&b, "%6d\t%s\n", i+1, lines[i]) ++ fmt.Fprintf(&b, "%d|%s\n", i+1, lines[i]) +``` + +The original separator was a tab, and tab-indented source starts with a tab, so +the boundary between prefix and content was ambiguous — and the number's +padding was spaces, so a model copying what it saw produced spaces where the +file had tabs. That is the whitespace ambiguity behind the original patch +failures. + +The change swapped one colliding separator for another. Pipes occur legitimately +at the start of a line in markdown tables and pipe-separated data, so +`read_file` now emits lines like `3|| Date | Event |`, and the prompt's +instruction that "the `|` is metadata only" is true of the first pipe only. + +Every helper added since is an attempt to recover from that collision at +match time: `stripReadFileLinePrefixes`, `resolveEditMatch`'s staged fallbacks, +and the `spliceAdjacentInsertion` family with its similarity, token-set, +edit-distance and digit gates. Three separate commits each fixed a silent +corruption in that family. The cure was applied to the matching layer; the +disease is in the format. + +## Goals + +- What `read_file` shows is what the file contains, with nothing to strip. +- `edit_file` writes only on an exact match, and writes exactly what it was given. +- A failed edit produces an error that names the real reason and a next step + that can work. +- `grep` and `read_file` agree on line numbers. +- The prompt describes what the code does. + +## Non-goals + +- Fuzzy, approximate, or "close enough" matching in any form. +- Changing `write_file`, `list_files`, or path resolution. +- Changing the checkpoint or approval behaviour of `edit_file`. +- Preserving the `NUMBER|CONTENT` format for compatibility. Nothing outside the + file tools, their tests, the prompt and `docs/tools.md` consumes it. + +## `read_file` + +Content is returned verbatim: the file's bytes, unmodified, including CRLF and +tabs. The only additions are a header line and, when the read was clipped, the +existing continuation note. + +``` +app.py — lines 1-40 of 120 + +def main(): + if enabled: + run() +``` + +The header is exactly ` — lines - of `, +followed by one blank line, then the content. When the whole file fits it still +appears, so the shape never varies. The same numbers are carried in `Meta` as +`path`, `first_line`, `last_line` and `total_lines`, so callers that want them +do not parse prose. + +Two current behaviours are kept, because they protect against something real +rather than guessing at intent: the 400 KB cap with its explicit notice, and +the rejection of content that is not valid UTF-8. The cap continues to trim on +a rune boundary. + +One behaviour is dropped: the current code replaces lone `\r` with `\n` for +display. Verbatim means verbatim, and `edit_file` now matches what was shown. + +## `edit_file` + +The contract returns to `v0.2.0`: count exact occurrences of `old_string`; +zero is an error; more than one without `replace_all` is an error; otherwise +replace and write exactly `new_string`. + +Deleted entirely: `spliceAdjacentInsertion`, `editLineSimilarity`, +`editTokenSet`, `nearEditDistance`, `digitsOfLine`, and +`stripReadFileLinePrefixes`. With verbatim reads there is no prefix to strip +and no reason to guess which line was meant. + +Kept, because the CRLF recovery below needs them: `fileEOL`, `eolOf` and +`toEOL`. `resolveEditMatch` survives as well, reduced to two stages — verbatim, +then the CRLF retry — with its staged prefix-stripping fallbacks removed. +`lineSpans` stays and becomes the shared line splitter described under "Line +numbering". + +### The one recovery that stays + +If the file uses CRLF and `old_string` does not match, the tool retries with +`old_string` translated from LF to CRLF. This is not a guess: it is a lossless +normalisation of a well-defined ambiguity, since a model may emit `\n` for a +line break regardless of what it read. + +Three rules bound it. It runs only after the verbatim match has already failed. +`new_string` is translated only when `old_string` had to be, so a replacement is +never rewritten on a path that matched exactly. And the result message says the +translation happened, so the caller is never told "exact" when it was not. + +### Errors + +The existing diagnostics are good and are kept — the near-miss hint, the +occurrence line numbers, and above all the message that names a tab-versus-space +mismatch, which today is unreachable because the splice fires first and reports +success instead. + +One rule is added: every count and every line number in an error must describe +the string the caller actually sent. Reporting occurrences of a rewritten anchor +is what turns one failed edit into a loop. + +## Line numbering + +`read_file`'s header, `grep`'s match lines, and `edit_file`'s occurrence lines +must count the same way. Today they do not: `read_file` splits on `\n` and so +reports a phantom trailing line for a newline-terminated file, while `grep`'s +scanner does not, and the two disagree entirely on a file terminated with lone +`\r`. A single shared splitter serves all three. + +## Prompt + +The tool notes are rewritten to match the code. The instruction to strip a +`NUMBER|` prefix goes, since there is no prefix. The claim that `edit_file` +"requires an exact, unique old_string" stays, and becomes true. The advice to +re-read before editing stays, because it is sound. `docs/tools.md` is updated +in the same pass. + +## Testing + +The four cases in the "defect" section above are the acceptance criteria. They +are written as tests that drive the real tool against real files on disk, and +they must fail before the change and pass after. + +Beyond those: a verbatim round trip on a file containing tabs, CRLF, a lone +`\r`, and a markdown table, asserting that text copied out of `read_file` +matches with `edit_file` unchanged; the CRLF recovery, asserting both that it +works and that the message discloses it; that `new_string` is written byte for +byte on both the exact and the recovery path; and that `read_file`, `grep` and +`edit_file` report the same line number for the same line across all four line +terminators. From 5f9d7d1c61522d509a0249d194f7a6bd94a8b9e6 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 00:53:00 +0700 Subject: [PATCH 02/20] Plan the return to verbatim reads and exact edits Four tasks: give the file tools one idea of what a line is, remove the line-number prefix from read_file's output, delete the machinery that existed only to recover from that prefix, and make the prompt describe the result. The four acceptance tests are committed red on purpose. Each was observed driving the real tool into writing a file it should have refused. Co-authored-by: Cursor --- .../2026-08-14-verbatim-read-exact-edit.md | 203 ++++++++++++++++++ .../tools/file_exactness_acceptance_test.go | 102 +++++++++ 2 files changed, 305 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-14-verbatim-read-exact-edit.md create mode 100644 internal/tools/file_exactness_acceptance_test.go diff --git a/docs/superpowers/plans/2026-08-14-verbatim-read-exact-edit.md b/docs/superpowers/plans/2026-08-14-verbatim-read-exact-edit.md new file mode 100644 index 0000000..b9a5733 --- /dev/null +++ b/docs/superpowers/plans/2026-08-14-verbatim-read-exact-edit.md @@ -0,0 +1,203 @@ +# Verbatim Read, Exact Edit Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** `read_file` returns the file's bytes verbatim with the line range in a header, and `edit_file` writes only on an exact match and writes exactly what it was given. + +**Architecture:** The line-number prefix is the disease and the ~500 lines of similarity scoring, edit distance and prefix stripping are the symptom. Task 1 gives the three tools one shared idea of what a line is. Task 2 removes the prefix. Task 3 deletes the machinery that existed to undo it. Task 4 makes the prompt and docs describe the result. + +**Tech Stack:** Go, standard library only. Tests drive the real tools against real files in `t.TempDir()`. + +## Global Constraints + +- Design spec: `docs/superpowers/specs/2026-08-14-verbatim-read-exact-edit-design.md`. Its wording governs; where this plan and the spec disagree, stop and ask. +- Repository: worktree `/home/nvdorman/antares/.worktrees/exact-editing`, branch `fix/verbatim-read-exact-edit`, based on `refactor/harness-tool-calls` (upstream main plus the tool-path determinism work, open as PR #30). +- `internal/tools/file_exactness_acceptance_test.go` holds four acceptance tests. Three are red and Task 3 turns them green; `TestEditWritesNewStringVerbatim` is already green and must stay green. Do not weaken, skip or delete that file. +- `read_file`'s header is exactly ` — lines - of `, then one blank line, then content. It appears even when the whole file fits. +- `Meta` carries `path`, `first_line`, `last_line`, `total_lines`. +- No fuzzy, approximate or "close enough" matching may be introduced, in any form, for any reason. +- No new third-party dependency; standard library only. +- No test may require network access, an API key, or a running daemon. +- `gofmt -l` must print nothing for the files you touch. Twenty-nine files are already unformatted upstream; leave them alone. +- Never run `git checkout --` in the worktree to undo an experiment; copy to /tmp instead. +- Run the full test suite for every package you touch. `internal/llm` holds opt-in live tests, so run the repository as `env -u OPENAI_API_KEY -u ANTHROPIC_API_KEY -u GEMINI_API_KEY go test ./... -count=1 -p 4`. + +--- + +### Task 1: One idea of what a line is + +**Files:** +- Modify: `internal/tools/file.go` (`lineSpans` ~564, `readFileTool.Execute` ~193-202) +- Modify: `internal/tools/search.go` (`grepTool.Execute`, the `bufio.Scanner` loop ~236-279) +- Test: `internal/tools/line_numbering_test.go` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: `lineSpans(content string) []lineSpan` becomes the single splitter used by `read_file`, `grep` and `edit_file`'s occurrence reporting. A file ending in a line terminator yields no trailing empty span. LF, CRLF and lone CR each terminate a line. + +- [ ] **Step 1: Write the failing test** + +Drive the real tools. For each of four files — LF, CRLF, lone CR, and no trailing terminator — put a unique token on the third line, then assert `read_file`'s reported total, `grep`'s reported match line, and `edit_file`'s occurrence line all say 3. + +```go +func TestLineNumbersAgreeAcrossTools(t *testing.T) { + for _, tc := range []struct{ name, eol string }{ + {"lf", "\n"}, {"crlf", "\r\n"}, {"cr", "\r"}, + } { + t.Run(tc.name, func(t *testing.T) { + body := "alpha" + tc.eol + "beta" + tc.eol + "NEEDLE" + tc.eol + // read_file's header must say "of 3", grep must report line 3. + }) + } +} +``` + +- [ ] **Step 2: Run it and confirm it fails** + +Run: `go test ./internal/tools/ -run TestLineNumbersAgree -v` +Expected: the LF case reports 4 total lines from `read_file` (a phantom trailing line) while `grep` says 3, and the CR case disagrees entirely. + +- [ ] **Step 3: Make `lineSpans` the shared splitter** + +Give `read_file` and `grep` their line boundaries from `lineSpans`. Drop the trailing empty element for a terminator-ended file. + +- [ ] **Step 4: Run the test and the package** + +Run: `go test ./internal/tools/ -count=1` +Expected: the new test passes; existing tests still pass. + +- [ ] **Step 5: Commit** + +```bash +gofmt -l internal/tools/file.go internal/tools/search.go internal/tools/line_numbering_test.go +git add -A && git commit -m "Count lines the same way in every file tool" +``` + +--- + +### Task 2: read_file returns the file + +**Files:** +- Modify: `internal/tools/file.go` (`readFileTool.Description` ~143, `Execute` ~154-231) +- Modify: `internal/tools/file_edit_regression_test.go` (tests that parse `NUMBER|`) +- Test: `internal/tools/read_verbatim_test.go` (create) + +**Interfaces:** +- Consumes: the shared splitter from Task 1. +- Produces: `read_file` content is `header + "\n\n" + verbatim bytes of the selected range`, plus the existing continuation and truncation notes. `Meta` gains `first_line`, `last_line`, `total_lines`; `lines` is removed. + +- [ ] **Step 1: Write the failing test** + +A file containing a tab-indented line, a CRLF line, and a markdown table row beginning with `|`. Assert that the bytes after the header and blank line are byte-identical to the file, and that a substring copied out of that region is found by `strings.Contains` on the original. + +- [ ] **Step 2: Run it and confirm it fails** + +Run: `go test ./internal/tools/ -run TestReadFileReturnsVerbatim -v` +Expected: fails — every line carries a `NUMBER|` prefix and CRLF has been normalised to LF. + +- [ ] **Step 3: Implement** + +Slice the file by the selected spans and emit those bytes unchanged. Build the header from the range. Keep the 400 KB cap, its rune-boundary trim, its notice, the binary rejection, and the "more lines" continuation note. Remove the lone-CR-to-LF display substitution. Update the tool description to describe verbatim output. + +- [ ] **Step 4: Run the test and the package** + +Run: `go test ./internal/tools/ -count=1` +Expected: the new test passes. Existing tests that parse `NUMBER|` will fail; update them to the new format in this task, but do not weaken what they assert. + +- [ ] **Step 5: Commit** + +```bash +git add -A && git commit -m "Show the file, not a rendering of it" +``` + +--- + +### Task 3: edit_file writes only what it was given + +**Files:** +- Modify: `internal/tools/file.go` (`editFileTool.Execute` ~329-397, `resolveEditMatch` ~506, and the helper block ~610-983) +- Modify: `internal/tools/file_edit_recovery_test.go`, `internal/tools/file_edit_regression_test.go` +- Test: `internal/tools/edit_exact_test.go` (create) + +**Interfaces:** +- Consumes: Tasks 1 and 2. +- Produces: `resolveEditMatch(content, oldIn, newIn) (oldString, newString string, count int, how string)` keeps its signature but has two stages only — verbatim, then a CRLF retry. `how` is empty for a verbatim match and names the translation otherwise. + +- [ ] **Step 1: Write the failing tests** + +Beyond the four acceptance tests already in the tree: that `new_string` is written byte for byte on both stages; that the CRLF recovery works and its message discloses the translation; that an exact match reports no `how` suffix; and that a tab-versus-space anchor produces the tab diagnostic rather than a write. + +- [ ] **Step 2: Run them and confirm they fail** + +Run: `go test ./internal/tools/ -run 'TestEdit' -v` +Expected: the three known-red acceptance tests fail as recorded in the spec, and the tab-diagnostic test fails because the splice reports success first. + +- [ ] **Step 3: Delete the guessing machinery** + +Remove `spliceAdjacentInsertion`, `editLineSimilarity`, `editTokenSet`, `nearEditDistance`, `digitsOfLine` and `stripReadFileLinePrefixes`, along with the branch in `Execute` that calls the splice and the prefix stages in `resolveEditMatch`. Remove `readFileLinePrefixCounts` and any diagnostic text that refers to `NUMBER|`. Keep `fileEOL`, `eolOf`, `toEOL`, `lineSpans`, `editNotFoundMessage`, `editAmbiguousMessage`, `occurrenceLines`, `nearMissHint`, `identifierTokens` and `expandTabs`. + +- [ ] **Step 4: Bound the CRLF recovery** + +It runs only after the verbatim match fails. `new_string` is translated only when `old_string` was. The result message says so. + +- [ ] **Step 5: Make errors describe the caller's anchor** + +Every count and line number in `editNotFoundMessage` and `editAmbiguousMessage` must refer to the string the caller sent. + +- [ ] **Step 6: Run everything** + +Run: `go test ./internal/tools/ -count=1` +Expected: all four acceptance tests green, package green. Delete or rewrite recovery tests that assert the deleted behaviour — say in your report which ones and why each no longer describes something true. + +- [ ] **Step 7: Commit** + +```bash +git add -A && git commit -m "Write only what was asked, only where it matches" +``` + +--- + +### Task 4: Say what the tools do + +**Files:** +- Modify: `internal/agent/prompt.go` (tool notes ~105-111) +- Modify: `docs/tools.md` +- Test: `internal/agent/prompt_file_notes_test.go` (create) + +**Interfaces:** +- Consumes: Tasks 2 and 3. +- Produces: no code behaviour; the prompt and docs match the tools. + +- [ ] **Step 1: Write the failing test** + +Assert the assembled prompt no longer mentions `NUMBER|` or instructs stripping, and does still tell the model to re-read before editing and to preserve tabs. + +- [ ] **Step 2: Run it and confirm it fails** + +Run: `go test ./internal/agent/ -run TestPromptFileNotes -v` +Expected: fails on the `NUMBER|` instruction still being present. + +- [ ] **Step 3: Rewrite the notes** + +Drop the stripping instruction. Keep "re-read the region before editing" and "preserve tabs and spaces exactly". State that `edit_file` matches exactly and that a failed match means the anchor is wrong, not that more context is needed. Update `docs/tools.md` to match. + +- [ ] **Step 4: Run the affected packages** + +Run: `go test ./internal/agent/ ./internal/tools/ -count=1` + +- [ ] **Step 5: Commit** + +```bash +git add -A && git commit -m "Describe the file tools as they now behave" +``` + +--- + +## Final verification + +```bash +env -u OPENAI_API_KEY -u ANTHROPIC_API_KEY -u GEMINI_API_KEY go test ./... -count=1 -p 4 +go vet ./... +``` + +All four acceptance tests green, and `internal/tools/file.go` should be close to half its current size. diff --git a/internal/tools/file_exactness_acceptance_test.go b/internal/tools/file_exactness_acceptance_test.go new file mode 100644 index 0000000..7c51272 --- /dev/null +++ b/internal/tools/file_exactness_acceptance_test.go @@ -0,0 +1,102 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// These four cases drive the real edit_file tool against real files. Each one +// was observed writing to disk and reporting success against an anchor that is +// not in the file, or writing something other than what it was given. They are +// the acceptance criteria for restoring the two properties edit_file lost: +// write only on an exact match, and write exactly new_string. + +func editOnDisk(t *testing.T, name, original string, args map[string]any) (said string, isError bool, after string) { + t.Helper() + workspace := t.TempDir() + path := filepath.Join(workspace, name) + if err := os.WriteFile(path, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + raw, err := json.Marshal(args) + if err != nil { + t.Fatal(err) + } + res := (editFileTool{}).Execute(context.Background(), Input{Args: raw, Workspace: workspace}) + written, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return res.Content, res.IsError, string(written) +} + +// A space-indented anchor against a tab-indented file is the most common way an +// edit goes wrong, and the anchor is nowhere in the file. Splicing it in +// produced Python that fails to compile with TabError. +func TestEditRefusesAnAnchorThatIsNotInTheFile(t *testing.T) { + original := "def main():\n\tif enabled:\n\t\trun()\n\t\treturn 0\n" + said, isError, after := editOnDisk(t, "app.py", original, map[string]any{ + "path": "app.py", + "old_string": " if enabled:", + "new_string": " if enabled:\n setup()", + }) + if after != original { + t.Errorf("file changed though old_string is absent\nsaid: %s\ngot: %q", said, after) + } + if !isError { + t.Errorf("reported success for an anchor that does not exist: %s", said) + } +} + +// new_string is authored by the model and never copied out of read_file, so a +// leading NUMBER| in it is content, not a prefix to strip. +func TestEditWritesNewStringVerbatim(t *testing.T) { + original := "1|alice|admin\n2|bob|user\n" + said, isError, after := editOnDisk(t, "rows.psv", original, map[string]any{ + "path": "rows.psv", + "old_string": "2|bob|user", + "new_string": "2|bob|admin", + }) + if isError { + t.Fatalf("exact anchor was refused: %s", said) + } + if want := "1|alice|admin\n2|bob|admin\n"; after != want { + t.Errorf("new_string was rewritten before it landed\nwant %q\ngot %q", want, after) + } +} + +// replace_all means every exact occurrence. An anchor occurring zero times must +// change nothing, however it might otherwise be rewritten to match. +func TestEditReplaceAllNeedsExactOccurrences(t *testing.T) { + original := "1|pending\n2|pending\n3|pending\n" + said, _, after := editOnDisk(t, "queue.psv", original, map[string]any{ + "path": "queue.psv", + "old_string": "9|pending", + "new_string": "9|done", + "replace_all": true, + }) + if after != original { + t.Errorf("an anchor matching zero times rewrote the file\nsaid: %s\ngot: %q", said, after) + } +} + +// An error has to describe the string the caller sent. Reporting occurrences of +// a silently rewritten anchor sends the model into a retry that cannot succeed. +func TestEditErrorDescribesTheCallersAnchor(t *testing.T) { + original := "1|pending\n2|pending\n3|pending\n" + said, isError, _ := editOnDisk(t, "queue.psv", original, map[string]any{ + "path": "queue.psv", + "old_string": "9|pending", + "new_string": "9|done", + }) + if !isError { + t.Fatalf("expected an error for an anchor appearing zero times: %s", said) + } + if strings.Contains(said, "appears 3 times") { + t.Errorf("error counts occurrences of a string the caller never sent: %s", said) + } +} From 4a7a8df0a8c1df161c068b6fb472802b48920fb0 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 00:59:39 +0700 Subject: [PATCH 03/20] Count lines the same way in every file tool read_file split on \n and so reported a phantom trailing line for any file ending in a newline, grep counted with a bufio.Scanner that did not, and on a file terminated with lone \r the two disagreed entirely. A line number one tool reported meant a different line in the next. lineSpans is now the only place a line boundary is decided. read_file's numbering and total, grep's match and context lines, and edit_file's occurrence and near-miss lines all come from it. grep reads the file whole, because telling a lone CR terminator from a CR byte inside a line is a property of the file rather than of any one line. The size gate moves to where the read happens, so a file named directly on the command is bounded by it too. Co-authored-by: Cursor --- internal/tools/file.go | 44 ++++-- internal/tools/grep_skip_test.go | 17 +++ internal/tools/line_numbering_test.go | 209 ++++++++++++++++++++++++++ internal/tools/search.go | 40 ++--- internal/tools/search_test.go | 17 ++- 5 files changed, 288 insertions(+), 39 deletions(-) create mode 100644 internal/tools/line_numbering_test.go diff --git a/internal/tools/file.go b/internal/tools/file.go index d3abaab..5f327ae 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -190,16 +190,8 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { return Errorf("%s appears to be a binary file (%d bytes)", args.Path, fi.Size()) } - normalized := strings.ReplaceAll(string(data), "\r", "\n") - if fileEOL(string(data)) != "\r" { - // Only a genuinely CR-terminated (classic Mac) file splits on a lone - // CR. In an LF or CRLF file a bare CR is data — a control character - // inside a string literal, say — and splitting on it would number the - // displayed lines differently from the file's real lines, handing the - // model an anchor that edit_file then cannot find. - normalized = strings.ReplaceAll(string(data), "\r\n", "\n") - } - lines := strings.Split(normalized, "\n") + content := string(data) + lines := lineSpans(content) offset := args.Offset if offset <= 0 { offset = 1 @@ -209,7 +201,9 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { limit = 2000 } start := offset - 1 - if start > len(lines) { + // An empty file has no line 1 to be past, so offset 1 on it reads as + // "nothing here" rather than as a mistake. + if start > 0 && start >= len(lines) { return Errorf("offset %d is past end of file (%d lines)", offset, len(lines)) } end := start + limit @@ -219,7 +213,7 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { var b strings.Builder for i := start; i < end; i++ { - fmt.Fprintf(&b, "%d|%s\n", i+1, lines[i]) + fmt.Fprintf(&b, "%d|%s\n", i+1, content[lines[i].start:lines[i].end]) } if end < len(lines) { fmt.Fprintf(&b, "\n… %d more lines (use offset=%d to continue)\n", len(lines)-end, end+1) @@ -561,6 +555,12 @@ func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string // content, excluding its \n, \r\n, or lone \r terminator. type lineSpan struct{ start, end int } +// lineSpans is the one place the file tools decide where a line begins and +// ends. read_file's numbering and total, grep's match lines and edit_file's +// occurrence lines all come from it, so a line number one tool reports means +// the same line in the next. A line the file terminates is complete: the +// terminator adds no empty line after it, so "a\nb\n" is two lines. Line +// numbers are the 1-based index into the returned slice. func lineSpans(content string) []lineSpan { // A lone CR terminates a line only in a genuinely CR-based file. Anywhere // else it is data, and treating it as a break here would number lines @@ -877,6 +877,7 @@ func occurrenceLines(content, needle string, max int) []int { if needle == "" || max <= 0 { return nil } + spans := lineSpans(content) var lines []int for from := 0; from < len(content) && len(lines) < max; { i := strings.Index(content[from:], needle) @@ -884,22 +885,37 @@ func occurrenceLines(content, needle string, max int) []int { break } at := from + i - lines = append(lines, 1+strings.Count(content[:at], "\n")) + lines = append(lines, lineOfOffset(spans, at)) from = at + len(needle) } return lines } +// lineOfOffset returns the 1-based number of the line containing byte offset +// at. An offset inside a terminator belongs to the line that terminator ends. +func lineOfOffset(spans []lineSpan, at int) int { + if len(spans) == 0 { + return 1 + } + next := sort.Search(len(spans), func(i int) bool { return spans[i].start > at }) + if next == 0 { + return 1 + } + return next +} + // nearMissHint reports a few real lines sharing a distinctive identifier with // old_string. It is intentionally short and bounded: the tool should correct // the model's stale context without dumping the file into an error response. func nearMissHint(content, oldString string) string { + spans := lineSpans(content) for _, token := range identifierTokens(oldString) { if len(token) < 8 || strings.Contains(strings.ToLower(token), "read_file") { continue } var hits []string - for i, line := range strings.Split(content, "\n") { + for i, sp := range spans { + line := content[sp.start:sp.end] if strings.Contains(line, token) { line = strings.TrimRight(line, "\r") if len(line) > 180 { diff --git a/internal/tools/grep_skip_test.go b/internal/tools/grep_skip_test.go index ad3bddf..4e1802d 100644 --- a/internal/tools/grep_skip_test.go +++ b/internal/tools/grep_skip_test.go @@ -29,6 +29,23 @@ func TestGrepReportsSkippedFiles(t *testing.T) { } }) + t.Run("a file named directly is gated by the same size", func(t *testing.T) { + workspace := t.TempDir() + writeSparseFile(t, filepath.Join(workspace, "huge.log"), "NEEDLE_TOKEN\n", 9*1024*1024) + + args, err := json.Marshal(map[string]any{"pattern": "NEEDLE_TOKEN", "path": "huge.log"}) + if err != nil { + t.Fatal(err) + } + result := (grepTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("grep errored: %s", result.Content) + } + if !strings.Contains(result.Content, "not searched") { + t.Errorf("a directly named file above the cap was silently unsearched: %q", result.Content) + } + }) + t.Run("a run that skipped nothing says nothing", func(t *testing.T) { workspace := t.TempDir() if err := os.WriteFile(filepath.Join(workspace, "small.log"), []byte("NEEDLE_TOKEN here\n"), 0o644); err != nil { diff --git a/internal/tools/line_numbering_test.go b/internal/tools/line_numbering_test.go new file mode 100644 index 0000000..93f6ee4 --- /dev/null +++ b/internal/tools/line_numbering_test.go @@ -0,0 +1,209 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// A line number is only worth reporting if every tool means the same thing by +// it. The agent reads a file, is handed a line number, and then greps or edits +// against it; when the tools count differently that number points at the wrong +// line, or at a line that does not exist. +// +// Each file here holds three lines, with a token on the third. read_file's +// total, grep's match line and edit_file's occurrence lines must all say 3, +// whichever sequence terminates the lines and whether or not the last line is +// terminated at all. +func TestLineNumbersAgreeAcrossTools(t *testing.T) { + for _, tc := range []struct{ name, eol, tail string }{ + {"lf", "\n", "\n"}, + {"crlf", "\r\n", "\r\n"}, + {"cr", "\r", "\r"}, + {"no trailing terminator", "\n", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + // The token twice on one line, so a single ambiguous edit_file call + // reports occurrence lines without a second line to confuse them. + body := "alpha" + tc.eol + "beta" + tc.eol + "NEEDLE NEEDLE" + tc.tail + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "sample.txt"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + if got := readFileTotalLines(t, workspace, "sample.txt"); got != 3 { + t.Errorf("read_file reports %d lines, want 3", got) + } + if got := grepMatchLines(t, workspace, "NEEDLE"); len(got) != 1 || got[0] != 3 { + t.Errorf("grep reports match line(s) %v, want [3]", got) + } + if got := editOccurrenceLines(t, workspace, "sample.txt", "NEEDLE"); len(got) == 0 { + t.Error("edit_file reported no occurrence lines for an ambiguous anchor") + } else { + for _, line := range got { + if line != 3 { + t.Errorf("edit_file reports occurrence line(s) %v, want every one to be 3", got) + break + } + } + } + }) + } +} + +// readFileTotalLines returns the total read_file reports for a whole-file read. +// The total comes from Meta so this stays a test about counting rather than +// about how the content happens to be rendered. +func readFileTotalLines(t *testing.T, workspace, name string) int { + t.Helper() + args, err := json.Marshal(map[string]any{"path": name}) + if err != nil { + t.Fatal(err) + } + res := (readFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if res.IsError { + t.Fatalf("read_file failed: %s", res.Content) + } + for _, key := range []string{"total_lines", "lines"} { + if v, ok := res.Meta[key]; ok { + switch n := v.(type) { + case int: + return n + case float64: + return int(n) + } + t.Fatalf("read_file Meta[%q] = %v (%T), want a number", key, v, v) + } + } + t.Fatalf("read_file reported no line total in Meta: %v", res.Meta) + return 0 +} + +var grepLinePrefix = regexp.MustCompile(`(?m)^\s*(\d+):\t`) + +// grepMatchLines returns the line numbers grep printed for its matches. +func grepMatchLines(t *testing.T, workspace, pattern string) []int { + t.Helper() + args, err := json.Marshal(map[string]any{"pattern": pattern, "path": "."}) + if err != nil { + t.Fatal(err) + } + res := (grepTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if res.IsError { + t.Fatalf("grep failed: %s", res.Content) + } + var lines []int + for _, m := range grepLinePrefix.FindAllStringSubmatch(res.Content, -1) { + n, err := strconv.Atoi(m[1]) + if err != nil { + t.Fatalf("grep printed a line number that will not parse: %q", m[1]) + } + lines = append(lines, n) + } + if len(lines) == 0 { + t.Fatalf("grep found nothing to report a line number for: %q", res.Content) + } + return lines +} + +var editOccurrenceList = regexp.MustCompile(`Current match line\(s\): ([\d, ]+)\.`) + +// editOccurrenceLines returns the line numbers edit_file names when it refuses +// an anchor for appearing more than once, which is the only path that reports +// them. +func editOccurrenceLines(t *testing.T, workspace, name, oldString string) []int { + t.Helper() + args, err := json.Marshal(map[string]any{ + "path": name, "old_string": oldString, "new_string": oldString + "_EDITED", + }) + if err != nil { + t.Fatal(err) + } + res := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !res.IsError { + t.Fatalf("edit_file accepted an anchor that appears twice: %s", res.Content) + } + m := editOccurrenceList.FindStringSubmatch(res.Content) + if m == nil { + t.Fatalf("edit_file named no occurrence lines: %s", res.Content) + } + var lines []int + for _, field := range strings.Split(m[1], ",") { + n, err := strconv.Atoi(strings.TrimSpace(field)) + if err != nil { + t.Fatalf("edit_file printed a line number that will not parse: %q", field) + } + lines = append(lines, n) + } + return lines +} + +// lineSpans is the one splitter the file tools count with, so its own rules are +// worth stating directly: a terminated last line adds no empty line after it, +// and 1-based numbering runs to exactly the number of lines the file has. +func TestLineSpansCountsTerminatedAndUnterminatedFilesAlike(t *testing.T) { + for _, tc := range []struct { + name string + content string + want int + }{ + {"empty file", "", 0}, + {"lf terminated", "a\nb\n", 2}, + {"lf unterminated", "a\nb", 2}, + {"crlf terminated", "a\r\nb\r\n", 2}, + {"cr terminated", "a\rb\r", 2}, + {"blank line before the end", "a\n\n", 2}, + {"lone cr is data in an lf file", "a\rb\nc\n", 2}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := len(lineSpans(tc.content)); got != tc.want { + t.Errorf("lineSpans(%q) = %d lines, want %d", tc.content, got, tc.want) + } + }) + } +} + +// grep's context lines carry line numbers too, and they are counted backwards +// from the match rather than tracked as they are read. They come from the same +// splitter as the match itself, so they have to stay in step with it. +func TestGrepNumbersContextLinesFromTheSameSplitter(t *testing.T) { + workspace := t.TempDir() + body := "alpha\r\nbeta\r\nNEEDLE\r\ndelta\r\n" + if err := os.WriteFile(filepath.Join(workspace, "sample.txt"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"pattern": "NEEDLE", "path": ".", "context": 1}) + res := (grepTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if res.IsError { + t.Fatalf("grep failed: %s", res.Content) + } + for _, want := range []string{" 2-\tbeta", " 3:\tNEEDLE", " 4-\tdelta"} { + if !strings.Contains(res.Content, want) { + t.Errorf("grep did not report %q: %q", want, res.Content) + } + } +} + +// An offset past the last line is a mistake worth naming. Returning nothing at +// all reads as "the file is empty from here", which is a different fact. Line 3 +// of a two-line file was reachable only because the count included a phantom +// trailing line. +func TestReadFileRejectsAnOffsetPastTheLastLine(t *testing.T) { + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "two.txt"), []byte("alpha\nbeta\n"), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"path": "two.txt", "offset": 3}) + res := (readFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !res.IsError { + t.Fatalf("offset 3 on a two-line file returned %q, want an error", res.Content) + } + if !strings.Contains(res.Content, "(2 lines)") { + t.Errorf("error does not report the real line count: %s", res.Content) + } +} diff --git a/internal/tools/search.go b/internal/tools/search.go index 0660271..31a3520 100644 --- a/internal/tools/search.go +++ b/internal/tools/search.go @@ -1,9 +1,9 @@ package tools import ( - "bufio" "context" "fmt" + "io" "io/fs" "os" "path/filepath" @@ -157,9 +157,10 @@ func globToRegexp(pattern string) (*regexp.Regexp, error) { // ---- grep ------------------------------------------------------------------- -// maxGrepFileBytes caps the size of a file grep will open, so a single huge log -// cannot stall a search across a whole tree. A file above the cap is never -// read, which is why the count of them has to reach the caller. +// maxGrepFileBytes caps the size of a file grep will read, so a single huge log +// cannot stall a search across a whole tree or be held in memory whole. A file +// above the cap is never read, which is why the count of them has to reach the +// caller. const maxGrepFileBytes = 8 * 1024 * 1024 type grepTool struct{} @@ -240,17 +241,27 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { return nil } defer f.Close() + // Line boundaries come from the whole file, because telling a lone CR + // terminator from a CR byte inside a line is a property of the file + // rather than of any one line. That makes the size gate this function's + // business: it is what bounds the read. + if info, err := f.Stat(); err == nil && info.Size() > maxGrepFileBytes { + skipped++ + return nil + } + data, err := io.ReadAll(f) + if err != nil { + return nil + } + content := string(data) - sc := bufio.NewScanner(f) - sc.Buffer(make([]byte, 0, 64*1024), 1024*1024) var window []string - lineNo := 0 fileMatched := false pending := 0 - for sc.Scan() { - lineNo++ - line := sc.Text() + for n, sp := range lineSpans(content) { + lineNo := n + 1 + line := content[sp.start:sp.end] if lineNo == 1 && !utf8.ValidString(line) { return nil // binary } @@ -285,11 +296,6 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { } } } - // A line longer than the scanner buffer aborts the scan; say so instead - // of silently reporting the rest of the file as match-free. - if err := sc.Err(); err != nil && len(warnings) < 8 { - warnings = append(warnings, fmt.Sprintf("%s: search stopped at line %d: %v", display, lineNo+1, err)) - } return nil } @@ -316,10 +322,6 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { if includeRe != nil && !includeRe.MatchString(rel) && !includeRe.MatchString(filepath.Base(rel)) { return nil } - if info, err := d.Info(); err == nil && info.Size() > maxGrepFileBytes { - skipped++ - return nil - } return searchFile(p, rel) }) } else { diff --git a/internal/tools/search_test.go b/internal/tools/search_test.go index 119a423..f054171 100644 --- a/internal/tools/search_test.go +++ b/internal/tools/search_test.go @@ -43,10 +43,12 @@ func TestGrepAndGlobFollowProjectReadBoundary(t *testing.T) { } } -// A line longer than the scanner buffer used to stop the file scan silently: -// no matches after it, no report. The tool must surface that the file scan -// stopped early. -func TestGrepReportsOverlongLineInsteadOfSilentStop(t *testing.T) { +// A line longer than the scanner buffer used to abort the file scan: no matches +// after it, and no report that the search had given up. Reporting the early +// stop was the fix available while lines came through a fixed buffer; lines are +// now cut from the whole file, so there is no buffer to overrun and the rest of +// the file is searched and numbered normally. +func TestGrepSearchesPastAnOverlongLine(t *testing.T) { workspace := t.TempDir() content := strings.Repeat("x", 2*1024*1024) + "\nNEEDLE line\n" if err := os.WriteFile(filepath.Join(workspace, "big.txt"), []byte(content), 0o644); err != nil { @@ -57,7 +59,10 @@ func TestGrepReportsOverlongLineInsteadOfSilentStop(t *testing.T) { if result.IsError { t.Fatalf("grep errored: %s", result.Content) } - if !strings.Contains(result.Content, "big.txt") || !strings.Contains(result.Content, "stopped") { - t.Fatalf("overlong line not reported: %q", result.Content) + if !strings.Contains(result.Content, "big.txt") || !strings.Contains(result.Content, "2:\tNEEDLE line") { + t.Fatalf("the match after an overlong line was not reported at line 2: %q", result.Content) + } + if strings.Contains(result.Content, "warning") { + t.Fatalf("the whole file was searched, so nothing should be warned about: %q", result.Content) } } From e806c696cfd6625d251b3c94bf8ec112f0a6e10a Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 01:09:22 +0700 Subject: [PATCH 04/20] Bound the read the size gate promised to bound The gate stated a file's size and then read without a limit, so anything whose stat understates what it yields walked straight through: character devices, most of /proc and /sys, and a file being appended to during the read. /dev/zero reports zero bytes and never ends, and a symlink to it in the workspace is an ordinary entry to filepath.WalkDir, so a plain recursive grep never returned. The bufio.Scanner this replaced stopped after 1 MB, so it was a regression, and it was the exact hazard cited to justify gating a directly named file in the first place. The LimitReader is the gate now and applies whether or not the fstat succeeded. Stat stays as an early-out so a 200 MB file is not read 8 MB deep before being rejected, and taken on the open descriptor it follows a symlink to its target, which the walk's Lstat did not. Also pins two behaviours that were carried without cover: an empty file stays readable, which is the other half of the offset guard, and a last line ending in a lone CR keeps that CR, which bufio.ScanLines dropped. Co-authored-by: Cursor --- internal/tools/grep_skip_test.go | 39 +++++++++++++++++++++++++ internal/tools/line_numbering_test.go | 42 +++++++++++++++++++++++++++ internal/tools/search.go | 16 ++++++++-- 3 files changed, 94 insertions(+), 3 deletions(-) diff --git a/internal/tools/grep_skip_test.go b/internal/tools/grep_skip_test.go index 4e1802d..c3fa3a9 100644 --- a/internal/tools/grep_skip_test.go +++ b/internal/tools/grep_skip_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) // The size gate keeps grep from opening large files at all, so a run that hit @@ -46,6 +47,44 @@ func TestGrepReportsSkippedFiles(t *testing.T) { } }) + // The gate has to bound the read, not just consult the size the file claims. + // A character device, most of /proc and /sys, and a file being appended to + // during the read all yield more than stat promised; /dev/zero reports zero + // bytes and never ends. A symlink to it is an ordinary entry to + // filepath.WalkDir, so a plain recursive grep reaches it without a project + // session, and an unbounded read there takes the process's memory with it. + t.Run("a file that understates its size is still bounded", func(t *testing.T) { + if _, err := os.Stat("/dev/zero"); err != nil { + t.Skip("no /dev/zero to read from on this platform") + } + workspace := t.TempDir() + if err := os.Symlink("/dev/zero", filepath.Join(workspace, "zero.log")); err != nil { + t.Skipf("cannot create a symlink in the workspace: %v", err) + } + args, err := json.Marshal(map[string]any{"pattern": "NEEDLE_TOKEN", "path": "."}) + if err != nil { + t.Fatal(err) + } + + // Run it off the test goroutine so an unbounded read fails the test + // instead of hanging the package until the go test deadline. + done := make(chan Result, 1) + go func() { + done <- (grepTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + }() + select { + case result := <-done: + if result.IsError { + t.Fatalf("grep errored: %s", result.Content) + } + if !strings.Contains(result.Content, "not searched") { + t.Errorf("an endless file was not reported as skipped: %q", result.Content) + } + case <-time.After(10 * time.Second): + t.Fatal("grep did not return: the read is not bounded by the size gate") + } + }) + t.Run("a run that skipped nothing says nothing", func(t *testing.T) { workspace := t.TempDir() if err := os.WriteFile(filepath.Join(workspace, "small.log"), []byte("NEEDLE_TOKEN here\n"), 0o644); err != nil { diff --git a/internal/tools/line_numbering_test.go b/internal/tools/line_numbering_test.go index 93f6ee4..faf2732 100644 --- a/internal/tools/line_numbering_test.go +++ b/internal/tools/line_numbering_test.go @@ -189,6 +189,48 @@ func TestGrepNumbersContextLinesFromTheSameSplitter(t *testing.T) { } } +// bufio.ScanLines dropped a trailing CR at end of file as well as before an LF, +// so the last line of "a\nb\r" reached the pattern as "b" — text the file does +// not contain. In an LF file that CR is data, and read_file shows it, so grep +// has to match what is really there. +func TestGrepKeepsATrailingCROnAnUnterminatedLastLine(t *testing.T) { + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "sample.txt"), []byte("a\nb\r"), 0o644); err != nil { + t.Fatal(err) + } + + args, _ := json.Marshal(map[string]any{"pattern": "b$", "path": "."}) + res := (grepTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if res.IsError { + t.Fatalf("grep failed: %s", res.Content) + } + if !strings.Contains(res.Content, "No matches") { + t.Errorf("b$ matched a line the file does not contain: %q", res.Content) + } + // The line is still found, and still numbered 2; only its text changed. + if got := grepMatchLines(t, workspace, "b"); len(got) != 1 || got[0] != 2 { + t.Errorf("grep reports the last line at %v, want [2]", got) + } +} + +// An empty file has no line 1 to be past, so offset 1 on it is not the mistake +// that an offset past the last line is. Without that half of the guard, the +// count of 0 lines would make every read of an empty file an error. +func TestReadFileStillReadsAnEmptyFile(t *testing.T) { + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "empty.txt"), nil, 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"path": "empty.txt"}) + res := (readFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if res.IsError { + t.Fatalf("reading an empty file was refused: %s", res.Content) + } + if got := readFileTotalLines(t, workspace, "empty.txt"); got != 0 { + t.Errorf("an empty file reports %d lines, want 0", got) + } +} + // An offset past the last line is a mistake worth naming. Returning nothing at // all reads as "the file is empty from here", which is a different fact. Line 3 // of a two-line file was reachable only because the count included a phantom diff --git a/internal/tools/search.go b/internal/tools/search.go index 31a3520..0c66110 100644 --- a/internal/tools/search.go +++ b/internal/tools/search.go @@ -243,16 +243,26 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { defer f.Close() // Line boundaries come from the whole file, because telling a lone CR // terminator from a CR byte inside a line is a property of the file - // rather than of any one line. That makes the size gate this function's - // business: it is what bounds the read. + // rather than of any one line. The read is therefore what the size gate + // has to bound, and the LimitReader is what bounds it. A stated size + // cannot: a character device, most of /proc, and a file being appended + // to during the read all yield more than stat promised, and /dev/zero + // reports zero bytes and never ends. Stat is only an early-out, so a + // 200 MB file is not read 8 MB deep before being rejected — and taken + // on the open descriptor it follows a symlink to its target, which the + // directory walk's Lstat did not. if info, err := f.Stat(); err == nil && info.Size() > maxGrepFileBytes { skipped++ return nil } - data, err := io.ReadAll(f) + data, err := io.ReadAll(io.LimitReader(f, maxGrepFileBytes+1)) if err != nil { return nil } + if len(data) > maxGrepFileBytes { + skipped++ + return nil + } content := string(data) var window []string From f94a6c02f4f6b26fec7cb9e148b9035d1fa8e81e Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 01:19:09 +0700 Subject: [PATCH 05/20] Show the file, not a rendering of it Co-authored-by: Cursor --- internal/tools/file.go | 34 ++++- internal/tools/file_edit_regression_test.go | 42 +++--- internal/tools/read_verbatim_test.go | 140 ++++++++++++++++++++ 3 files changed, 191 insertions(+), 25 deletions(-) create mode 100644 internal/tools/read_verbatim_test.go diff --git a/internal/tools/file.go b/internal/tools/file.go index 5f327ae..27f60ac 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -141,7 +141,7 @@ type readFileTool struct{} func (readFileTool) Name() string { return "read_file" } func (readFileTool) Description() string { - return "Read a text file from the workspace. Returns NUMBER|CONTENT lines; only text after | belongs in edit_file.old_string. Use offset/limit for large files." + return "Read a text file from the workspace. Returns a header line naming the path and line range, a blank line, then the file's exact bytes — copy any region of it straight into edit_file.old_string. Use offset/limit for large files." } func (readFileTool) Schema() map[string]any { return schema(map[string]any{ @@ -211,17 +211,41 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { end = len(lines) } - var b strings.Builder - for i := start; i < end; i++ { - fmt.Fprintf(&b, "%d|%s\n", i+1, content[lines[i].start:lines[i].end]) + // The selected lines' own bytes, terminators included, so what the model is + // shown is what edit_file can find. Slicing to the start of the line after + // the range keeps the last terminator; nothing here rewrites tabs, CRLF or + // a lone CR. + body := "" + if start < end { + to := len(content) + if end < len(lines) { + to = lines[end].start + } + body = content[lines[start].start:to] + } + // An empty file has no line 1, so the header names no line rather than + // inventing one. + first := start + 1 + if start == end { + first = 0 } + + rel := relTo(in.Workspace, path) + var b strings.Builder + fmt.Fprintf(&b, "%s — lines %d-%d of %d\n\n", rel, first, end, len(lines)) + b.WriteString(body) if end < len(lines) { fmt.Fprintf(&b, "\n… %d more lines (use offset=%d to continue)\n", len(lines)-end, end+1) } if truncatedBytes { b.WriteString("\n… file truncated at 400 KB\n") } - return Result{Content: b.String(), Meta: map[string]any{"path": relTo(in.Workspace, path), "lines": len(lines)}} + return Result{Content: b.String(), Meta: map[string]any{ + "path": rel, + "first_line": first, + "last_line": end, + "total_lines": len(lines), + }} } // ---- write_file ------------------------------------------------------------- diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index b902e07..ca65c40 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -21,8 +21,8 @@ func TestReadAndEditPreserveTabbedCRLFContent(t *testing.T) { if read.IsError { t.Fatalf("read_file: %s", read.Content) } - if !strings.Contains(read.Content, "2|\treturn !failed;") { - t.Fatalf("read output does not preserve indentation unambiguously: %q", read.Content) + if !strings.Contains(read.Content, "\treturn !failed;\r\n") { + t.Fatalf("read output does not preserve the line's tab and CRLF: %q", read.Content) } edit := (editFileTool{}).Execute(context.Background(), Input{ @@ -42,9 +42,9 @@ func TestReadAndEditPreserveTabbedCRLFContent(t *testing.T) { } } -// Model copies old_string from read_file output, which always uses LF, even when -// the on-disk file is CRLF. edit_file must still match and preserve the file's -// original line endings on write. +// A model writes \n for a line break whatever the file it read uses, so an +// old_string copied out of a CRLF file commonly comes back with LF. edit_file +// must still match it and preserve the file's original line endings on write. func TestEditFileMatchesCRLFWhenCopiedFromRead(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "win.go") @@ -59,15 +59,12 @@ func TestEditFileMatchesCRLFWhenCopiedFromRead(t *testing.T) { t.Fatalf("read: %s", read.Content) } - var copied []string - for _, line := range strings.Split(strings.TrimSuffix(read.Content, "\n"), "\n") { - _, content, ok := strings.Cut(line, "|") - if !ok { - t.Fatalf("read line missing NUMBER| separator: %q", line) - } - copied = append(copied, content) - } - // Function body as the model would reassemble it from the LF display. + body := readFileBody(t, read.Content) + if body != original { + t.Fatalf("read_file returned something other than the file's bytes: %q", body) + } + copied := strings.Split(strings.TrimSuffix(body, "\r\n"), "\r\n") + // Function body as the model reassembles it, with LF for every break. oldString := strings.Join(copied[2:5], "\n") newString := strings.Replace(oldString, "hi", "bye", 1) @@ -337,12 +334,14 @@ func TestReadFileTruncationDoesNotSplitRune(t *testing.T) { } } -// Classic-Mac style lone CR line endings must display as separate lines, not -// one giant line with embedded CR bytes. -func TestReadFileDisplaysLoneCRLines(t *testing.T) { +// Classic-Mac style lone CR line endings terminate lines, so such a file is +// three lines rather than one — but counting them is no licence to rewrite +// them, and edit_file matches the CR bytes that are really there. +func TestReadFileCountsLoneCRLinesWithoutRewritingThem(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "old.txt") - if err := os.WriteFile(path, []byte("a\rb\rc"), 0o644); err != nil { + original := "a\rb\rc" + if err := os.WriteFile(path, []byte(original), 0o644); err != nil { t.Fatal(err) } in := Input{Workspace: workspace, Args: []byte(`{"path":"old.txt"}`)} @@ -350,8 +349,11 @@ func TestReadFileDisplaysLoneCRLines(t *testing.T) { if result.IsError { t.Fatalf("read failed: %s", result.Content) } - if !strings.Contains(result.Content, "1|a\n2|b\n3|c") { - t.Fatalf("lone-CR file not split into lines: %q", result.Content) + if !strings.HasPrefix(result.Content, "old.txt — lines 1-3 of 3\n\n") { + t.Fatalf("lone-CR file not counted as three lines: %q", result.Content) + } + if body := readFileBody(t, result.Content); body != original { + t.Fatalf("lone-CR bytes rewritten for display: %q, want %q", body, original) } } diff --git a/internal/tools/read_verbatim_test.go b/internal/tools/read_verbatim_test.go new file mode 100644 index 0000000..2fb4935 --- /dev/null +++ b/internal/tools/read_verbatim_test.go @@ -0,0 +1,140 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// readFileResult drives the real tool and fails the test if the read was +// refused, so each case below asserts about content rather than about plumbing. +func readFileResult(t *testing.T, workspace string, args map[string]any) Result { + t.Helper() + raw, err := json.Marshal(args) + if err != nil { + t.Fatal(err) + } + res := (readFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: raw}) + if res.IsError { + t.Fatalf("read_file failed: %s", res.Content) + } + return res +} + +// readFileBody returns what read_file put below its header. The header is one +// line, so the first blank line in the output is the one that ends it. +func readFileBody(t *testing.T, out string) string { + t.Helper() + _, body, ok := strings.Cut(out, "\n\n") + if !ok { + t.Fatalf("read_file output has no header line followed by a blank line: %q", out) + } + return body +} + +// What read_file returns is what the model must hand back as an edit_file +// anchor, so anything the tool adds to a line is something the model has to +// remove again — and a separator stamped in front of the content collides with +// content that legitimately starts with it, most visibly a markdown table row. +// Returning the file's own bytes is what makes a copied region an exact anchor. +func TestReadFileReturnsVerbatim(t *testing.T) { + workspace := t.TempDir() + // Tab indentation, one CRLF line among LF lines, a lone CR that is data + // rather than a terminator, and rows beginning with a pipe. + original := "def main():\n" + + "\tif enabled:\r\n" + + "\t\trun() # ends here\rand continues\n" + + "\n" + + "| Date | Event |\n" + + "|------|-------|\n" + + "| 2026-01-01 | ship |\n" + if err := os.WriteFile(filepath.Join(workspace, "sample.py"), []byte(original), 0o644); err != nil { + t.Fatal(err) + } + + res := readFileResult(t, workspace, map[string]any{"path": "sample.py"}) + header := "sample.py — lines 1-7 of 7" + if !strings.HasPrefix(res.Content, header+"\n\n") { + t.Fatalf("read_file does not open with %q and a blank line:\n%q", header, res.Content) + } + body := strings.TrimPrefix(res.Content, header+"\n\n") + if body != original { + t.Fatalf("read_file returned a rendering of the file, not the file:\ngot %q\nwant %q", body, original) + } + + // The workflow the format exists for: copy a region out of what read_file + // returned and hand it straight back as an anchor. Cut it out of the body + // rather than writing it out again, so this still means something if the + // body ever stops being the file. + lines := strings.SplitAfter(body, "\n") + if len(lines) < 4 { + t.Fatalf("body does not hold the file's lines: %q", body) + } + copied := strings.Join(lines[1:3], "") + if !strings.Contains(original, copied) { + t.Errorf("a region copied out of read_file output is not in the file: %q", copied) + } +} + +// Rendering each line as "%s\n" gave an unterminated last line a newline the +// file does not have, so an anchor copied from the end of a file matched +// nothing. +func TestReadFileDoesNotTerminateAnUnterminatedLastLine(t *testing.T) { + workspace := t.TempDir() + original := "alpha\nbeta" + if err := os.WriteFile(filepath.Join(workspace, "tail.txt"), []byte(original), 0o644); err != nil { + t.Fatal(err) + } + res := readFileResult(t, workspace, map[string]any{"path": "tail.txt"}) + if body := readFileBody(t, res.Content); body != original { + t.Fatalf("body = %q, want the file's bytes %q", body, original) + } +} + +// The line range moves from the content into the header and Meta, so a clipped +// read still says which lines it covers and how many there are — the numbers +// grep and edit_file report against. +func TestReadFileHeaderAndMetaDescribeTheRange(t *testing.T) { + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "notes.txt"), []byte("one\ntwo\nthree\nfour\nfive\n"), 0o644); err != nil { + t.Fatal(err) + } + res := readFileResult(t, workspace, map[string]any{"path": "notes.txt", "offset": 2, "limit": 2}) + + want := "notes.txt — lines 2-3 of 5\n\ntwo\nthree\n" + if !strings.HasPrefix(res.Content, want) { + t.Fatalf("read_file output = %q, want it to begin %q", res.Content, want) + } + if !strings.Contains(res.Content, "… 2 more lines (use offset=4 to continue)") { + t.Errorf("clipped read does not say how to continue: %q", res.Content) + } + for key, want := range map[string]any{ + "path": "notes.txt", "first_line": 2, "last_line": 3, "total_lines": 5, + } { + if got := res.Meta[key]; got != want { + t.Errorf("Meta[%q] = %v, want %v", key, got, want) + } + } + if _, ok := res.Meta["lines"]; ok { + t.Errorf("Meta still carries the ambiguous \"lines\" key: %v", res.Meta) + } +} + +// A file with nothing in it has no line 1, so the header names no line rather +// than inventing one, and still reports the total the rest of the tools use. +func TestReadFileHeaderOnAnEmptyFile(t *testing.T) { + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "empty.txt"), nil, 0o644); err != nil { + t.Fatal(err) + } + res := readFileResult(t, workspace, map[string]any{"path": "empty.txt"}) + if want := "empty.txt — lines 0-0 of 0\n\n"; res.Content != want { + t.Fatalf("read_file on an empty file = %q, want %q", res.Content, want) + } + if got := res.Meta["total_lines"]; got != 0 { + t.Errorf("Meta[\"total_lines\"] = %v, want 0", got) + } +} From f00c0560f70c06dad46c0ee403e8942c4dde204a Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 01:27:30 +0700 Subject: [PATCH 06/20] Pin the clipped-read terminator and the binary rejection Co-authored-by: Cursor --- internal/tools/read_verbatim_test.go | 44 ++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/internal/tools/read_verbatim_test.go b/internal/tools/read_verbatim_test.go index 2fb4935..59eca52 100644 --- a/internal/tools/read_verbatim_test.go +++ b/internal/tools/read_verbatim_test.go @@ -104,12 +104,12 @@ func TestReadFileHeaderAndMetaDescribeTheRange(t *testing.T) { } res := readFileResult(t, workspace, map[string]any{"path": "notes.txt", "offset": 2, "limit": 2}) - want := "notes.txt — lines 2-3 of 5\n\ntwo\nthree\n" - if !strings.HasPrefix(res.Content, want) { - t.Fatalf("read_file output = %q, want it to begin %q", res.Content, want) - } - if !strings.Contains(res.Content, "… 2 more lines (use offset=4 to continue)") { - t.Errorf("clipped read does not say how to continue: %q", res.Content) + // Compared whole rather than by prefix. The continuation note opens with a + // newline, so a prefix check accepts a body that lost its last line's + // terminator — a byte per seam once the model pages through a file. + want := "notes.txt — lines 2-3 of 5\n\ntwo\nthree\n\n… 2 more lines (use offset=4 to continue)\n" + if res.Content != want { + t.Fatalf("read_file output = %q,\nwant %q", res.Content, want) } for key, want := range map[string]any{ "path": "notes.txt", "first_line": 2, "last_line": 3, "total_lines": 5, @@ -123,6 +123,38 @@ func TestReadFileHeaderAndMetaDescribeTheRange(t *testing.T) { } } +// Returning bytes that are not text spends the context on mojibake and hands +// the model an anchor it cannot reproduce, so a binary file is refused. The +// guard tests validity, not byte range: text that merely happens to be +// multibyte still reads, and reads verbatim. +func TestReadFileRefusesInvalidUTF8ButNotMultibyteText(t *testing.T) { + workspace := t.TempDir() + elf := []byte{0x7f, 'E', 'L', 'F', 0x02, 0x01, 0x01, 0x00, 0xff, 0xfe, 0x80, 0x00} + if err := os.WriteFile(filepath.Join(workspace, "app.bin"), elf, 0o644); err != nil { + t.Fatal(err) + } + raw, err := json.Marshal(map[string]any{"path": "app.bin"}) + if err != nil { + t.Fatal(err) + } + res := (readFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: raw}) + if !res.IsError { + t.Fatalf("bytes that are not valid UTF-8 were returned as text: %q", res.Content) + } + if !strings.Contains(res.Content, "binary file") { + t.Errorf("refusal does not say what was wrong with the file: %s", res.Content) + } + + original := "héllo — 日本語 ✓\nmultibyte, and perfectly readable\n" + if err := os.WriteFile(filepath.Join(workspace, "utf8.txt"), []byte(original), 0o644); err != nil { + t.Fatal(err) + } + got := readFileResult(t, workspace, map[string]any{"path": "utf8.txt"}) + if body := readFileBody(t, got.Content); body != original { + t.Fatalf("multibyte text = %q, want %q", body, original) + } +} + // A file with nothing in it has no line 1, so the header names no line rather // than inventing one, and still reports the total the rest of the tools use. func TestReadFileHeaderOnAnEmptyFile(t *testing.T) { From ef948cc315a8c64af6f2046d1c435ef663765ff7 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 01:37:22 +0700 Subject: [PATCH 07/20] Write only what was asked, only where it matches Co-authored-by: Cursor --- internal/tools/edit_exact_test.go | 97 +++++ internal/tools/file.go | 391 ++------------------ internal/tools/file_edit_recovery_test.go | 132 ------- internal/tools/file_edit_regression_test.go | 137 ++----- internal/tools/line_numbering_test.go | 1 + 5 files changed, 161 insertions(+), 597 deletions(-) create mode 100644 internal/tools/edit_exact_test.go delete mode 100644 internal/tools/file_edit_recovery_test.go diff --git a/internal/tools/edit_exact_test.go b/internal/tools/edit_exact_test.go new file mode 100644 index 0000000..47ca699 --- /dev/null +++ b/internal/tools/edit_exact_test.go @@ -0,0 +1,97 @@ +package tools + +import ( + "strings" + "testing" +) + +// The acceptance tests next door pin that edit_file refuses an anchor the file +// does not contain. These pin the other half of the contract: what reaches disk +// when it does write, and what the tool says it did. Both drive the real tool +// against a real file, through editOnDisk. + +// On the stage where old_string matched the file's own bytes there is nothing +// to reconcile, so anything done to new_string on the way to disk is damage. +// A CR inside a value is data, not a line break; rewriting it hands the file a +// line the caller never wrote. +func TestEditWritesNewStringByteForByteOnTheExactStage(t *testing.T) { + original := "id\tvalue\nMARKER\ntail\n" + said, isError, after := editOnDisk(t, "rows.tsv", original, map[string]any{ + "path": "rows.tsv", + "old_string": "MARKER", + "new_string": "note ends\rand continues", + }) + if isError { + t.Fatalf("an anchor present in the file was refused: %s", said) + } + if want := "id\tvalue\nnote ends\rand continues\ntail\n"; after != want { + t.Errorf("new_string was rewritten on the way to disk\nwant %q\ngot %q", want, after) + } +} + +// A model writes \n for a line break whatever the file it read used, so an +// all-LF anchor against a CRLF file is a well-defined ambiguity rather than a +// guess — the one translation the tool is allowed to make. What it must not do +// is make it silently: the caller asked for particular bytes and is entitled to +// know that different ones were matched and written. +func TestEditTranslatesAnLFAnchorForACRLFFileAndSaysSo(t *testing.T) { + original := "package main\r\n\r\nfunc main() {\r\n\tfmt.Println(\"hi\")\r\n}\r\n" + said, isError, after := editOnDisk(t, "win.go", original, map[string]any{ + "path": "win.go", + "old_string": "func main() {\n\tfmt.Println(\"hi\")\n}", + "new_string": "func main() {\n\tfmt.Println(\"bye\")\n}", + }) + if isError { + t.Fatalf("an LF anchor was refused against a CRLF file: %s", said) + } + want := "package main\r\n\r\nfunc main() {\r\n\tfmt.Println(\"bye\")\r\n}\r\n" + if after != want { + t.Errorf("new_string did not land in the file's own line endings\nwant %q\ngot %q", want, after) + } + if !strings.Contains(said, "LF to CRLF") { + t.Errorf("success message does not disclose the translation it performed: %s", said) + } +} + +// "Matched exactly" and "matched after a translation" are different facts, and +// a caller deciding whether to re-read acts on them differently. The exact path +// has nothing to disclose, so it must not decorate its result. +func TestEditReportsNoRecoveryWhenTheAnchorMatchedExactly(t *testing.T) { + original := "alpha\r\nbeta\r\ngamma\r\n" + said, isError, after := editOnDisk(t, "crlf.txt", original, map[string]any{ + "path": "crlf.txt", + "old_string": "beta\r\ngamma", + "new_string": "beta\r\nGAMMA", + }) + if isError { + t.Fatalf("a byte-exact anchor was refused: %s", said) + } + if want := "alpha\r\nbeta\r\nGAMMA\r\n"; after != want { + t.Errorf("edited bytes = %q, want %q", after, want) + } + if strings.Contains(said, "[") { + t.Errorf("an exact match reported a recovery: %s", said) + } +} + +// A space-indented anchor against a tab-indented file is the commonest way an +// edit misses, and the tool already knows how to say so. The message was +// unreachable: the adjacent-insertion splice claimed the edit first and wrote +// the spaces into the file. +func TestEditTabVersusSpaceAnchorGetsTheTabDiagnostic(t *testing.T) { + original := "def main():\n\tif enabled:\n\t\trun()\n" + said, isError, after := editOnDisk(t, "app.py", original, map[string]any{ + "path": "app.py", + "old_string": " if enabled:", + "new_string": " if enabled:\n setup()", + }) + if !isError { + t.Fatalf("an anchor that is not in the file was accepted: %s", said) + } + if after != original { + t.Errorf("file changed although the edit failed: %q", after) + } + if !strings.Contains(said, "indents with TAB characters") { + t.Errorf("error does not name the tab-versus-space mismatch: %s", said) + } +} diff --git a/internal/tools/file.go b/internal/tools/file.go index 27f60ac..ebdc5c8 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -9,7 +9,6 @@ import ( "os" "path/filepath" "sort" - "strconv" "strings" "unicode/utf8" ) @@ -332,13 +331,13 @@ type editFileTool struct{} func (editFileTool) Name() string { return "edit_file" } func (editFileTool) Description() string { - return "Replace an exact string in a file. The old_string must appear exactly once unless replace_all is set. Copy old_string from read_file output using only the content after the NUMBER| separator (never the line number). Preserve tabs/spaces exactly; line endings are matched automatically." + return "Replace an exact string in a file. The old_string must appear in the file exactly, and exactly once unless replace_all is set. Copy it straight out of read_file output, preserving tabs and spaces; only LF-for-CRLF line endings are reconciled for you." } func (editFileTool) RequiresApproval() bool { return true } func (editFileTool) Schema() map[string]any { return schema(map[string]any{ "path": prop("string", "File to edit."), - "old_string": prop("string", "Exact text to find, including indentation (tabs/spaces). Do not include read_file line numbers."), + "old_string": prop("string", "Exact text to find, including indentation (tabs/spaces)."), "new_string": prop("string", "Replacement text."), "replace_all": propDefault("boolean", "Replace every occurrence.", false), }, "path", "old_string", "new_string") @@ -369,25 +368,15 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { oldString, newString, count, how := resolveEditMatch(content, args.OldString, args.NewString) switch { case count == 0: - // Last-resort recovery for one narrow shape: a stale single-line anchor - // whose new_string only inserts adjacent text. Spliced by line index so - // it can never touch any other occurrence; never combined with - // replace_all, whose contract is "every exact occurrence". - if !args.ReplaceAll { - if updated, ok := spliceAdjacentInsertion(content, args.OldString, args.NewString); ok { - if err := writeWithCheckpoint(in, path, []byte(updated), "edit_file"); err != nil { - return Errorf("cannot write %s: %v", args.Path, err) - } - rel := relTo(in.Workspace, path) - return Result{ - Content: fmt.Sprintf("Edited %s (1 replacement(s)) [matched unique near line for adjacent insertion]", rel), - Meta: map[string]any{"path": rel, "replacements": 1}, - } - } - } return Errorf("%s", editNotFoundMessage(args.Path, content, args.OldString)) case count > 1 && !args.ReplaceAll: - return Errorf("%s", editAmbiguousMessage(args.Path, content, oldString, count)) + msg := editAmbiguousMessage(args.Path, content, oldString, count) + if how != "" { + // The count and the line numbers describe the translated anchor, so + // the caller has to be told which string they belong to. + msg += " [" + how + "]" + } + return Errorf("%s", msg) } var updated string @@ -458,117 +447,27 @@ func toEOL(s, eol string) string { return strings.ReplaceAll(s, "\n", eol) } -// stripReadFileLinePrefixes removes a NUMBER| prefix from every line when the -// whole block looks like a paste of read_file output. Returns ok=false when the -// string should be left alone (mixed or missing prefixes). -func stripReadFileLinePrefixes(s string) (string, bool) { - if s == "" { - return s, false - } - // Work on LF so CR in a pasted block does not hide the prefix. - normalized := strings.ReplaceAll(s, "\r\n", "\n") - normalized = strings.ReplaceAll(normalized, "\r", "\n") - // Preserve whether the input ended with a newline so join stays faithful. - trimTrailing := strings.HasSuffix(normalized, "\n") - body := normalized - if trimTrailing { - body = strings.TrimSuffix(body, "\n") - } - if body == "" { - return s, false - } - lines := strings.Split(body, "\n") - out := make([]string, 0, len(lines)) - nums := make([]int, 0, len(lines)) - for _, line := range lines { - i := strings.IndexByte(line, '|') - if i <= 0 { - return s, false - } - for _, c := range line[:i] { - if c < '0' || c > '9' { - return s, false - } - } - n, err := strconv.Atoi(line[:i]) - if err != nil { - return s, false - } - nums = append(nums, n) - out = append(out, line[i+1:]) - } - // read_file prefixes are always consecutive. A multi-line block whose - // numbers are not is real pipe-delimited data — stripping it could make a - // stale old_string match somewhere else entirely. - for k := 1; k < len(nums); k++ { - if nums[k] != nums[k-1]+1 { - return s, false - } - } - joined := strings.Join(out, "\n") - if trimTrailing { - joined += "\n" - } - return joined, true -} - -// resolveEditMatch finds old/new strings that match content, recovering from -// the two failure modes that read_file → edit_file commonly hits: -// 1. LF vs CRLF (read_file always displays LF) -// 2. pasted NUMBER| line prefixes from read_file output -// -// The verbatim input is always tried first: when old_string already matches -// the file bytes exactly, no newline heuristic may reject or rewrite it. -// how is a short note for the success message when recovery was used; empty on -// a plain exact match. +// resolveEditMatch locates the bytes old_string names and decides the bytes to +// write in their place. old_string is matched verbatim; the sole recovery is a +// line-ending translation. how names that translation for the result message +// and is empty on a verbatim match, so a caller is never told "exact" when it +// was not. func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string, count int, how string) { - // 0. Verbatim bytes. Mixed-EOL files and stray CR bytes made the old - // normalize-first order fail edits whose old_string was byte-perfect. if c := strings.Count(content, oldIn); c > 0 { - flav := eolOf(oldIn) - if flav == "" { - flav = fileEOL(content) - } - return oldIn, toEOL(newIn, flav), c, "" - } - - // Candidate flavors for normalized matching: the file's dominant flavor - // first, then the alternatives a mixed-EOL file may need. - flavors := []string{fileEOL(content), "\n", "\r\n"} - - try := func(oldCand, newCand, label string) bool { - tried := map[string]bool{oldIn: true} // verbatim already attempted - for _, flav := range flavors { - o := toEOL(oldCand, flav) - if o == "" || tried[o] { - continue - } - tried[o] = true - c := strings.Count(content, o) - if c == 0 { - continue - } - oldString, newString, count, how = o, toEOL(newCand, flav), c, label - return true - } - return false - } - - // 1. EOL-normalized (covers LF paste against a CRLF file and vice versa). - if try(oldIn, newIn, "normalized line endings to match file") { - return + return oldIn, newIn, c, "" } - // 2. Strip NUMBER| prefixes from a full paste of read_file output. - oldStripped, oldOK := stripReadFileLinePrefixes(oldIn) - newStripped, newOK := stripReadFileLinePrefixes(newIn) - if oldOK { - newCand := newIn - if newOK { - newCand = newStripped - } - if try(oldStripped, newCand, "stripped read_file NUMBER| prefixes") { - return + // A model emits \n for a line break whatever the file it read used, so an + // all-LF anchor against a CRLF file is a well-defined ambiguity rather than + // a guess, and translating it is lossless. It is reached only once the + // verbatim match has failed, and new_string is translated only because + // old_string had to be: a replacement is never rewritten on a path that + // matched exactly. + if fileEOL(content) == "\r\n" && eolOf(oldIn) == "\n" { + crlf := toEOL(oldIn, "\r\n") + if c := strings.Count(content, crlf); c > 0 { + return crlf, toEOL(newIn, "\r\n"), c, + "translated old_string line endings from LF to CRLF to match the file" } } @@ -623,263 +522,35 @@ func lineSpans(content string) []lineSpan { return spans } -// spliceAdjacentInsertion recovers one narrow failure shape: a common -// README/table operation copies a line from an earlier read, abbreviates one -// phrase, and adds a new row immediately before or after it. old_string is a -// single stale line, new_string only wraps it with inserted text, and exactly -// one file line is a clear similarity match. The inserted text is spliced at -// that line's byte range: the anchor line is kept byte-for-byte, and no other -// occurrence of similar text can be touched. Ordinary replacements remain -// exact-only. -func spliceAdjacentInsertion(content, oldIn, newIn string) (string, bool) { - oldNorm := toEOL(oldIn, "\n") - newNorm := toEOL(newIn, "\n") - if oldNorm == "" || strings.Contains(oldNorm, "\n") { - return "", false - } - // A NUMBER| line-prefix paste must never reach the fuzzy path. The anchor - // carries the prefix, so its token set still scores high against the real - // line, and the inserted text would be written to the file WITH its "13|" - // prefix while the tool reported success. Exact matching handles prefixed - // pastes properly via stripReadFileLinePrefixes; guessing must not. - if prefixed, total := readFileLinePrefixCounts(oldIn); prefixed > 0 && total > 0 { - return "", false - } - if prefixed, total := readFileLinePrefixCounts(newIn); prefixed > 0 && total > 0 { - return "", false - } - - insertAfter := false - insert := "" - switch { - case strings.HasPrefix(newNorm, oldNorm+"\n"): - insertAfter = true - insert = strings.TrimPrefix(newNorm, oldNorm) - case strings.HasSuffix(newNorm, "\n"+oldNorm): - insert = strings.TrimSuffix(newNorm, oldNorm) - default: - return "", false - } - - spans := lineSpans(content) - best, second := -1.0, -1.0 - bestIdx := -1 - for i, sp := range spans { - score := editLineSimilarity(oldNorm, content[sp.start:sp.end]) - if score > best { - second, best = best, score - bestIdx = i - } else if score > second { - second = score - } - } - if bestIdx < 0 || best < 0.78 || (second >= 0 && best-second < 0.12) { - return "", false - } - // Token overlap alone is not enough to claim "this is the same line, - // lightly reworded". Sibling rows of one table share almost every token by - // construction, so a deleted anchor row scores 0.8+ against a surviving row - // while the runner-up (a heading, a paragraph) sits far below and clears - // the margin gate too. - // - // Edit distance does not separate those cases either: "2026-02-01" vs - // "2026-03-01" is a one-character difference, exactly like a typo. What - // actually distinguishes them is WHICH characters differ. Digits are a - // line's identifying detail — dates, ids, versions, counts — and a - // rewording never changes them, while a different row almost always does. - // Requiring identical digits admits the abbreviations this recovery exists - // for and rejects the sibling-row confusion that silently corrupts files. - anchor := content[spans[bestIdx].start:spans[bestIdx].end] - if digitsOfLine(oldNorm) != digitsOfLine(anchor) { - return "", false - } - // Belt and braces: even with matching digits, the line must still be a - // light edit rather than a wholesale rewrite. - if !nearEditDistance(oldNorm, anchor, 0.34) { - return "", false - } - - insert = toEOL(insert, fileEOL(content)) - sp := spans[bestIdx] - if insertAfter { - return content[:sp.end] + insert + content[sp.end:], true - } - return content[:sp.start] + insert + content[sp.start:], true -} - -// digitsOfLine returns just the digits of s, in order. Two renderings of the -// same line keep the same digits; two different rows of one table almost never -// do. -func digitsOfLine(s string) string { - out := make([]byte, 0, 16) - for i := 0; i < len(s); i++ { - if s[i] >= '0' && s[i] <= '9' { - out = append(out, s[i]) - } - } - return string(out) -} - -// nearEditDistance reports whether b is within maxRatio of a's length in -// Levenshtein distance — i.e. b looks like a lightly edited a rather than a -// different line that merely reuses the same vocabulary. Long lines are capped -// so the O(n*m) table stays small on an error-recovery path. -func nearEditDistance(a, b string, maxRatio float64) bool { - const cap = 512 - if len(a) > cap { - a = a[:cap] - } - if len(b) > cap { - b = b[:cap] - } - if a == b { - return true - } - longest := len(a) - if len(b) > longest { - longest = len(b) - } - if longest == 0 { - return false - } - budget := int(float64(longest) * maxRatio) - // A length gap alone can already exceed the budget; skip the table then. - if diff := len(a) - len(b); diff > budget || -diff > budget { - return false - } - - prev := make([]int, len(b)+1) - cur := make([]int, len(b)+1) - for j := range prev { - prev[j] = j - } - for i := 1; i <= len(a); i++ { - cur[0] = i - for j := 1; j <= len(b); j++ { - cost := 1 - if a[i-1] == b[j-1] { - cost = 0 - } - del, ins, sub := prev[j]+1, cur[j-1]+1, prev[j-1]+cost - best := del - if ins < best { - best = ins - } - if sub < best { - best = sub - } - cur[j] = best - } - prev, cur = cur, prev - } - return prev[len(b)] <= budget -} - -func editLineSimilarity(a, b string) float64 { - aSet := editTokenSet(a) - bSet := editTokenSet(b) - if len(aSet) == 0 || len(bSet) == 0 { - return 0 - } - common := 0 - for token := range aSet { - if _, ok := bSet[token]; ok { - common++ - } - } - return float64(common) / float64(len(aSet)+len(bSet)-common) -} - -func editTokenSet(s string) map[string]struct{} { - set := make(map[string]struct{}) - start := -1 - flush := func(end int) { - if start >= 0 && end-start >= 2 { - set[strings.ToLower(s[start:end])] = struct{}{} - } - start = -1 - } - for i := 0; i < len(s); i++ { - c := s[i] - isToken := (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' - if isToken { - if start < 0 { - start = i - } - } else { - flush(i) - } - } - flush(len(s)) - return set -} - -// editNotFoundMessage explains why an edit missed, with actionable recovery -// hints for the model (line prefixes, tabs vs spaces, re-read). +// editNotFoundMessage explains why an edit missed, and names a next step that +// can actually succeed. Whitespace is the usual culprit and the hardest thing +// to see in a diff of two quoted strings, so it is diagnosed first. func editNotFoundMessage(path, content, oldString string) string { var b strings.Builder fmt.Fprintf(&b, "old_string not found in %s.", path) - if stripped, ok := stripReadFileLinePrefixes(oldString); ok { - if strings.Count(content, toEOL(stripped, fileEOL(content))) > 0 { - b.WriteString(" Your old_string still includes read_file line numbers (NUMBER|). Call edit_file again with only the content after each |.") - return b.String() - } - } - if strings.Contains(content, "\t") && strings.Contains(oldString, " ") && !strings.Contains(oldString, "\t") { // Spaces in old_string might still be inter-word; only flag when a // detabbed view of the file contains the old_string. for _, width := range []int{2, 4, 8} { detabbed := expandTabs(content, width) if strings.Contains(detabbed, toEOL(oldString, "\n")) || strings.Contains(detabbed, oldString) { - fmt.Fprintf(&b, " The file indents with TAB characters, but old_string uses spaces (tab width ~%d). Re-read the file and copy the content after NUMBER| without expanding tabs.", width) + fmt.Fprintf(&b, " The file indents with TAB characters, but old_string uses spaces (tab width ~%d). Re-read the file and copy the indentation exactly as it comes back, without expanding tabs.", width) return b.String() } } } - // A mixed paste usually means the model copied the display prefix from only - // one or two read_file lines. Do not silently strip it: the unprefixed lines - // may contain literal pipe characters. - if prefixed, total := readFileLinePrefixCounts(oldString); prefixed > 0 && prefixed < total { - b.WriteString(" Some old_string lines still include read_file line numbers (NUMBER|) while others do not. Remove every numeric prefix and keep only the text after each |, then retry from a fresh read.") - return b.String() - } if hint := nearMissHint(content, oldString); hint != "" { b.WriteByte(' ') b.WriteString(hint) return b.String() } - b.WriteString(" Read the file first and copy only the content after the NUMBER| separator; preserve tabs, spaces, and indentation exactly.") + b.WriteString(" Read the file and copy old_string straight out of what it returns; preserve tabs, spaces, and indentation exactly.") return b.String() } -func readFileLinePrefixCounts(s string) (prefixed, total int) { - lines := strings.Split(strings.ReplaceAll(s, "\r\n", "\n"), "\n") - if len(lines) > 1 && lines[len(lines)-1] == "" { - lines = lines[:len(lines)-1] - } - for _, line := range lines { - total++ - i := strings.IndexByte(line, '|') - if i > 0 { - allDigits := true - for _, c := range line[:i] { - if c < '0' || c > '9' { - allDigits = false - break - } - } - if allDigits { - prefixed++ - } - } - } - return prefixed, total -} - func editAmbiguousMessage(path, content, oldString string, count int) string { var b strings.Builder fmt.Fprintf(&b, "old_string appears %d times in %s; add unique surrounding context or set replace_all only if every occurrence should change.", count, path) diff --git a/internal/tools/file_edit_recovery_test.go b/internal/tools/file_edit_recovery_test.go deleted file mode 100644 index 7d35f48..0000000 --- a/internal/tools/file_edit_recovery_test.go +++ /dev/null @@ -1,132 +0,0 @@ -package tools - -import ( - "strings" - "testing" -) - -// The adjacent-insertion recovery reports success, so every case where it -// guesses wrong is a silent file corruption. These tests pin the guards that -// keep it from guessing. - -// A NUMBER| paste must never reach the fuzzy path. The prefix survives in the -// anchor's token set, so the stale line still scored high against the real -// one — and the inserted row was written to the file carrying its literal -// "13|" prefix while the tool reported "1 replacement(s)". -func TestSpliceRefusesReadFilePrefixedPaste(t *testing.T) { - content := "# Pools\n\n- **pool39v2** | 14 hand | active\n- **pool40** | 9 hand | active\n" - old := "12|- **pool39v2** | 14 hand | active" - nw := old + "\n13|- **pool41** | 3 hand | new" - - got, ok := spliceAdjacentInsertion(content, old, nw) - if ok { - t.Fatalf("spliced a NUMBER|-prefixed paste; file would gain a literal prefix:\n%q", got) - } -} - -// A deleted anchor whose sibling row survives must not be spliced onto that -// sibling. Rows of one table share almost every token by construction, so -// token overlap cleared both the 0.78 floor and the 0.12 margin. Edit distance -// does not separate them either — the dates differ by one character, exactly -// like a typo — which is why the guard keys on digits. -func TestSpliceRefusesDeletedAnchorWithSimilarSibling(t *testing.T) { - content := "# Release notes\n\nThis document tracks published build artifacts.\n\n| 2026-03-01 | nightly | artifacts uploaded to the mirror |\n" - old := "| 2026-02-01 | nightly | artifacts uploaded to the mirror |" - nw := old + "\n| 2026-04-01 | stable | artifacts signed and uploaded |" - - got, ok := spliceAdjacentInsertion(content, old, nw) - if ok { - t.Fatalf("spliced against an anchor that is not in the file:\n%q", got) - } -} - -// The digit guard must not reject a light rewording that keeps its numbers. -// (The similarity floor of 0.78 already bounds how far the wording may drift; -// this pins that digits do not add a second, stricter rejection on top.) -func TestSpliceAllowsRewordingThatKeepsDigits(t *testing.T) { - content := "# Status\n\n| v2 | 14 items | active on the primary host right now |\n| v3 | 2 items | idle |\n" - old := "| v2 | 14 items | active on the primary host |" - nw := old + "\n| v9 | 8 items | new |" - - got, ok := spliceAdjacentInsertion(content, old, nw) - if !ok { - t.Fatal("a lightly reworded anchor with identical digits should still recover") - } - if !strings.Contains(got, "| v9 | 8 items | new |") { - t.Fatalf("insertion missing:\n%q", got) - } - if !strings.Contains(got, "| v2 | 14 items | active on the primary host right now |") { - t.Fatalf("anchor was mutated instead of preserved byte-for-byte:\n%q", got) - } -} - -func TestDigitsOfLine(t *testing.T) { - if digitsOfLine("| 2026-02-01 | nightly |") == digitsOfLine("| 2026-03-01 | nightly |") { - t.Error("different dates must yield different digit strings") - } - if digitsOfLine("compiles the binry") != digitsOfLine("compiles the binary") { - t.Error("a typo in a digit-free line must not change the digits") - } - if got := digitsOfLine("a1b2c3"); got != "123" { - t.Errorf("digitsOfLine = %q, want %q", got, "123") - } -} - -// The guards must not kill the case the recovery exists for: the same line, -// lightly abbreviated, with a row added after it. -func TestSpliceStillRecoversLightlyRewordedAnchor(t *testing.T) { - content := "# Commands\n\n| make build | compiles the static binary for the host platform |\n| make lint | vet |\n" - old := "| make build | compiles the static binary for the host platfrm |" // one typo - nw := old + "\n| make test | runs the suite |" - - got, ok := spliceAdjacentInsertion(content, old, nw) - if !ok { - t.Fatal("recovery declined a genuine near-edit anchor; the guards are too tight") - } - if !strings.Contains(got, "| make build | compiles the static binary for the host platform |") { - t.Fatalf("anchor line was mutated instead of preserved byte-for-byte:\n%q", got) - } - if !strings.Contains(got, "| make test | runs the suite |") { - t.Fatalf("the insertion is missing:\n%q", got) - } - if strings.Count(got, "| make lint | vet |") != 1 { - t.Fatalf("an unrelated line was duplicated or lost:\n%q", got) - } -} - -func TestNearEditDistance(t *testing.T) { - if !nearEditDistance("hello world", "hello world", 0.25) { - t.Error("identical strings must be near") - } - if !nearEditDistance("compiles the binary", "compiles the binry", 0.25) { - t.Error("a one-character typo must stay near") - } - if nearEditDistance("| 2026-02-01 | nightly | uploaded |", "| 2026-03-01 | stable | signed |", 0.25) { - t.Error("two different table rows must not be near") - } - if nearEditDistance("short", "a much much longer line entirely", 0.25) { - t.Error("a large length gap must not be near") - } -} - -// read_file and edit_file must agree on what a lone CR means. When they -// disagree, read_file hands the model line numbers that do not exist in the -// file's real line structure — producing exactly the stale anchors this PR is -// trying to eliminate. -func TestLoneCRIsDataUnlessTheFileIsCRTerminated(t *testing.T) { - // LF file with a CR inside a value: one logical line per LF. - lfWithData := "note ends\rrest\nsecond line\n" - if got := len(lineSpans(lfWithData)); got != 2 { - t.Errorf("LF file with an embedded CR: got %d lines, want 2", got) - } - // Genuine classic-Mac file: CR terminates. - crFile := "alpha\rbeta\rgamma" - if got := len(lineSpans(crFile)); got != 3 { - t.Errorf("CR-terminated file: got %d lines, want 3", got) - } - // CRLF stays one line per pair. - crlf := "a\r\nb\r\nc\r\n" - if got := len(lineSpans(crlf)); got != 3 { - t.Errorf("CRLF file: got %d lines, want 3", got) - } -} diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index ca65c40..180d662 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -86,36 +86,6 @@ func TestEditFileMatchesCRLFWhenCopiedFromRead(t *testing.T) { } } -// Models sometimes paste the whole NUMBER| line from read_file into old_string. -// edit_file should strip a consistent line-number prefix block and still match. -func TestEditFileStripsReadFileLineNumberPrefixes(t *testing.T) { - workspace := t.TempDir() - path := filepath.Join(workspace, "a.go") - original := "package main\n\nfunc main() {\n\treturn\n}\n" - if err := os.WriteFile(path, []byte(original), 0o644); err != nil { - t.Fatal(err) - } - - // Accidental paste of the read_file display format. - oldWithPrefix := "3|func main() {\n4|\treturn\n5|}" - newWithPrefix := "3|func main() {\n4|\treturn nil\n5|}" - editArgs, _ := json.Marshal(map[string]any{ - "path": "a.go", "old_string": oldWithPrefix, "new_string": newWithPrefix, - }) - edited := (editFileTool{}).Execute(context.Background(), Input{Args: editArgs, Workspace: workspace}) - if edited.IsError { - t.Fatalf("edit_file should strip NUMBER| prefixes: %s", edited.Content) - } - got, err := os.ReadFile(path) - if err != nil { - t.Fatal(err) - } - want := "package main\n\nfunc main() {\n\treturn nil\n}\n" - if string(got) != want { - t.Fatalf("edited = %q, want %q", got, want) - } -} - // When the match still fails, the error must say what went wrong in a way the // model can act on (tabs vs spaces is the common indentation trap). func TestEditFileDiagnosesTabVsSpaceMismatch(t *testing.T) { @@ -166,7 +136,13 @@ func TestEditFileNotFoundShowsNearMiss(t *testing.T) { } } -func TestEditFileRecoversUniqueNearInsertionWithoutChangingExistingLine(t *testing.T) { +// The row in the file says "85 (early stop @55)" and the anchor abbreviates it +// to "85 (ES@55)", so the anchor is not in the file. This is the case the +// adjacent-insertion recovery was built for, and the case that shows why it +// cannot exist: the same shape is indistinguishable from an anchor whose row +// was deleted, where the insertion lands against a sibling row instead. A +// stale anchor is a stale read, and the answer is to read again. +func TestEditFileRefusesAnAbbreviatedTableRowAnchor(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "README.md") actual := "| **pool39v2** | 14 hand + 25 vision-audited | 33 train / 6 val | T4 | 85 (early stop @55) | **0.009** | `artifacts/pool39v2/` |\n" @@ -177,52 +153,48 @@ func TestEditFileRecoversUniqueNearInsertionWithoutChangingExistingLine(t *testi newString := old + "\n| **pool39v2_sc** | single-class icon | 33 train / 6 val | T4 | 70 | **0.519** | `artifacts/pool39v2_sc/` |" args, _ := json.Marshal(map[string]any{"path": "README.md", "old_string": old, "new_string": newString}) result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) - if result.IsError { - t.Fatalf("unique adjacent insertion should recover: %s", result.Content) + if !result.IsError || !strings.Contains(result.Content, "old_string not found") { + t.Fatalf("an anchor that is not in the file was accepted: %+v", result) } got, err := os.ReadFile(path) if err != nil { t.Fatal(err) } - want := actual + "| **pool39v2_sc** | single-class icon | 33 train / 6 val | T4 | 70 | **0.519** | `artifacts/pool39v2_sc/` |\n" - if string(got) != want { - t.Fatalf("recovery changed the existing line:\n%s\nwant:\n%s", got, want) + if string(got) != actual { + t.Fatalf("file changed although the anchor is absent:\n%s", got) } } -// The similarity search picks a unique best line, so the insertion must land -// at that line — not at an earlier occurrence of the same text inside a longer -// line, which strings.Replace-based recovery corrupted mid-line. -func TestEditFileAdjacentInsertionSplicesAtMatchedLine(t *testing.T) { +// One space apart from a line that is really there is still not that line. The +// margin between "close" and "correct" is where silent corruption lives, and +// nothing in the tool is allowed to cross it. +func TestEditFileRefusesAnAnchorOneSpaceOffARealLine(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "f.txt") content := "start\nreturn nil // TODO cleanup\nmiddle\nreturn nil\nend\n" if err := os.WriteFile(path, []byte(content), 0o644); err != nil { t.Fatal(err) } - // Stale old_string (double space) matches nothing exactly; similarity must - // pick line 4 ("return nil", score 1.0) over line 2 (score 0.5). - old := "return nil" + old := "return nil" // double space; the file has one args, _ := json.Marshal(map[string]any{ "path": "f.txt", "old_string": old, "new_string": old + "\nINSERTED", }) result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) - if result.IsError { - t.Fatalf("unique near-line insertion should recover: %s", result.Content) + if !result.IsError { + t.Fatalf("a near-miss anchor was accepted: %s", result.Content) } got, err := os.ReadFile(path) if err != nil { t.Fatal(err) } - want := "start\nreturn nil // TODO cleanup\nmiddle\nreturn nil\nINSERTED\nend\n" - if string(got) != want { - t.Fatalf("insertion landed at the wrong place:\n%s\nwant:\n%s", got, want) + if string(got) != content { + t.Fatalf("file changed although the anchor is absent:\n%s", got) } } -// replace_all promises "replace every exact occurrence"; a similarity-based -// recovery must never piggyback on it and multiply insertions. -func TestEditFileAdjacentInsertionIgnoredWithReplaceAll(t *testing.T) { +// replace_all promises "replace every exact occurrence", so an anchor that +// occurs zero times must change nothing at all. +func TestEditFileReplaceAllRefusesAnAbsentAnchor(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "ra.txt") content := "return nil // TODO cleanup\nmiddle\nreturn nil\nend\n" @@ -235,14 +207,14 @@ func TestEditFileAdjacentInsertionIgnoredWithReplaceAll(t *testing.T) { }) result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) if !result.IsError { - t.Fatalf("replace_all must not trigger similarity recovery: %s", result.Content) + t.Fatalf("replace_all accepted an anchor that occurs zero times: %s", result.Content) } got, err := os.ReadFile(path) if err != nil { t.Fatal(err) } if string(got) != content { - t.Fatalf("file modified by rejected recovery:\n%s", got) + t.Fatalf("file changed although the anchor is absent:\n%s", got) } } @@ -299,21 +271,6 @@ func TestEditFileExactMatchDespiteStrayCR(t *testing.T) { } } -// read_file line numbers are always consecutive, so a multi-line block whose -// numeric prefixes are not sequential is real pipe-delimited data, not a paste. -func TestStripReadFileLinePrefixesRequiresSequentialNumbers(t *testing.T) { - if _, ok := stripReadFileLinePrefixes("3|a\n7|b"); ok { - t.Fatal("non-sequential numeric prefixes must not strip") - } - if _, ok := stripReadFileLinePrefixes("5|x\n5|y"); ok { - t.Fatal("repeated numeric prefixes must not strip") - } - got, ok := stripReadFileLinePrefixes("9|a\n10|b\n11|c") - if !ok || got != "a\nb\nc" { - t.Fatalf("sequential prefixes should strip, got %q ok=%v", got, ok) - } -} - // Truncating at the byte cap must not cut a multi-byte rune in half and then // misreport the whole file as binary. func TestReadFileTruncationDoesNotSplitRune(t *testing.T) { @@ -357,7 +314,11 @@ func TestReadFileCountsLoneCRLinesWithoutRewritingThem(t *testing.T) { } } -func TestEditFileDoesNotRecoverAmbiguousNearInsertion(t *testing.T) { +// A markdown table row is the shape that made "close enough" look reasonable, +// and two sibling rows are the shape that made it dangerous: the anchor is +// equally near to both, and either choice writes into a row the caller did not +// name. +func TestEditFileRefusesAnAnchorNearTwoSiblingRows(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "README.md") content := "| **pool39v2** | 85 (early stop @55) | artifacts/a |\n| **pool39v2** | 85 (early stop @55) | artifacts/b |\n" @@ -370,47 +331,13 @@ func TestEditFileDoesNotRecoverAmbiguousNearInsertion(t *testing.T) { }) result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) if !result.IsError || !strings.Contains(result.Content, "old_string not found") { - t.Fatalf("ambiguous near insertion must remain exact-only: %+v", result) + t.Fatalf("an anchor that is not in the file was accepted: %+v", result) } got, err := os.ReadFile(path) if err != nil { t.Fatal(err) } if string(got) != content { - t.Fatalf("ambiguous recovery modified file: %q", got) - } -} - -func TestStripReadFileLinePrefixes(t *testing.T) { - in := "10|\tfoo()\n11|\tbar()\n12|}" - got, ok := stripReadFileLinePrefixes(in) - if !ok { - t.Fatal("expected strip success") - } - if got != "\tfoo()\n\tbar()\n}" { - t.Fatalf("got %q", got) - } - // Not every line prefixed → leave alone (could be real pipe content). - if _, ok := stripReadFileLinePrefixes("a|b\nc"); ok { - t.Fatal("partial prefix block must not strip") - } - // Single line of real data that happens to contain a pipe stays intact. - if s, ok := stripReadFileLinePrefixes("nope"); ok || s != "nope" { - t.Fatalf("non-prefixed = %q ok=%v", s, ok) - } -} - -func TestEditFileDiagnosesMixedReadPrefixes(t *testing.T) { - workspace := t.TempDir() - path := filepath.Join(workspace, "mixed.go") - if err := os.WriteFile(path, []byte("func run() {\n\treturn\n}\n"), 0o644); err != nil { - t.Fatal(err) - } - args, _ := json.Marshal(map[string]any{ - "path": "mixed.go", "old_string": "1|func run() {\n\treturn\n}", "new_string": "1|func run() {\n\treturn nil\n}", - }) - result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) - if !result.IsError || !strings.Contains(result.Content, "Some old_string lines still include") { - t.Fatalf("mixed-prefix diagnostic missing: %+v", result) + t.Fatalf("file changed although the anchor is absent: %q", got) } } diff --git a/internal/tools/line_numbering_test.go b/internal/tools/line_numbering_test.go index faf2732..f654a0c 100644 --- a/internal/tools/line_numbering_test.go +++ b/internal/tools/line_numbering_test.go @@ -157,6 +157,7 @@ func TestLineSpansCountsTerminatedAndUnterminatedFilesAlike(t *testing.T) { {"lf unterminated", "a\nb", 2}, {"crlf terminated", "a\r\nb\r\n", 2}, {"cr terminated", "a\rb\r", 2}, + {"cr unterminated", "alpha\rbeta\rgamma", 3}, {"blank line before the end", "a\n\n", 2}, {"lone cr is data in an lf file", "a\rb\nc\n", 2}, } { From 5b4ebf086b0a9df62b9f74e1000b1173b375f0b1 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 01:44:42 +0700 Subject: [PATCH 08/20] Translate line endings, not the bytes between them Co-authored-by: Cursor --- internal/tools/edit_exact_test.go | 37 +++++++++++++++++++++++++++++++ internal/tools/file.go | 17 +++++++++++--- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/internal/tools/edit_exact_test.go b/internal/tools/edit_exact_test.go index 47ca699..a9fcdfc 100644 --- a/internal/tools/edit_exact_test.go +++ b/internal/tools/edit_exact_test.go @@ -29,6 +29,43 @@ func TestEditWritesNewStringByteForByteOnTheExactStage(t *testing.T) { } } +// The twin of the test above, on the other stage. Translating line endings and +// reinterpreting a byte as a line ending are different acts, and the recovery +// stage is authorised only for the first. A CR the caller did not write as a +// line break is data, and folding it into one splits a value across two lines +// while the tool reports success — the same harm as stripping a prefix. +func TestEditWritesNewStringByteForByteOnTheRecoveryStage(t *testing.T) { + for _, tc := range []struct{ name, newString, want string }{ + { + "a lone CR stays data", + "beta\nnote ends\rand continues", + "alpha\r\nbeta\r\nnote ends\rand continues\r\n", + }, + { + // Expanding every LF without folding an existing CRLF first would + // write \r\r\n here. + "a break already written as CRLF is not doubled", + "beta\r\ndelta\nepsilon", + "alpha\r\nbeta\r\ndelta\r\nepsilon\r\n", + }, + } { + t.Run(tc.name, func(t *testing.T) { + said, isError, after := editOnDisk(t, "notes.txt", "alpha\r\nbeta\r\ngamma\r\n", map[string]any{ + "path": "notes.txt", + // LF, so only the recovery stage can match it. + "old_string": "beta\ngamma", + "new_string": tc.newString, + }) + if isError { + t.Fatalf("an LF anchor was refused against a CRLF file: %s", said) + } + if after != tc.want { + t.Errorf("new_string was altered beyond its line endings\nwant %q\ngot %q", tc.want, after) + } + }) + } +} + // A model writes \n for a line break whatever the file it read used, so an // all-LF anchor against a CRLF file is a well-defined ambiguity rather than a // guess — the one translation the tool is allowed to make. What it must not do diff --git a/internal/tools/file.go b/internal/tools/file.go index ebdc5c8..bdd516e 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -447,6 +447,16 @@ func toEOL(s, eol string) string { return strings.ReplaceAll(s, "\n", eol) } +// lfToCRLF expands the LF line breaks in s to CRLF and leaves every other byte +// as it was. toEOL cannot stand in for it on a string bound for the file: toEOL +// folds a lone CR into a line break before expanding, and a CR the caller did +// not write as a line break is data. Folding existing CRLF first is what keeps +// the expansion from writing \r\r\n. +func lfToCRLF(s string) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + return strings.ReplaceAll(s, "\n", "\r\n") +} + // resolveEditMatch locates the bytes old_string names and decides the bytes to // write in their place. old_string is matched verbatim; the sole recovery is a // line-ending translation. how names that translation for the result message @@ -462,11 +472,12 @@ func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string // a guess, and translating it is lossless. It is reached only once the // verbatim match has failed, and new_string is translated only because // old_string had to be: a replacement is never rewritten on a path that - // matched exactly. + // matched exactly. eolOf has already established that old_string holds no + // CR at all, so every \n in it is unambiguously a line break. if fileEOL(content) == "\r\n" && eolOf(oldIn) == "\n" { - crlf := toEOL(oldIn, "\r\n") + crlf := lfToCRLF(oldIn) if c := strings.Count(content, crlf); c > 0 { - return crlf, toEOL(newIn, "\r\n"), c, + return crlf, lfToCRLF(newIn), c, "translated old_string line endings from LF to CRLF to match the file" } } From a38e0d8ca816a30831ca8d4e3b8702f22901c86d Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 01:54:55 +0700 Subject: [PATCH 09/20] Describe the file tools as they now behave Co-authored-by: Cursor --- docs/tools.md | 27 ++++-- internal/agent/prompt.go | 12 ++- internal/agent/prompt_file_notes_test.go | 114 +++++++++++++++++++++++ internal/tools/edit_eol_advisory_test.go | 80 ++++++++++++++++ internal/tools/file.go | 10 ++ 5 files changed, 232 insertions(+), 11 deletions(-) create mode 100644 internal/agent/prompt_file_notes_test.go create mode 100644 internal/tools/edit_eol_advisory_test.go diff --git a/docs/tools.md b/docs/tools.md index 43dc04c..a7f558e 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -9,7 +9,7 @@ whatever MCP servers add. | Tool | What it does | |---|---| -| `read_file` | Read a text file as `NUMBER|CONTENT` lines, with offset and limit for large ones | +| `read_file` | Read a text file's bytes verbatim under a line-range header, with offset and limit for large ones | | `write_file` | Create or overwrite, making parent directories | | `edit_file` | Replace an exact string, which must appear exactly once unless told otherwise | | `list_files` | Directory entries, optionally recursive | @@ -21,11 +21,26 @@ Paths are relative to the workspace and cannot escape it. `edit_file` requiring a unique match is deliberate: an edit that silently hits the wrong occurrence is worse than one that fails. -`read_file` prints each line as `NUMBER|CONTENT`. The number and `|` are -metadata for the model — they are not part of the file. `edit_file` matches -line endings to the file automatically (so a paste from `read_file` works on -CRLF files) and will strip a whole-block paste of `NUMBER|` prefixes if the -model includes them by mistake. Tabs and spaces must still match exactly. +`read_file` returns one header line, ` — lines - of +`, a blank line, then the file's bytes unaltered — tabs, CRLF, and lone +CR included. Nothing is stamped onto a line, so there is nothing for the model +to strip before using a region as an `edit_file` anchor. The same numbers are +in the result's metadata (`path`, `first_line`, `last_line`, `total_lines`) for +callers that would otherwise parse the header. + +`edit_file` matches `old_string` byte for byte and writes `new_string` byte for +byte. One recovery exists: if the file uses CRLF and an anchor whose every break +is LF did not match, it is retried with those breaks expanded to CRLF, since a +model emits `\n` whatever it read. That runs only after the exact match has +failed, translates `new_string` only because `old_string` had to be, and says so +in its result. So when the anchor matched exactly, a `new_string` written with +LF lands in a CRLF file as LF; the result notes that rather than rewriting bytes +the caller asked for. + +An anchor that is not found is an anchor that is wrong — stale, misremembered, +or reformatted. The fix is another `read_file`, not more surrounding context. +Extra context is the answer to the other error, where the anchor matches more +than once. ### Terminal diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index d9bef39..b38a247 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -103,11 +103,13 @@ help them now — do not block them. b.WriteString("\n## Tool notes\n\n") b.WriteString("- Paths given to file tools are relative to the workspace; you cannot read outside it.\n") if hasTool(active, "read_file") || hasTool(active, "edit_file") { - // Harness guidance for the read → edit loop. Without this, models - // paste line numbers into old_string or expand tabs to spaces and - // the exact match fails repeatedly. - b.WriteString("- read_file returns lines as `NUMBER|CONTENT`. The `|` is metadata only. When calling edit_file, copy **only** the content after `|` into old_string/new_string — never the line number. Preserve tabs and spaces exactly (do not expand tabs to spaces). Line endings are matched automatically.\n") - b.WriteString("- Before every edit_file call, re-read the region you are editing (read_file with offset/limit on large files). edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") + // Harness guidance for the read → edit loop. edit_file matches + // old_string byte for byte, so anything the model does to what it + // read — expanding tabs, trimming, reformatting — is what makes the + // match fail. The read is verbatim precisely so there is nothing to + // undo before using it. + b.WriteString("- read_file returns a header line ` — lines - of ` and a blank line, then the file's exact bytes. Everything below that blank line is file content: no line numbers, no prefixes, nothing to strip. Copy a region of it straight into edit_file's old_string. Preserve tabs and spaces exactly (do not expand tabs to spaces). new_string is written byte for byte as you send it; only an all-LF old_string is translated to CRLF when the file uses CRLF.\n") + b.WriteString("- Before every edit_file call, re-read the region you are editing (read_file with offset/limit on large files). edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If old_string is not found, the anchor itself is wrong — stale, misremembered, or reformatted — so re-read and copy it again instead of retrying with more context around it. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") } if hasTool(active, "write_file") { b.WriteString("- File creation is complete only after write_file returns a successful result. A filename in the request, planned arguments, a diff preview, or the user's statement that a file was created is not evidence that it exists. Never turn any of those into your own confirmation. If asked whether a file exists, where it was written, or what it contains without a successful write_file result in this run, check it with read_file first and report the real result. Do not retry the same failed write; explain its actionable error or use a genuinely different valid path/content.\n") diff --git a/internal/agent/prompt_file_notes_test.go b/internal/agent/prompt_file_notes_test.go new file mode 100644 index 0000000..aa3fd0e --- /dev/null +++ b/internal/agent/prompt_file_notes_test.go @@ -0,0 +1,114 @@ +package agent + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/store" + "github.com/enowdev/antares/internal/tools" +) + +// filePrompt assembles the system prompt with only the two file tools active, +// so the assertions below are about the read → edit guidance and nothing else. +func filePrompt(t *testing.T) string { + t.Helper() + cfg := config.Default() + cfg.Memory.Enabled = false + a := agentWithConfig(cfg) + sess := &store.Session{ID: "s", Workspace: "/workspace", Meta: store.Meta{}} + var active []tools.Tool + for _, name := range []string{"read_file", "edit_file"} { + tool, ok := tools.Default().Get(name) + if !ok { + t.Fatalf("%s is not registered", name) + } + active = append(active, tool) + } + return a.buildSystemPrompt(context.Background(), Request{}, sess, active) +} + +// The prompt is the only description of the tools the model ever sees, so an +// instruction that no longer matches them is not stale documentation — it is a +// standing order to corrupt data. read_file adds no prefix to a line, and +// new_string is authored rather than copied, so telling the model to keep only +// what follows a "|" deletes real content from files whose lines start with one. +func TestPromptFileNotesDoNotDescribeALineNumberPrefix(t *testing.T) { + prompt := filePrompt(t) + for _, gone := range []string{ + "NUMBER|", + "NUMBER|CONTENT", + "metadata only", + "content after `|`", + "never the line number", + "Line endings are matched automatically", + } { + if strings.Contains(prompt, gone) { + t.Errorf("prompt still describes the removed line-prefix format: %q", gone) + } + } +} + +// Dropping the wrong advice is only half the job. These four are what keeps the +// read → edit loop working, and each one is now true of the code: the header +// says which lines came back, the bytes below it are the file's own, the anchor +// must be exact and unique, and whitespace is part of it. +func TestPromptFileNotesDescribeVerbatimReadsAndExactAnchors(t *testing.T) { + prompt := filePrompt(t) + for _, want := range []string{ + "lines - of ", + "the file's exact bytes", + "exact, unique old_string", + "Preserve tabs and spaces exactly", + "re-read the region you are editing", + } { + if !strings.Contains(prompt, want) { + t.Errorf("prompt no longer says %q", want) + } + } +} + +// A missed anchor is a stale or misremembered anchor; the bytes the model sent +// are not in the file, and no amount of extra context around them will find +// them. The old advice sent models into a retry loop that could not terminate, +// so the prompt has to name the real cause and the one move that works. +func TestPromptSendsAFailedMatchBackToTheFile(t *testing.T) { + prompt := filePrompt(t) + if !strings.Contains(prompt, "the anchor itself is wrong") { + t.Errorf("prompt does not tell the model a failed match means a wrong anchor:\n%s", prompt) + } +} + +// Nothing above is worth asserting if read_file has gone back to decorating +// lines: the prompt would be lying again, in the same way and to the same cost. +func TestPromptClaimOfVerbatimReadsHoldsAgainstTheRealTool(t *testing.T) { + workspace := t.TempDir() + original := "| Date | Event |\n|---|---|\n\tindented\n" + if err := os.WriteFile(filepath.Join(workspace, "table.md"), []byte(original), 0o644); err != nil { + t.Fatal(err) + } + read, ok := tools.Default().Get("read_file") + if !ok { + t.Fatal("read_file is not registered") + } + res := read.Execute(context.Background(), tools.Input{ + Workspace: workspace, + Args: []byte(`{"path":"table.md"}`), + }) + if res.IsError { + t.Fatalf("read_file: %s", res.Content) + } + header, body, ok := strings.Cut(res.Content, "\n\n") + if !ok { + t.Fatalf("read_file output has no header line followed by a blank line: %q", res.Content) + } + if header != "table.md — lines 1-3 of 3" { + t.Errorf("header is not the shape the prompt describes: %q", header) + } + if body != original { + t.Errorf("prompt promises the file's exact bytes, read_file returned %q", body) + } +} diff --git a/internal/tools/edit_eol_advisory_test.go b/internal/tools/edit_eol_advisory_test.go new file mode 100644 index 0000000..bcb06e6 --- /dev/null +++ b/internal/tools/edit_eol_advisory_test.go @@ -0,0 +1,80 @@ +package tools + +import ( + "strings" + "testing" +) + +// An anchor that matched the file's own bytes authorises no rewriting of the +// replacement, so LF breaks in new_string reach a CRLF file as LF. That is the +// right trade — every byte asked for is on disk — but it leaves the file with +// two line-ending flavors, and the caller is the only one who can decide +// whether that matters. Saying so costs nothing and is the difference between +// a visible consequence and a silent one. +func TestEditNotesLFReplacementLandingInACRLFFile(t *testing.T) { + said, isError, after := editOnDisk(t, "win.txt", "alpha\r\nbeta\r\ngamma\r\n", map[string]any{ + "path": "win.txt", + "old_string": "beta\r\ngamma", + "new_string": "beta\nGAMMA", + }) + if isError { + t.Fatalf("a byte-exact anchor was refused: %s", said) + } + if want := "alpha\r\nbeta\nGAMMA\r\n"; after != want { + t.Fatalf("new_string was not written verbatim\nwant %q\ngot %q", want, after) + } + for _, want := range []string{"CRLF line endings", "LF line breaks"} { + if !strings.Contains(said, want) { + t.Errorf("success message does not mention %q: %s", want, said) + } + } +} + +// The note describes one situation, so it must appear in exactly that one. On +// every other path it is either false or noise, and a note the caller learns to +// ignore is worse than no note at all. +func TestEditDoesNotNoteLineEndingsOnAnyOtherPath(t *testing.T) { + for _, tc := range []struct { + name, original, oldString, newString, want string + }{ + { + "an LF file has nothing to be inconsistent with", + "alpha\nbeta\ngamma\n", "beta\ngamma", "beta\nGAMMA", + "alpha\nbeta\nGAMMA\n", + }, + { + "a replacement written in the file's own endings is consistent", + "alpha\r\nbeta\r\ngamma\r\n", "beta\r\ngamma", "beta\r\nGAMMA", + "alpha\r\nbeta\r\nGAMMA\r\n", + }, + { + "a replacement with no line break has no line ending to differ in", + "alpha\r\nbeta\r\ngamma\r\n", "beta", "BETA", + "alpha\r\nBETA\r\ngamma\r\n", + }, + { + // The recovery stage translates new_string because it had to + // translate old_string, so CRLF is what actually reaches the file. + "the LF-to-CRLF recovery already wrote CRLF", + "alpha\r\nbeta\r\ngamma\r\n", "beta\ngamma", "beta\nGAMMA", + "alpha\r\nbeta\r\nGAMMA\r\n", + }, + } { + t.Run(tc.name, func(t *testing.T) { + said, isError, after := editOnDisk(t, "f.txt", tc.original, map[string]any{ + "path": "f.txt", + "old_string": tc.oldString, + "new_string": tc.newString, + }) + if isError { + t.Fatalf("edit refused: %s", said) + } + if after != tc.want { + t.Fatalf("edited bytes = %q, want %q", after, tc.want) + } + if strings.Contains(said, "Note:") { + t.Errorf("line-ending note appeared where it does not apply: %s", said) + } + }) + } +} diff --git a/internal/tools/file.go b/internal/tools/file.go index bdd516e..e1d78d1 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -397,6 +397,16 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { if how != "" { msg += " [" + how + "]" } + // A verbatim match authorises no rewriting of the replacement, so LF breaks + // in new_string stay LF inside a CRLF file. Every byte asked for is on disk + // and that is the trade we want, but the mixed endings are invisible until + // something else surfaces them, so the caller is told. This changes nothing + // about what was written. newString is the string that actually reached + // disk, so the LF-to-CRLF recovery — which already translated it — cannot + // trip this. + if fileEOL(content) == "\r\n" && strings.Contains(newString, "\n") && !strings.Contains(newString, "\r\n") { + msg += " Note: the file uses CRLF line endings and new_string used LF, so the replaced region now has LF line breaks. It was written exactly as given; send new_string with \\r\\n breaks if the file must stay consistent." + } return Result{ Content: msg, Meta: map[string]any{"path": rel, "replacements": replaced}, From 6d277e4030ed260f2cd7f3de6eaba82f764ecbf6 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:03:01 +0700 Subject: [PATCH 10/20] Say only what the file tools actually do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two claims added in the previous commit were false, which is the defect class this branch exists to remove: - "Everything below that blank line is file content" ignored the trailing "… N more lines" and "… file truncated" notes read_file appends, on exactly the large files the same bullet tells the model to page through. - "new_string is written byte for byte" is not true on the LF-to-CRLF recovery, which translates new_string because old_string had to be. docs/tools.md asserted it and then contradicted itself four lines later. The prompt tests now establish each fact from the real tool before asserting the prompt states it, and judge the claim one sentence at a time, so a true clause can no longer keep a false neighbour green. Co-authored-by: Cursor --- docs/tools.md | 22 +++-- internal/agent/prompt.go | 15 +-- internal/agent/prompt_file_notes_test.go | 115 +++++++++++++++++++++++ 3 files changed, 138 insertions(+), 14 deletions(-) diff --git a/docs/tools.md b/docs/tools.md index a7f558e..649ec43 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -28,14 +28,20 @@ to strip before using a region as an `edit_file` anchor. The same numbers are in the result's metadata (`path`, `first_line`, `last_line`, `total_lines`) for callers that would otherwise parse the header. -`edit_file` matches `old_string` byte for byte and writes `new_string` byte for -byte. One recovery exists: if the file uses CRLF and an anchor whose every break -is LF did not match, it is retried with those breaks expanded to CRLF, since a -model emits `\n` whatever it read. That runs only after the exact match has -failed, translates `new_string` only because `old_string` had to be, and says so -in its result. So when the anchor matched exactly, a `new_string` written with -LF lands in a CRLF file as LF; the result notes that rather than rewriting bytes -the caller asked for. +Two things below the header are the tool's rather than the file's: a clipped +read appends `… N more lines (use offset=N to continue)`, and a read that hit +the byte cap appends `… file truncated at 400 KB`. Each sits after a blank line +and begins with `…`, which is how the model is told to recognise them; an +anchor must never be taken from one. + +`edit_file` matches `old_string` byte for byte, and writes `new_string` byte for +byte on every path but one. That path is the single recovery: if the file uses +CRLF and an anchor whose every break is LF did not match, it is retried with +those breaks expanded to CRLF, since a model emits `\n` whatever it read. It +runs only after the exact match has failed, translates `new_string` only because +`old_string` had to be, and says so in its result. So when the anchor matched +exactly, a `new_string` written with LF lands in a CRLF file as LF; the result +notes that rather than rewriting bytes the caller asked for. An anchor that is not found is an anchor that is wrong — stale, misremembered, or reformatted. The fix is another `read_file`, not more surrounding context. diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index b38a247..fc10223 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -103,12 +103,15 @@ help them now — do not block them. b.WriteString("\n## Tool notes\n\n") b.WriteString("- Paths given to file tools are relative to the workspace; you cannot read outside it.\n") if hasTool(active, "read_file") || hasTool(active, "edit_file") { - // Harness guidance for the read → edit loop. edit_file matches - // old_string byte for byte, so anything the model does to what it - // read — expanding tabs, trimming, reformatting — is what makes the - // match fail. The read is verbatim precisely so there is nothing to - // undo before using it. - b.WriteString("- read_file returns a header line ` — lines - of ` and a blank line, then the file's exact bytes. Everything below that blank line is file content: no line numbers, no prefixes, nothing to strip. Copy a region of it straight into edit_file's old_string. Preserve tabs and spaces exactly (do not expand tabs to spaces). new_string is written byte for byte as you send it; only an all-LF old_string is translated to CRLF when the file uses CRLF.\n") + // Harness guidance for the read → edit loop. The two tools share + // one contract — what a read returns is what an edit can find — so + // anything done to what was read (expanding tabs, trimming, + // reformatting) is what makes a match fail. Every factual claim + // below is pinned against the real tools by + // prompt_file_notes_test.go: an instruction that overstates what + // they do is how files get corrupted. + b.WriteString("- read_file returns a header line ` — lines - of `, a blank line, then the file's exact bytes: no line numbers, no prefixes, nothing to strip. Below that blank line everything is file content except the trailing notes a clipped read adds, which begin with `…` (more lines to page through, or the 400 KB cap) and are preceded by a blank line — never copy one of those into old_string. Copy any other region straight into edit_file's old_string. Preserve tabs and spaces exactly (do not expand tabs to spaces).\n") + b.WriteString("- edit_file matches old_string byte for byte and writes new_string byte for byte, with one exception: if the file uses CRLF and an all-LF old_string does not match, the tool retries it with CRLF breaks, and if that matches, new_string's line breaks are expanded to CRLF too. The result message flags that translation whenever it happens.\n") b.WriteString("- Before every edit_file call, re-read the region you are editing (read_file with offset/limit on large files). edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If old_string is not found, the anchor itself is wrong — stale, misremembered, or reformatted — so re-read and copy it again instead of retrying with more context around it. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") } if hasTool(active, "write_file") { diff --git a/internal/agent/prompt_file_notes_test.go b/internal/agent/prompt_file_notes_test.go index aa3fd0e..8ac71da 100644 --- a/internal/agent/prompt_file_notes_test.go +++ b/internal/agent/prompt_file_notes_test.go @@ -82,6 +82,121 @@ func TestPromptSendsAFailedMatchBackToTheFile(t *testing.T) { } } +// claimAround returns the single sentence of the prompt that makes a claim +// mentioning needle. A claim has to be judged whole: an assertion that searches +// the entire prompt is satisfied by a true clause sitting next to a false one, +// which is how a sentence that scoped a translation to old_string alone stayed +// green while the tool was translating new_string too. +func claimAround(t *testing.T, prompt, needle string) string { + t.Helper() + at := strings.Index(prompt, needle) + if at < 0 { + t.Fatalf("prompt makes no claim mentioning %q", needle) + } + start := 0 + if i := strings.LastIndex(prompt[:at], ". "); i >= 0 { + start = i + 2 + } + if i := strings.LastIndex(prompt[:at], "\n"); i >= start { + start = i + 1 + } + end := len(prompt) + if i := strings.Index(prompt[at:], ". "); i >= 0 { + end = at + i + 1 + } + if i := strings.Index(prompt[at:], "\n"); i >= 0 && at+i < end { + end = at + i + } + return strings.TrimSpace(prompt[start:end]) +} + +// read_file appends a note of its own whenever it clips a read, so the lines +// below the header are not all file content — and the bullet making that claim +// is the same one that sends the model through large files with offset and +// limit, which is precisely when the note appears. Taken literally, an +// unqualified claim invites "… 2 more lines" into an anchor. +func TestPromptExemptsTheToolsOwnTrailingNoteFromTheContentClaim(t *testing.T) { + const noteMarker = "…" + + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "notes.txt"), []byte("one\ntwo\nthree\nfour\nfive\n"), 0o644); err != nil { + t.Fatal(err) + } + read, ok := tools.Default().Get("read_file") + if !ok { + t.Fatal("read_file is not registered") + } + res := read.Execute(context.Background(), tools.Input{ + Workspace: workspace, + Args: []byte(`{"path":"notes.txt","offset":2,"limit":2}`), + }) + if res.IsError { + t.Fatalf("read_file: %s", res.Content) + } + _, below, ok := strings.Cut(res.Content, "\n\n") + if !ok { + t.Fatalf("read_file output has no header line followed by a blank line: %q", res.Content) + } + const inTheFile = "two\nthree\n" + if !strings.HasPrefix(below, inTheFile) { + t.Fatalf("read_file did not return the range's own bytes first: %q", below) + } + added := strings.TrimPrefix(below, inTheFile) + if strings.TrimSpace(added) == "" { + t.Fatal("a clipped read no longer appends a note; the prompt's exception for one is stale and should go") + } + for _, line := range strings.Split(strings.Trim(added, "\n"), "\n") { + if !strings.HasPrefix(line, noteMarker) { + t.Fatalf("read_file appends a line the prompt gives the model no way to tell from content: %q", line) + } + } + + claim := claimAround(t, filePrompt(t), "that blank line") + if !strings.Contains(claim, noteMarker) { + t.Errorf("prompt claims file content below the header without exempting the %q note read_file just appended: %q", noteMarker, claim) + } +} + +// The recovery stage translates new_string because it had to translate +// old_string, so "written byte for byte" is false on exactly the path the rest +// of the same sentence describes. The tool is right to do it — the alternative +// is refusing an anchor it can match losslessly — so it is the sentence that +// has to change. +func TestPromptQualifiesTheByteForByteWriteTheRecoveryStageBreaks(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "win.txt") + if err := os.WriteFile(path, []byte("alpha\r\nbeta\r\ngamma\r\n"), 0o644); err != nil { + t.Fatal(err) + } + edit, ok := tools.Default().Get("edit_file") + if !ok { + t.Fatal("edit_file is not registered") + } + // An all-LF anchor against a CRLF file: only the recovery stage can match it. + res := edit.Execute(context.Background(), tools.Input{ + Workspace: workspace, + Args: []byte(`{"path":"win.txt","old_string":"beta\ngamma","new_string":"beta\nGAMMA"}`), + }) + if res.IsError { + t.Fatalf("edit_file: %s", res.Content) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(after), "beta\nGAMMA") { + t.Fatal("new_string reached disk byte for byte here; the prompt's exception for the recovery stage is stale and should go") + } + if want := "alpha\r\nbeta\r\nGAMMA\r\n"; string(after) != want { + t.Fatalf("recovery stage wrote %q, want %q", after, want) + } + + claim := claimAround(t, filePrompt(t), "byte for byte") + if !strings.Contains(claim, "new_string's line breaks are expanded to CRLF") { + t.Errorf("prompt promises a byte-for-byte write and does not name the translation edit_file just applied to new_string: %q", claim) + } +} + // Nothing above is worth asserting if read_file has gone back to decorating // lines: the prompt would be lying again, in the same way and to the same cost. func TestPromptClaimOfVerbatimReadsHoldsAgainstTheRealTool(t *testing.T) { From ef30b94a5508dfb51009f5fd5acacae203a392f9 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:11:01 +0700 Subject: [PATCH 11/20] Promise only the note marker, which always holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clause added with the last fix said the trailing notes are preceded by a blank line. read_file writes a note as "\n…" onto whatever body ends with, so that blank line exists only when the body already ended in its own newline. On the byte-cap path it does not: a file over 400 KB whose first 400 KB holds one long line gets "…xxx\n… file truncated at 400 KB\n", and the newline in front of the note is the tool's, not the file's. Telling the model otherwise says the last content line is terminated when it is not, which is the retry loop the third bullet exists to prevent. The marker is the rule that holds on both branches, so it is the only one offered. The read test now drives the byte cap as well as the line clip, asserts the real tail of each, and forbids the prompt from describing what precedes a note unless every branch puts it there. Co-authored-by: Cursor --- docs/tools.md | 8 +- internal/agent/prompt.go | 2 +- internal/agent/prompt_file_notes_test.go | 135 ++++++++++++++++++----- 3 files changed, 112 insertions(+), 33 deletions(-) diff --git a/docs/tools.md b/docs/tools.md index 649ec43..797d534 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -30,9 +30,11 @@ callers that would otherwise parse the header. Two things below the header are the tool's rather than the file's: a clipped read appends `… N more lines (use offset=N to continue)`, and a read that hit -the byte cap appends `… file truncated at 400 KB`. Each sits after a blank line -and begins with `…`, which is how the model is told to recognise them; an -anchor must never be taken from one. +the byte cap appends `… file truncated at 400 KB`. Each occupies a line of its +own beginning with `…`, which is how the model is told to recognise them; an +anchor must never be taken from one. The newline that starts that line is the +tool's, so on the byte-cap path it is not evidence that the last content line +was terminated in the file. `edit_file` matches `old_string` byte for byte, and writes `new_string` byte for byte on every path but one. That path is the single recovery: if the file uses diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index fc10223..5e0b420 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -110,7 +110,7 @@ help them now — do not block them. // below is pinned against the real tools by // prompt_file_notes_test.go: an instruction that overstates what // they do is how files get corrupted. - b.WriteString("- read_file returns a header line ` — lines - of `, a blank line, then the file's exact bytes: no line numbers, no prefixes, nothing to strip. Below that blank line everything is file content except the trailing notes a clipped read adds, which begin with `…` (more lines to page through, or the 400 KB cap) and are preceded by a blank line — never copy one of those into old_string. Copy any other region straight into edit_file's old_string. Preserve tabs and spaces exactly (do not expand tabs to spaces).\n") + b.WriteString("- read_file returns a header line ` — lines - of `, a blank line, then the file's exact bytes: no line numbers, no prefixes, nothing to strip. Below that blank line everything is file content except the trailing notes a clipped read adds, each on its own line beginning with `…` (more lines to page through, or the 400 KB cap) — never copy one of those into old_string. Copy any other region straight into edit_file's old_string. Preserve tabs and spaces exactly (do not expand tabs to spaces).\n") b.WriteString("- edit_file matches old_string byte for byte and writes new_string byte for byte, with one exception: if the file uses CRLF and an all-LF old_string does not match, the tool retries it with CRLF breaks, and if that matches, new_string's line breaks are expanded to CRLF too. The result message flags that translation whenever it happens.\n") b.WriteString("- Before every edit_file call, re-read the region you are editing (read_file with offset/limit on large files). edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If old_string is not found, the anchor itself is wrong — stale, misremembered, or reformatted — so re-read and copy it again instead of retrying with more context around it. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") } diff --git a/internal/agent/prompt_file_notes_test.go b/internal/agent/prompt_file_notes_test.go index 8ac71da..b65203f 100644 --- a/internal/agent/prompt_file_notes_test.go +++ b/internal/agent/prompt_file_notes_test.go @@ -87,6 +87,14 @@ func TestPromptSendsAFailedMatchBackToTheFile(t *testing.T) { // the entire prompt is satisfied by a true clause sitting next to a false one, // which is how a sentence that scoped a translation to old_string alone stayed // green while the tool was translating new_string too. +// +// It knows two sentence boundaries, ". " and a newline, which is enough for the +// tool notes as written and has two consequences for anyone rewording them. An +// abbreviation ("e.g.") ends the sentence early and fails the assertion loudly. +// A sentence ended with "!" or "?" does not end it at all: the claim merges with +// the sentence after it and can be satisfied by a qualifier that is no longer in +// the same sentence — a silent pass. Keep these bullets to plain full stops, or +// teach this function the terminator you introduce. func claimAround(t *testing.T, prompt, needle string) string { t.Helper() at := strings.Index(prompt, needle) @@ -113,48 +121,117 @@ func claimAround(t *testing.T, prompt, needle string) string { // read_file appends a note of its own whenever it clips a read, so the lines // below the header are not all file content — and the bullet making that claim // is the same one that sends the model through large files with offset and -// limit, which is precisely when the note appears. Taken literally, an +// limit, which is precisely when a note appears. Taken literally, an // unqualified claim invites "… 2 more lines" into an anchor. -func TestPromptExemptsTheToolsOwnTrailingNoteFromTheContentClaim(t *testing.T) { +// +// Both branches that append a note are driven, because they do not produce the +// same shape. The note is written as "\n…" onto whatever the body ended with, +// so a blank line appears in front of it only when the body already ended in a +// newline of its own. On the byte-cap branch it does not, and that leading "\n" +// is the tool's rather than the file's. +func TestPromptExemptsTheToolsOwnTrailingNotesFromTheContentClaim(t *testing.T) { const noteMarker = "…" + // maxReadBytes over in internal/tools. A file past it is truncated and noted. + const byteCap = 400 * 1024 + oversized := strings.Repeat("x", byteCap+64) - workspace := t.TempDir() - if err := os.WriteFile(filepath.Join(workspace, "notes.txt"), []byte("one\ntwo\nthree\nfour\nfive\n"), 0o644); err != nil { - t.Fatal(err) - } read, ok := tools.Default().Get("read_file") if !ok { t.Fatal("read_file is not registered") } - res := read.Execute(context.Background(), tools.Input{ - Workspace: workspace, - Args: []byte(`{"path":"notes.txt","offset":2,"limit":2}`), - }) - if res.IsError { - t.Fatalf("read_file: %s", res.Content) - } - _, below, ok := strings.Cut(res.Content, "\n\n") - if !ok { - t.Fatalf("read_file output has no header line followed by a blank line: %q", res.Content) - } - const inTheFile = "two\nthree\n" - if !strings.HasPrefix(below, inTheFile) { - t.Fatalf("read_file did not return the range's own bytes first: %q", below) + + blankLineAlways := true + for _, tc := range []struct{ name, file, content, args, inTheFile, wantTail string }{ + { + "clipped by line range", + "notes.txt", "one\ntwo\nthree\nfour\nfive\n", + `{"path":"notes.txt","offset":2,"limit":2}`, + "two\nthree\n", + "three\n\n… 2 more lines (use offset=4 to continue)\n", + }, + { + // One line, longer than the cap: no "more lines" note fires, and + // the cap note lands straight onto a content byte. A minified + // bundle, a single-line JSON document or a long-lined log. + "clipped by the 400 KB byte cap", + "bundle.min.js", oversized, + `{"path":"bundle.min.js"}`, + oversized[:byteCap], + "x\n… file truncated at 400 KB\n", + }, + } { + t.Run(tc.name, func(t *testing.T) { + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, tc.file), []byte(tc.content), 0o644); err != nil { + t.Fatal(err) + } + res := read.Execute(context.Background(), tools.Input{ + Workspace: workspace, + Args: []byte(tc.args), + }) + if res.IsError { + t.Fatalf("read_file: %s", res.Content) + } + if !strings.HasSuffix(res.Content, tc.wantTail) { + t.Fatalf("read ends %q, want it to end %q — the prompt describes this shape and has to be revisited with it", + lastBytes(res.Content, len(tc.wantTail)+8), tc.wantTail) + } + _, below, ok := strings.Cut(res.Content, "\n\n") + if !ok { + t.Fatalf("read_file output has no header line followed by a blank line: %q", lastBytes(res.Content, 120)) + } + if !strings.HasPrefix(below, tc.inTheFile) { + t.Fatalf("read_file did not return the range's own bytes first: %q", lastBytes(below, 120)) + } + added := strings.TrimPrefix(below, tc.inTheFile) + if strings.TrimSpace(added) == "" { + t.Fatal("this branch no longer appends a note; the prompt's exception for one is stale and should go") + } + // The marker is the only property that holds on both branches, so + // it is the only one the prompt may offer as the recognition rule. + for _, line := range strings.Split(strings.Trim(added, "\n"), "\n") { + if !strings.HasPrefix(line, noteMarker) { + t.Fatalf("read_file adds a line the prompt gives the model no way to tell from content: %q", line) + } + } + shown := strings.Split(strings.TrimSuffix(below, "\n"), "\n") + for i, line := range shown { + if !strings.HasPrefix(line, noteMarker) { + continue + } + if i == 0 || shown[i-1] != "" { + blankLineAlways = false + } + break + } + }) } - added := strings.TrimPrefix(below, inTheFile) - if strings.TrimSpace(added) == "" { - t.Fatal("a clipped read no longer appends a note; the prompt's exception for one is stale and should go") + + claim := claimAround(t, filePrompt(t), "that blank line") + if !strings.Contains(claim, noteMarker) { + t.Errorf("prompt claims file content below the header without exempting the %q notes read_file just appended: %q", noteMarker, claim) } - for _, line := range strings.Split(strings.Trim(added, "\n"), "\n") { - if !strings.HasPrefix(line, noteMarker) { - t.Fatalf("read_file appends a line the prompt gives the model no way to tell from content: %q", line) + // The prompt may describe what sits in front of a note only if every branch + // puts it there. One does not, so promising it would tell the model the last + // content line is terminated when the terminator it sees is the tool's. + if !blankLineAlways { + for _, promise := range []string{ + "preceded by a blank line", "after a blank line", + "behind a blank line", "following a blank line", + } { + if strings.Contains(claim, promise) { + t.Errorf("prompt says a note is %q, but a read above put one straight onto a content byte: %q", promise, claim) + } } } +} - claim := claimAround(t, filePrompt(t), "that blank line") - if !strings.Contains(claim, noteMarker) { - t.Errorf("prompt claims file content below the header without exempting the %q note read_file just appended: %q", noteMarker, claim) +// lastBytes keeps a failure message readable when the subject is a 400 KB read. +func lastBytes(s string, n int) string { + if len(s) <= n { + return s } + return "…" + s[len(s)-n:] } // The recovery stage translates new_string because it had to translate From b850be8a9c9d7f4c3230f4ec26cb13b0aa623e99 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:28:22 +0700 Subject: [PATCH 12/20] Bound read_file's read, not only its result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 400 KB cap ran after os.ReadFile had already loaded the file, so it bounded what reached the model and nothing bounded what reached the process. An ordinary large workspace file was held whole to be trimmed, and a file that understates its size — a character device, most of /proc, a file being appended to, /dev/zero — ended the process in a fatal out-of-memory. grep's gate was moved onto the read for exactly this reason; read_file's now is too. Co-authored-by: Cursor --- internal/tools/file.go | 13 +++- internal/tools/read_bounded_test.go | 95 +++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 internal/tools/read_bounded_test.go diff --git a/internal/tools/file.go b/internal/tools/file.go index e1d78d1..20da638 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "errors" "fmt" + "io" "os" "path/filepath" "sort" @@ -171,7 +172,17 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { return Errorf("%s is a directory; use list_files instead", args.Path) } - data, err := os.ReadFile(path) + f, err := os.Open(path) + if err != nil { + return Errorf("cannot read %s: %v", args.Path, err) + } + defer f.Close() + // The cap has to bound the read, not trim what has already been read. The + // size stat reports cannot bound it either: a character device, most of + // /proc, and a file being appended to during the read all yield more than + // stat promised, and /dev/zero reports zero bytes and never ends. Reading + // one byte past the cap is what tells a file at the cap from one over it. + data, err := io.ReadAll(io.LimitReader(f, maxReadBytes+1)) if err != nil { return Errorf("cannot read %s: %v", args.Path, err) } diff --git a/internal/tools/read_bounded_test.go b/internal/tools/read_bounded_test.go new file mode 100644 index 0000000..c22d85f --- /dev/null +++ b/internal/tools/read_bounded_test.go @@ -0,0 +1,95 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +// The 400 KB cap decides what reaches the model. What reaches the process is a +// separate question, and until the read itself is bounded the cap answers it +// only by accident: a file is loaded whole and then trimmed, so an ordinary +// workspace file that is simply large takes the process's memory with it before +// any of this code runs. grep's gate was moved onto the read for exactly this +// reason (search.go), and read_file's has to be too. +func TestReadFileBoundsTheReadAndNotOnlyTheResult(t *testing.T) { + // A large file needs no device node and no project session to reach: a + // core dump, a vendored bundle or a captured log inside the workspace is + // enough, and holding it whole is the cost the cap was supposed to remove. + t.Run("an oversized file is never held whole", func(t *testing.T) { + const size = 64 << 20 + workspace := t.TempDir() + writeSparseFile(t, filepath.Join(workspace, "core.dump"), "header line\n", size) + args, err := json.Marshal(map[string]any{"path": "core.dump"}) + if err != nil { + t.Fatal(err) + } + + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + res := (readFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + runtime.ReadMemStats(&after) + if res.IsError { + t.Fatalf("read_file failed on a large text file: %s", res.Content) + } + + // Generous next to the 64 MB file and far above what a bounded read of + // 400 KB costs, so this fails on the defect rather than on allocator + // noise. + const budget = 8 << 20 + if grew := after.TotalAlloc - before.TotalAlloc; grew > budget { + t.Errorf("read_file allocated %d bytes to return 400 KB of a %d-byte file; the cap has to bound the read, not trim what was already read", grew, size) + } + }) + + // Nor is the size a file states a bound. A character device, most of /proc + // and /sys, and a file being appended to during the read all yield more + // than stat promised; /dev/zero reports zero bytes and never ends. A + // project session leaves reads unconfined, so a path like this one is + // reachable through resolveRead as written. + t.Run("a file that understates its size is still bounded", func(t *testing.T) { + if _, err := os.Stat("/dev/zero"); err != nil { + t.Skip("no /dev/zero to read from on this platform") + } + workspace := t.TempDir() + args, err := json.Marshal(map[string]any{"path": "/dev/zero"}) + if err != nil { + t.Fatal(err) + } + + // Off the test goroutine, so an unbounded read fails the test instead + // of hanging the package until the go test deadline. + done := make(chan Result, 1) + go func() { + done <- (readFileTool{}).Execute(context.Background(), Input{ + Workspace: workspace, WriteRoots: []string{workspace}, Args: args, + }) + }() + select { + case res := <-done: + if res.IsError { + t.Fatalf("read_file failed: %s", firstBytes(res.Content, 200)) + } + if !strings.Contains(res.Content, "truncated at 400 KB") { + t.Errorf("an endless file came back without the truncation notice: %q", firstBytes(res.Content, 200)) + } + case <-time.After(10 * time.Second): + t.Fatal("read_file did not return: the read is not bounded by the size cap") + } + }) +} + +// firstBytes keeps a failure message readable when the subject is a 400 KB read +// of NUL bytes. +func firstBytes(s string, n int) string { + if len(s) <= n { + return s + } + return s[:n] + "…" +} From 7824d12845bbb4a15317dd5dd7839e0999d1b202 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:31:28 +0700 Subject: [PATCH 13/20] State no line total the read did not count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Past 400 KB read_file trimmed the bytes and then counted the lines it had left, so it reported the head's line count as the file's total. grep counts the whole file, so the two disagreed above the cap: grep found NEEDLE at line 15001 of a 600 KB log and read_file called the same file 10240 lines long and refused that offset as past its end — the retry loop this branch exists to remove, moved one tool along. Learning the real total means reading past the cap, which is what the cap is for, so the tool states a floor instead of a count: the header reads "of ≥N", the continuation note "at least N more lines", and Meta drops total_lines for truncated. An offset beyond the cap is still refused, now naming the cap and pointing at grep, which reads the whole file. Co-authored-by: Cursor --- docs/tools.md | 8 ++ internal/agent/prompt.go | 2 +- internal/tools/file.go | 39 +++++-- internal/tools/read_truncated_test.go | 140 ++++++++++++++++++++++++++ 4 files changed, 180 insertions(+), 9 deletions(-) create mode 100644 internal/tools/read_truncated_test.go diff --git a/docs/tools.md b/docs/tools.md index 797d534..a49150f 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -36,6 +36,14 @@ anchor must never be taken from one. The newline that starts that line is the tool's, so on the byte-cap path it is not evidence that the last content line was terminated in the file. +A read the 400 KB cap stopped has not counted the lines behind it, and says so +rather than reporting what it read as the whole: the header's total becomes +`≥N`, the continuation note says `at least N more lines`, and the metadata +drops `total_lines` for `truncated: true` — `last_line` is then the floor the +header states. `grep` counts the whole file (up to 8 MB), so it is the tool +that can still find something past the cap; asking `read_file` to page to a +line beyond it is refused, naming the cap. + `edit_file` matches `old_string` byte for byte, and writes `new_string` byte for byte on every path but one. That path is the single recovery: if the file uses CRLF and an anchor whose every break is LF did not match, it is retried with diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 5e0b420..29ea196 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -110,7 +110,7 @@ help them now — do not block them. // below is pinned against the real tools by // prompt_file_notes_test.go: an instruction that overstates what // they do is how files get corrupted. - b.WriteString("- read_file returns a header line ` — lines - of `, a blank line, then the file's exact bytes: no line numbers, no prefixes, nothing to strip. Below that blank line everything is file content except the trailing notes a clipped read adds, each on its own line beginning with `…` (more lines to page through, or the 400 KB cap) — never copy one of those into old_string. Copy any other region straight into edit_file's old_string. Preserve tabs and spaces exactly (do not expand tabs to spaces).\n") + b.WriteString("- read_file returns a header line ` — lines - of `, a blank line, then the file's exact bytes: no line numbers, no prefixes, nothing to strip. A read the 400 KB cap stopped never counted the rest of the file, so its total comes back as `≥N`, a floor rather than a count. Below that blank line everything is file content except the trailing notes a clipped read adds, each on its own line beginning with `…` (more lines to page through, or the 400 KB cap) — never copy one of those into old_string. Copy any other region straight into edit_file's old_string. Preserve tabs and spaces exactly (do not expand tabs to spaces).\n") b.WriteString("- edit_file matches old_string byte for byte and writes new_string byte for byte, with one exception: if the file uses CRLF and an all-LF old_string does not match, the tool retries it with CRLF breaks, and if that matches, new_string's line breaks are expanded to CRLF too. The result message flags that translation whenever it happens.\n") b.WriteString("- Before every edit_file call, re-read the region you are editing (read_file with offset/limit on large files). edit_file requires an exact, unique old_string from that fresh read. After any successful edit or write, re-read before making another edit; do not reuse an older block or invent identifiers. If old_string is not found, the anchor itself is wrong — stale, misremembered, or reformatted — so re-read and copy it again instead of retrying with more context around it. If it reports multiple occurrences, include unique neighbouring lines or use replace_all only when every occurrence should change.\n") } diff --git a/internal/tools/file.go b/internal/tools/file.go index 20da638..0f5d892 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -214,6 +214,12 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { // An empty file has no line 1 to be past, so offset 1 on it reads as // "nothing here" rather than as a mistake. if start > 0 && start >= len(lines) { + if truncatedBytes { + // The line may well exist; this read simply never reached it. Told + // it is past the end, a caller following a line number grep just + // gave it concludes the file changed under it and reads again. + return Errorf("offset %d is past line %d, where the 400 KB cap stopped this read; the file continues beyond it, so search the rest with grep rather than paging to it", offset, len(lines)) + } return Errorf("offset %d is past end of file (%d lines)", offset, len(lines)) } end := start + limit @@ -240,22 +246,39 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { first = 0 } + // A read stopped by the byte cap counted the lines it read and no others, + // so every number it can offer about the whole file is a floor rather than + // a count. grep counts the file whole, and a total that disagrees with it + // sends a caller holding one of grep's line numbers back to a file it was + // just told is shorter than that. Learning the real total means reading + // past the cap, which is what the cap is for. + floor, atLeast := "", "" + if truncatedBytes { + floor, atLeast = "≥", "at least " + } rel := relTo(in.Workspace, path) var b strings.Builder - fmt.Fprintf(&b, "%s — lines %d-%d of %d\n\n", rel, first, end, len(lines)) + fmt.Fprintf(&b, "%s — lines %d-%d of %s%d\n\n", rel, first, end, floor, len(lines)) b.WriteString(body) if end < len(lines) { - fmt.Fprintf(&b, "\n… %d more lines (use offset=%d to continue)\n", len(lines)-end, end+1) + fmt.Fprintf(&b, "\n… %s%d more lines (use offset=%d to continue)\n", atLeast, len(lines)-end, end+1) } if truncatedBytes { b.WriteString("\n… file truncated at 400 KB\n") } - return Result{Content: b.String(), Meta: map[string]any{ - "path": rel, - "first_line": first, - "last_line": end, - "total_lines": len(lines), - }} + meta := map[string]any{ + "path": rel, + "first_line": first, + "last_line": end, + } + if truncatedBytes { + // last_line is the floor the header states; a caller that wants a + // count rather than a floor has to be able to tell them apart. + meta["truncated"] = true + } else { + meta["total_lines"] = len(lines) + } + return Result{Content: b.String(), Meta: meta} } // ---- write_file ------------------------------------------------------------- diff --git a/internal/tools/read_truncated_test.go b/internal/tools/read_truncated_test.go new file mode 100644 index 0000000..fee681f --- /dev/null +++ b/internal/tools/read_truncated_test.go @@ -0,0 +1,140 @@ +package tools + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// bigLog builds a log past the 400 KB read cap out of fixed-width lines, so the +// line the cap lands on is arithmetic rather than a guess: 40 bytes a line puts +// exactly 10240 lines inside the cap, with no partial line at the seam. +func bigLog(t *testing.T, lines int, needleAt int, needle string) string { + t.Helper() + var b strings.Builder + for i := 1; i <= lines; i++ { + text := fmt.Sprintf("log line %d", i) + if i == needleAt { + text = needle + } + if len(text) > 39 { + t.Fatalf("line %d does not fit the fixed width: %q", i, text) + } + fmt.Fprintf(&b, "%-39s\n", text) + } + return b.String() +} + +var readHeaderRange = regexp.MustCompile(`^(.*) — lines (\d+)-(\d+) of (≥?)(\d+)\n`) + +// headerTotal returns the total read_file's header states and whether it is +// stated as a lower bound rather than as a count. +func headerTotal(t *testing.T, out string) (total int, isLowerBound bool) { + t.Helper() + m := readHeaderRange.FindStringSubmatch(out) + if m == nil { + t.Fatalf("read_file output does not open with a line-range header: %q", firstBytes(out, 120)) + } + n, err := strconv.Atoi(m[5]) + if err != nil { + t.Fatalf("header states a total that will not parse: %q", m[0]) + } + return n, m[4] == "≥" +} + +// The cap stops the read after 400 KB, so the lines behind it were never +// counted. Reporting the lines that were read as the file's total is a claim +// about bytes the tool declined to look at, and it is the claim grep +// contradicts: grep counts the whole file, so it hands back line numbers this +// tool then calls impossible. A number the caller cannot act on is worse than +// no number, because it looks like one. +func TestReadFileStatesNoTotalItHasNotCounted(t *testing.T) { + const ( + lines = 15360 // 600 KB at 40 bytes a line + needle = "NEEDLE_HERE" + ) + workspace := t.TempDir() + body := bigLog(t, lines, 15001, needle) + if err := os.WriteFile(filepath.Join(workspace, "big.log"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + if got := len(lineSpans(body)); got != lines { + t.Fatalf("fixture is %d lines, want %d", got, lines) + } + + t.Run("the header states no total the read did not count", func(t *testing.T) { + res := readFileResult(t, workspace, map[string]any{"path": "big.log", "limit": lines}) + total, isLowerBound := headerTotal(t, res.Content) + switch { + case isLowerBound && total > lines: + t.Errorf("header claims at least %d lines, and the file has %d", total, lines) + case !isLowerBound && total != lines: + t.Errorf("header states %d lines as a fact; the file has %d, and the read stopped at the cap without counting them", total, lines) + } + }) + + t.Run("Meta states no total the read did not count", func(t *testing.T) { + res := readFileResult(t, workspace, map[string]any{"path": "big.log", "limit": lines}) + if got, ok := res.Meta["total_lines"]; ok && got != lines { + t.Errorf("Meta[\"total_lines\"] = %v, and the file has %d lines; a total the read never counted does not belong in it", got, lines) + } + }) + + // The continuation note carries the same claim in another form: a caller + // paging through the file is told how much is left. + t.Run("the continuation note states no remainder the read did not count", func(t *testing.T) { + res := readFileResult(t, workspace, map[string]any{"path": "big.log", "limit": 2000}) + m := regexp.MustCompile(`… (?:at least )?(\d+) more lines`).FindStringSubmatch(res.Content) + if m == nil { + t.Fatalf("a read of 2000 lines out of %d appended no continuation note: %q", lines, firstBytes(res.Content, 160)) + } + more, err := strconv.Atoi(m[1]) + if err != nil { + t.Fatal(err) + } + if want := lines - 2000; more != want && !strings.Contains(m[0], "at least") { + t.Errorf("note says %d more lines as a fact; %d follow", more, want) + } + if more > lines-2000 { + t.Errorf("note claims at least %d more lines; only %d follow", more, lines-2000) + } + }) + + // The handoff the numbers exist for. grep reports the needle's line; the + // natural next call is read_file at that offset, and answering it with + // "past end of file" denies a line the file has. + t.Run("grep's line number is not refused as past the end of the file", func(t *testing.T) { + at := grepMatchLines(t, workspace, needle) + if len(at) != 1 || at[0] != 15001 { + t.Fatalf("grep reports the needle at %v, want [15001]", at) + } + res := readFileArgs(t, workspace, map[string]any{"path": "big.log", "offset": at[0]}) + if !res.IsError { + return // the read served the line, which is more than the claim needs + } + if strings.Contains(res.Content, "past end of file") { + t.Errorf("read_file denies a line grep just read: %s", res.Content) + } + if !strings.Contains(res.Content, "400 KB") { + t.Errorf("refusal does not name the cap that caused it: %s", res.Content) + } + }) +} + +// readFileArgs drives read_file and returns whatever it produced, error or not. +// readFileResult next door fails the test on an error result, which is right +// for a test about content and wrong for one about a refusal. +func readFileArgs(t *testing.T, workspace string, args map[string]any) Result { + t.Helper() + raw, err := json.Marshal(args) + if err != nil { + t.Fatal(err) + } + return (readFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: raw}) +} From 202dc9ecebe6ffff15b7969e7fa4de5ca2f702c8 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:32:14 +0700 Subject: [PATCH 14/20] Refuse the anchor that is in every file strings.Count(content, "") is the rune count plus one, so an empty old_string was "found" between every pair of characters: replace_all interleaved new_string with the file a character at a time and reported eighteen replacements on a two-line config, and a single replacement was refused for appearing as many times as the file has characters. It was the last input for which an exact match wrote anyway. Co-authored-by: Cursor --- internal/tools/edit_exact_test.go | 36 +++++++++++++++++++++++++++++++ internal/tools/file.go | 8 +++++++ 2 files changed, 44 insertions(+) diff --git a/internal/tools/edit_exact_test.go b/internal/tools/edit_exact_test.go index a9fcdfc..dfaa1bc 100644 --- a/internal/tools/edit_exact_test.go +++ b/internal/tools/edit_exact_test.go @@ -111,6 +111,42 @@ func TestEditReportsNoRecoveryWhenTheAnchorMatchedExactly(t *testing.T) { } } +// strings.Count(content, "") is the rune count plus one, so the empty anchor +// is "found" between every pair of runes in the file. It is the one remaining +// input for which "write only where old_string is in the file exactly" does +// not hold, and replace_all then interleaves new_string with the file a rune at +// a time. A model reaching for it is usually trying to append a line to a +// config, which is an anchor it has not chosen yet rather than an anchor that +// is empty. +func TestEditRefusesAnEmptyOldString(t *testing.T) { + const original = "# Config\nkey = 1\n" + for _, replaceAll := range []bool{true, false} { + name := "one replacement" + if replaceAll { + name = "replace_all" + } + t.Run(name, func(t *testing.T) { + said, isError, after := editOnDisk(t, "app.conf", original, map[string]any{ + "path": "app.conf", + "old_string": "", + "new_string": "key2 = 2\n", + "replace_all": replaceAll, + }) + if after != original { + t.Errorf("an empty anchor rewrote the file\nsaid: %s\ngot: %q", said, after) + } + if !isError { + t.Fatalf("an empty anchor was accepted: %s", said) + } + // Counting the places an empty string "matches" describes nothing + // the caller can fix, and the count is of the file's runes. + if !strings.Contains(said, "old_string is empty") { + t.Errorf("refusal does not name the empty anchor as the problem: %s", said) + } + }) + } +} + // A space-indented anchor against a tab-indented file is the commonest way an // edit misses, and the tool already knows how to say so. The message was // unreachable: the adjacent-insertion splice claimed the edit first and wrote diff --git a/internal/tools/file.go b/internal/tools/file.go index 0f5d892..eb7a145 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -387,6 +387,14 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { if err := in.Bind(&args); err != nil { return Errorf("%v", err) } + // An empty string is "in" every file, between every pair of characters, so + // this is the one anchor an exact match cannot refuse on its own: with + // replace_all it interleaves new_string with the file a character at a + // time, and without it the file is reported as matching as many times as + // it has characters. + if args.OldString == "" { + return Errorf("old_string is empty, and an empty anchor matches between every pair of characters. Name the text to replace: to insert a line, anchor on a line beside where it goes and repeat that line in new_string; to create or replace a whole file, use write_file.") + } if args.OldString == args.NewString { return Errorf("old_string and new_string are identical") } From 5672cef3a835bfedfee6365703388b4036183558 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:34:57 +0700 Subject: [PATCH 15/20] Count a written file the way it will be read back write_file counted "\n" and added one, so "a\nb\n" was reported as three lines where read_file, grep and edit_file all say two, and a CR- terminated file as one where they say two. It is the same file a moment apart, so both counts land in the same conversation. Co-authored-by: Cursor --- internal/tools/file.go | 8 +++--- internal/tools/line_numbering_test.go | 39 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/internal/tools/file.go b/internal/tools/file.go index eb7a145..ccc6d96 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -349,12 +349,10 @@ func (writeFileTool) Execute(_ context.Context, in Input) Result { verb = "Appended to" } rel := relTo(in.Workspace, path) - lines := 0 - if content != "" { - lines = strings.Count(content, "\n") + 1 - } + // The same splitter the other three tools count with, so what the write + // says it put in the file is what a read of it comes back with. return Result{ - Content: fmt.Sprintf("%s %s (%d bytes, %d lines)", verb, rel, len(content), lines), + Content: fmt.Sprintf("%s %s (%d bytes, %d lines)", verb, rel, len(content), len(lineSpans(content))), Meta: map[string]any{"path": rel, "bytes": len(content)}, } } diff --git a/internal/tools/line_numbering_test.go b/internal/tools/line_numbering_test.go index f654a0c..674c346 100644 --- a/internal/tools/line_numbering_test.go +++ b/internal/tools/line_numbering_test.go @@ -143,6 +143,45 @@ func editOccurrenceLines(t *testing.T, workspace, name, oldString string) []int return lines } +// write_file reports a line count as well, and it is the fourth tool in the +// same conversation: a model writes a file, is told what it now holds, and +// reads it back a moment later. Counting a terminated last line as two lines +// there and one line everywhere else makes the write look like it added +// something the read then cannot find. +func TestWriteFileCountsLinesLikeTheToolsThatReadItBack(t *testing.T) { + for _, tc := range []struct{ name, content string }{ + {"lf terminated", "alpha\nbeta\n"}, + {"lf unterminated", "alpha\nbeta"}, + {"crlf terminated", "alpha\r\nbeta\r\n"}, + {"cr terminated", "alpha\rbeta\r"}, + {"blank line before the end", "alpha\n\n"}, + {"empty file", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + workspace := t.TempDir() + args, err := json.Marshal(map[string]any{"path": "out.txt", "content": tc.content}) + if err != nil { + t.Fatal(err) + } + res := (writeFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if res.IsError { + t.Fatalf("write_file: %s", res.Content) + } + m := regexp.MustCompile(`(\d+) lines\)`).FindStringSubmatch(res.Content) + if m == nil { + t.Fatalf("write_file reported no line count: %q", res.Content) + } + said, err := strconv.Atoi(m[1]) + if err != nil { + t.Fatal(err) + } + if want := readFileTotalLines(t, workspace, "out.txt"); said != want { + t.Errorf("write_file says it wrote %d lines and read_file finds %d in %q", said, want, tc.content) + } + }) + } +} + // lineSpans is the one splitter the file tools count with, so its own rules are // worth stating directly: a terminated last line adds no empty line after it, // and 1-based numbering runs to exactly the number of lines the file has. From c12204bc1cadba9cfef0fe7815e6a3ddc4759790 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:35:18 +0700 Subject: [PATCH 16/20] Note a mixed replacement, not just an all-LF one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The advisory asked whether new_string contained a CRLF anywhere, so a replacement that mixed the flavors — "one\r\ntwo\nthree" into a CRLF file — put a bare LF on disk with nothing said, while docs/tools.md promised the note. The question worth asking is whether any break reached the file as a bare LF. Co-authored-by: Cursor --- internal/tools/edit_eol_advisory_test.go | 24 ++++++++++++++++++++++++ internal/tools/file.go | 10 +++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/internal/tools/edit_eol_advisory_test.go b/internal/tools/edit_eol_advisory_test.go index bcb06e6..8269405 100644 --- a/internal/tools/edit_eol_advisory_test.go +++ b/internal/tools/edit_eol_advisory_test.go @@ -30,6 +30,30 @@ func TestEditNotesLFReplacementLandingInACRLFFile(t *testing.T) { } } +// A replacement that mixes the two flavors leaves the file exactly as +// inconsistent as an all-LF one, and hides it better: the model wrote most of +// its breaks the way the file has them, so the one it did not is the easiest +// to miss. Asking whether new_string contains a CRLF anywhere answers a +// different question and skips the note here. +func TestEditNotesAReplacementThatMixesItsOwnLineEndings(t *testing.T) { + said, isError, after := editOnDisk(t, "win.txt", "alpha\r\nbeta\r\ngamma\r\n", map[string]any{ + "path": "win.txt", + "old_string": "beta\r\ngamma", + "new_string": "one\r\ntwo\nthree", + }) + if isError { + t.Fatalf("a byte-exact anchor was refused: %s", said) + } + if want := "alpha\r\none\r\ntwo\nthree\r\n"; after != want { + t.Fatalf("new_string was not written verbatim\nwant %q\ngot %q", want, after) + } + for _, want := range []string{"CRLF line endings", "LF line breaks"} { + if !strings.Contains(said, want) { + t.Errorf("a bare LF landed in a CRLF file with no mention of %q: %s", want, said) + } + } +} + // The note describes one situation, so it must appear in exactly that one. On // every other path it is either false or noise, and a note the caller learns to // ignore is worse than no note at all. diff --git a/internal/tools/file.go b/internal/tools/file.go index ccc6d96..2f408f0 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -444,7 +444,7 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { // about what was written. newString is the string that actually reached // disk, so the LF-to-CRLF recovery — which already translated it — cannot // trip this. - if fileEOL(content) == "\r\n" && strings.Contains(newString, "\n") && !strings.Contains(newString, "\r\n") { + if fileEOL(content) == "\r\n" && hasBareLF(newString) { msg += " Note: the file uses CRLF line endings and new_string used LF, so the replaced region now has LF line breaks. It was written exactly as given; send new_string with \\r\\n breaks if the file must stay consistent." } return Result{ @@ -470,6 +470,14 @@ func fileEOL(s string) string { return "\n" } +// hasBareLF reports whether s breaks a line with an LF that is not half of a +// CRLF pair. Asking instead whether s contains a CRLF anywhere answers a +// different question: a replacement that mixes the two leaves the file just as +// inconsistent as an all-LF one, and its single bare LF is the harder to see. +func hasBareLF(s string) bool { + return strings.Count(s, "\n") > strings.Count(s, "\r\n") +} + // eolOf reports the single newline flavor used in s, or "" when s has no // newlines or mixes flavors. func eolOf(s string) string { From fe4331b86425de9cdc1a399c119566cb0968581a Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:35:34 +0700 Subject: [PATCH 17/20] Stop skipping read_file tokens in the near-miss hint The exclusion dates from the NUMBER| format, where a token naming the tool meant the anchor had been copied out of read_file's own decoration. Nothing decorates a line now, so all it does is withhold the hint from anyone editing code that has a read_file identifier in it. Co-authored-by: Cursor --- internal/tools/file.go | 2 +- internal/tools/file_edit_regression_test.go | 30 +++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/internal/tools/file.go b/internal/tools/file.go index 2f408f0..ac0636b 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -674,7 +674,7 @@ func lineOfOffset(spans []lineSpan, at int) int { func nearMissHint(content, oldString string) string { spans := lineSpans(content) for _, token := range identifierTokens(oldString) { - if len(token) < 8 || strings.Contains(strings.ToLower(token), "read_file") { + if len(token) < 8 { continue } var hits []string diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index 180d662..453e595 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -136,6 +136,36 @@ func TestEditFileNotFoundShowsNearMiss(t *testing.T) { } } +// The hint used to skip any token containing "read_file", from the era when a +// stale anchor could be a line of read_file's own NUMBER| output and the token +// meant "this came from the tool, not the file". There is no such output any +// more, so the exclusion only fires on what it was never about: source that +// has a read_file identifier in it, which here is most of the file tools' own +// code and their callers. +func TestEditFileNearMissHintDoesNotSkipReadFileIdentifiers(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "dispatch.go") + content := "func dispatch(name string) {\n if name == \"read_file\" && verbose {\n log(name)\n }\n}\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{ + "path": "dispatch.go", + "old_string": " if name == \"read_file\" && verbos {", + "new_string": " if name == \"read_file\" {", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError { + t.Fatalf("an anchor that is not in the file was accepted: %s", result.Content) + } + if !strings.Contains(result.Content, "Near-miss") { + t.Errorf("no near-miss hint for an anchor whose only long token is read_file: %s", result.Content) + } + if !strings.Contains(result.Content, "line 2:") { + t.Errorf("hint does not name the line that shares the token: %s", result.Content) + } +} + // The row in the file says "85 (early stop @55)" and the anchor abbreviates it // to "85 (ES@55)", so the anchor is not in the file. This is the case the // adjacent-insertion recovery was built for, and the case that shows why it From e66eb9ad4726bd2052551da515b23752e6dbf809 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:37:48 +0700 Subject: [PATCH 18/20] Make three tests able to fail The tab-diagnostic regression test asserted "tab" against an input the tab branch never fires on: the generic advice contains the word too, so deleting the whole branch left the test green. It now asserts the diagnostic's own sentence against an anchor detabbed at eight, and the file is checked unchanged. The note-shape test drew its verdict from a variable that starts at the permissive answer, so a case that fataled before contributing evidence skipped the check on the prompt entirely. Cases that reached the end are now counted, and an incomplete run says the claim cannot be judged. readFileTotalLines accepted the removed "lines" key as a fallback, so the key this branch replaced could come back with the test that looks like it owns the question still passing. Co-authored-by: Cursor --- internal/agent/prompt_file_notes_test.go | 16 ++++++++++--- internal/tools/file_edit_regression_test.go | 24 +++++++++++++++---- internal/tools/line_numbering_test.go | 26 +++++++++++---------- 3 files changed, 46 insertions(+), 20 deletions(-) diff --git a/internal/agent/prompt_file_notes_test.go b/internal/agent/prompt_file_notes_test.go index b65203f..4813ba2 100644 --- a/internal/agent/prompt_file_notes_test.go +++ b/internal/agent/prompt_file_notes_test.go @@ -140,8 +140,13 @@ func TestPromptExemptsTheToolsOwnTrailingNotesFromTheContentClaim(t *testing.T) t.Fatal("read_file is not registered") } - blankLineAlways := true - for _, tc := range []struct{ name, file, content, args, inTheFile, wantTail string }{ + // blankLineAlways is the verdict, and it starts as the answer that lets the + // prompt promise most. A case that does not reach the end contributes no + // evidence to it, so the count of cases that did is checked before the + // verdict is used: otherwise a read whose shape changed takes the claim + // check down with it and the prompt goes unexamined. + blankLineAlways, judged := true, 0 + cases := []struct{ name, file, content, args, inTheFile, wantTail string }{ { "clipped by line range", "notes.txt", "one\ntwo\nthree\nfour\nfive\n", @@ -159,7 +164,8 @@ func TestPromptExemptsTheToolsOwnTrailingNotesFromTheContentClaim(t *testing.T) oversized[:byteCap], "x\n… file truncated at 400 KB\n", }, - } { + } + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { workspace := t.TempDir() if err := os.WriteFile(filepath.Join(workspace, tc.file), []byte(tc.content), 0o644); err != nil { @@ -204,8 +210,12 @@ func TestPromptExemptsTheToolsOwnTrailingNotesFromTheContentClaim(t *testing.T) } break } + judged++ }) } + if judged != len(cases) { + t.Fatalf("%d of %d reads did not get as far as the note they append, so what read_file puts in front of one is unknown and the prompt's promise about it cannot be judged", len(cases)-judged, len(cases)) + } claim := claimAround(t, filePrompt(t), "that blank line") if !strings.Contains(claim, noteMarker) { diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index 453e595..5e1be6d 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -87,26 +87,40 @@ func TestEditFileMatchesCRLFWhenCopiedFromRead(t *testing.T) { } // When the match still fails, the error must say what went wrong in a way the -// model can act on (tabs vs spaces is the common indentation trap). +// model can act on (tabs vs spaces is the common indentation trap). The anchor +// here is the file detabbed at eight, which is a width the diagnostic reaches +// only after trying two and four, and it spans three lines: the message has to +// come from the tab branch rather than from the generic advice, which also +// contains the word "tab". func TestEditFileDiagnosesTabVsSpaceMismatch(t *testing.T) { workspace := t.TempDir() path := filepath.Join(workspace, "tabs.c") - original := "\t\tif (x) {\n\t\t\tdo_work();\n\t\t}\n" + original := "\tif (x) {\n\t\tdo_work();\n\t}\n" if err := os.WriteFile(path, []byte(original), 0o644); err != nil { t.Fatal(err) } editArgs, _ := json.Marshal(map[string]any{ "path": "tabs.c", - "old_string": " if (x) {\n do_work();\n }", - "new_string": " if (x) {\n do_work2();\n }", + "old_string": " if (x) {\n do_work();\n }", + "new_string": " if (x) {\n do_work2();\n }", }) edited := (editFileTool{}).Execute(context.Background(), Input{Args: editArgs, Workspace: workspace}) if !edited.IsError { t.Fatal("expected failure for tab/space mismatch") } - if !strings.Contains(edited.Content, "tab") { + if !strings.Contains(edited.Content, "indents with TAB characters") { t.Fatalf("error should diagnose tabs vs spaces, got: %s", edited.Content) } + if !strings.Contains(edited.Content, "tab width ~8") { + t.Errorf("error does not name the width the anchor was indented at, got: %s", edited.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != original { + t.Fatalf("file changed although the edit failed: %q", got) + } } func TestEditFileAmbiguousListsCurrentMatchLines(t *testing.T) { diff --git a/internal/tools/line_numbering_test.go b/internal/tools/line_numbering_test.go index 674c346..0737936 100644 --- a/internal/tools/line_numbering_test.go +++ b/internal/tools/line_numbering_test.go @@ -58,7 +58,10 @@ func TestLineNumbersAgreeAcrossTools(t *testing.T) { // readFileTotalLines returns the total read_file reports for a whole-file read. // The total comes from Meta so this stays a test about counting rather than -// about how the content happens to be rendered. +// about how the content happens to be rendered. It reads one key and no +// other: accepting the removed "lines" key as a fallback let the key this +// branch replaced come back without the test that looks like it owns the +// question noticing. func readFileTotalLines(t *testing.T, workspace, name string) int { t.Helper() args, err := json.Marshal(map[string]any{"path": name}) @@ -69,18 +72,17 @@ func readFileTotalLines(t *testing.T, workspace, name string) int { if res.IsError { t.Fatalf("read_file failed: %s", res.Content) } - for _, key := range []string{"total_lines", "lines"} { - if v, ok := res.Meta[key]; ok { - switch n := v.(type) { - case int: - return n - case float64: - return int(n) - } - t.Fatalf("read_file Meta[%q] = %v (%T), want a number", key, v, v) - } + v, ok := res.Meta["total_lines"] + if !ok { + t.Fatalf("read_file reported no total_lines in Meta: %v", res.Meta) + } + switch n := v.(type) { + case int: + return n + case float64: + return int(n) } - t.Fatalf("read_file reported no line total in Meta: %v", res.Meta) + t.Fatalf("read_file Meta[\"total_lines\"] = %v (%T), want a number", v, v) return 0 } From 14e88b139da89f9bc982d770c7a7ced458bd5541 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:39:37 +0700 Subject: [PATCH 19/20] Check the read_file bullet against the tool, not a word list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard on the removed prefix instruction was a list of the phrases it used, and the same order rewritten around a colon passes all of them — "a line starting with a digit and a colon is read_file's own numbering" was appended to the bullet and every prompt test stayed green. grep prints "%6d:\t" from the package next door, so that shape is a real line number somewhere in this repository, just not in read_file's output. The list is replaced by the tool: a file whose lines open with grep's format and with a pipe-separated row is read, the bytes that come back are the file's, a region copied out of them is an anchor edit_file accepts, and following a stripping rule on the same region rebuilds the row around the prefix it dropped and reports success. Co-authored-by: Cursor --- internal/agent/prompt_file_notes_test.go | 131 ++++++++++++++++++++--- 1 file changed, 116 insertions(+), 15 deletions(-) diff --git a/internal/agent/prompt_file_notes_test.go b/internal/agent/prompt_file_notes_test.go index 4813ba2..b7ef680 100644 --- a/internal/agent/prompt_file_notes_test.go +++ b/internal/agent/prompt_file_notes_test.go @@ -2,8 +2,10 @@ package agent import ( "context" + "encoding/json" "os" "path/filepath" + "regexp" "strings" "testing" @@ -33,23 +35,122 @@ func filePrompt(t *testing.T) string { // The prompt is the only description of the tools the model ever sees, so an // instruction that no longer matches them is not stale documentation — it is a -// standing order to corrupt data. read_file adds no prefix to a line, and -// new_string is authored rather than copied, so telling the model to keep only -// what follows a "|" deletes real content from files whose lines start with one. -func TestPromptFileNotesDoNotDescribeALineNumberPrefix(t *testing.T) { - prompt := filePrompt(t) - for _, gone := range []string{ - "NUMBER|", - "NUMBER|CONTENT", - "metadata only", - "content after `|`", - "never the line number", - "Line endings are matched automatically", - } { - if strings.Contains(prompt, gone) { - t.Errorf("prompt still describes the removed line-prefix format: %q", gone) +// standing order to corrupt data. The instruction that was there told the model +// to keep only what follows a "|", and a list of the phrases it used cannot +// guard against it: the same order rewritten around a colon reads as new advice +// and passes every one of them. grep prints "%6d:\t" from the package next +// door, so a line of spaces, digits and a colon genuinely is a line number +// somewhere in this repository — just not in read_file's output. +// +// What does not depend on the wording is the tool. This drives it on content +// shaped like the numbering such an instruction would describe, and checks the +// two things the bullet tells the model to do with what comes back: copy a +// region straight into old_string, which has to work, and strip nothing from +// it, because stripping is what breaks the file. +func TestPromptClaimThatACopiedRegionIsAnAnchorHoldsAgainstTheRealTools(t *testing.T) { + // Line 1 is grep's own output shape, line 2 the pipe-separated row the + // removed format collided with, and line 4 is tab-indented. + const original = " 288:\tfmt.Fprintf(&b, \"%6d:\\t%s\\n\", lineNo, line)\n" + + "12|alice|admin\n" + + "| Date | Event |\n" + + "\tif enabled:\n" + // One row promoted, and nothing else in the file touched. + const intended = " 288:\tfmt.Fprintf(&b, \"%6d:\\t%s\\n\", lineNo, line)\n" + + "12|alice|owner\n" + + "| Date | Event |\n" + + "\tif enabled:\n" + + read, ok := tools.Default().Get("read_file") + if !ok { + t.Fatal("read_file is not registered") + } + edit, ok := tools.Default().Get("edit_file") + if !ok { + t.Fatal("edit_file is not registered") + } + // A workspace holding the file, and the region the model would copy: cut + // out of what read_file returned rather than written out again, so it is + // the tool's output that is under test and not the fixture. + setUp := func(t *testing.T) (workspace, copied string) { + t.Helper() + workspace = t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "rows.txt"), []byte(original), 0o644); err != nil { + t.Fatal(err) + } + res := read.Execute(context.Background(), tools.Input{ + Workspace: workspace, + Args: []byte(`{"path":"rows.txt"}`), + }) + if res.IsError { + t.Fatalf("read_file: %s", res.Content) + } + _, body, ok := strings.Cut(res.Content, "\n\n") + if !ok { + t.Fatalf("read_file output has no header line followed by a blank line: %q", res.Content) } + if body != original { + t.Fatalf("prompt promises the file's exact bytes, read_file returned %q", body) + } + lines := strings.Split(body, "\n") + if len(lines) < 2 { + t.Fatalf("read_file returned no second line to copy: %q", body) + } + return workspace, lines[1] + } + + editRow := func(t *testing.T, workspace, oldString, newString string) (said string, after string) { + t.Helper() + args, err := json.Marshal(map[string]any{ + "path": "rows.txt", "old_string": oldString, "new_string": newString, + }) + if err != nil { + t.Fatal(err) + } + res := edit.Execute(context.Background(), tools.Input{Workspace: workspace, Args: args}) + written, err := os.ReadFile(filepath.Join(workspace, "rows.txt")) + if err != nil { + t.Fatal(err) + } + return res.Content, string(written) } + + t.Run("a region copied straight out of the read is an anchor", func(t *testing.T) { + workspace, copied := setUp(t) + if copied != "12|alice|admin" { + t.Fatalf("the second line came back as %q; the fixture's own bytes are \"12|alice|admin\"", copied) + } + said, after := editRow(t, workspace, copied, "12|alice|owner") + if after != intended { + t.Errorf("copying a region into old_string did not produce what was asked for\nsaid: %s\nwant %q\ngot %q", said, intended, after) + } + }) + + // The same intent, through the instruction the bullet must never carry + // again: the leading digits and separator are read as the tool's numbering + // and dropped. Nothing refuses it — the remainder of a row is really in the + // file — so the row is rewritten around a prefix that was content, and the + // tool reports success. + t.Run("dropping a leading number and separator writes something else", func(t *testing.T) { + workspace, copied := setUp(t) + stripped := regexp.MustCompile(`^\s*\d+[:|]\s?`).ReplaceAllString(copied, "") + if stripped == copied { + t.Fatalf("the copied line %q has no leading number to strip; this case no longer tests anything", copied) + } + said, after := editRow(t, workspace, stripped, "12|alice|owner") + switch { + case after == intended: + t.Fatalf("stripping the line's leading characters produced the file the caller wanted, so an instruction to do it would be sound: %s", said) + case after == original: + // Refused, which is a safe way for the instruction to be wrong. + default: + // Accepted: the remainder of a row is really in the file, so what + // was dropped as numbering is still there and the row now carries + // it twice. + if !strings.Contains(after, "12|12|alice|owner") { + t.Errorf("the stripped anchor wrote something other than a row rebuilt around the prefix it dropped: %q (%s)", after, said) + } + } + }) } // Dropping the wrong advice is only half the job. These four are what keeps the From 0c9e8d047880745c8c3b9a778f293615db88ee9c Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Fri, 14 Aug 2026 02:53:54 +0700 Subject: [PATCH 20/20] Put the floor the header states into the metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs and the comment beside the code said last_line carried the floor on a clipped read. It does when the line limit reaches as far as the byte clip, and not otherwise: a default read of a 600 KB file states "of ≥10240" in the header and returns Meta with last_line 2000 and the floor nowhere in it. The metadata is offered so a caller need not parse the header, so the number is now carried as total_lines_at_least, named for what it is, and both sentences say what is true. The mixed-endings note said "new_string used LF", which is true of a replacement that used both and describes a different one. It now says new_string broke at least one line with a bare LF. Co-authored-by: Cursor --- docs/tools.md | 6 ++-- internal/tools/edit_eol_advisory_test.go | 5 ++- internal/tools/file.go | 9 +++-- internal/tools/read_truncated_test.go | 45 +++++++++++++++++++----- 4 files changed, 51 insertions(+), 14 deletions(-) diff --git a/docs/tools.md b/docs/tools.md index a49150f..3981b78 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -39,8 +39,10 @@ was terminated in the file. A read the 400 KB cap stopped has not counted the lines behind it, and says so rather than reporting what it read as the whole: the header's total becomes `≥N`, the continuation note says `at least N more lines`, and the metadata -drops `total_lines` for `truncated: true` — `last_line` is then the floor the -header states. `grep` counts the whole file (up to 8 MB), so it is the tool +drops `total_lines` for `truncated: true` plus `total_lines_at_least`, which is +the `N` the header states. `last_line` is the last line returned, as on any +other read, and is smaller than `N` whenever the line limit stopped the read +before the byte cap did. `grep` counts the whole file (up to 8 MB), so it is the tool that can still find something past the cap; asking `read_file` to page to a line beyond it is refused, naming the cap. diff --git a/internal/tools/edit_eol_advisory_test.go b/internal/tools/edit_eol_advisory_test.go index 8269405..7bab1e6 100644 --- a/internal/tools/edit_eol_advisory_test.go +++ b/internal/tools/edit_eol_advisory_test.go @@ -47,7 +47,10 @@ func TestEditNotesAReplacementThatMixesItsOwnLineEndings(t *testing.T) { if want := "alpha\r\none\r\ntwo\nthree\r\n"; after != want { t.Fatalf("new_string was not written verbatim\nwant %q\ngot %q", want, after) } - for _, want := range []string{"CRLF line endings", "LF line breaks"} { + // "new_string used LF" is true of this replacement and describes a + // different one: most of its breaks are the file's own, and the note is + // about the one that is not. + for _, want := range []string{"CRLF line endings", "LF line breaks", "at least one line with a bare LF"} { if !strings.Contains(said, want) { t.Errorf("a bare LF landed in a CRLF file with no mention of %q: %s", want, said) } diff --git a/internal/tools/file.go b/internal/tools/file.go index ac0636b..2fe68f6 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -272,9 +272,12 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { "last_line": end, } if truncatedBytes { - // last_line is the floor the header states; a caller that wants a - // count rather than a floor has to be able to tell them apart. + // The number the header states after "≥", carried under its own key so + // a caller need not parse the header for it. last_line cannot stand in: + // the line limit usually stops the read long before the byte cap does, + // and on a default read of a 600 KB file the two are 2000 and 10240. meta["truncated"] = true + meta["total_lines_at_least"] = len(lines) } else { meta["total_lines"] = len(lines) } @@ -445,7 +448,7 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { // disk, so the LF-to-CRLF recovery — which already translated it — cannot // trip this. if fileEOL(content) == "\r\n" && hasBareLF(newString) { - msg += " Note: the file uses CRLF line endings and new_string used LF, so the replaced region now has LF line breaks. It was written exactly as given; send new_string with \\r\\n breaks if the file must stay consistent." + msg += " Note: the file uses CRLF line endings and new_string broke at least one line with a bare LF, so the replaced region now has LF line breaks. It was written exactly as given; send new_string with \\r\\n breaks if the file must stay consistent." } return Result{ Content: msg, diff --git a/internal/tools/read_truncated_test.go b/internal/tools/read_truncated_test.go index fee681f..398bcfa 100644 --- a/internal/tools/read_truncated_test.go +++ b/internal/tools/read_truncated_test.go @@ -33,19 +33,23 @@ func bigLog(t *testing.T, lines int, needleAt int, needle string) string { var readHeaderRange = regexp.MustCompile(`^(.*) — lines (\d+)-(\d+) of (≥?)(\d+)\n`) -// headerTotal returns the total read_file's header states and whether it is -// stated as a lower bound rather than as a count. -func headerTotal(t *testing.T, out string) (total int, isLowerBound bool) { +// headerRange returns the three numbers read_file's header states and whether +// the total is stated as a lower bound rather than as a count. +func headerRange(t *testing.T, out string) (first, last, total int, isLowerBound bool) { t.Helper() m := readHeaderRange.FindStringSubmatch(out) if m == nil { t.Fatalf("read_file output does not open with a line-range header: %q", firstBytes(out, 120)) } - n, err := strconv.Atoi(m[5]) - if err != nil { - t.Fatalf("header states a total that will not parse: %q", m[0]) + var n [3]int + for i, field := range []string{m[2], m[3], m[5]} { + v, err := strconv.Atoi(field) + if err != nil { + t.Fatalf("header states a number that will not parse: %q", m[0]) + } + n[i] = v } - return n, m[4] == "≥" + return n[0], n[1], n[2], m[4] == "≥" } // The cap stops the read after 400 KB, so the lines behind it were never @@ -70,7 +74,7 @@ func TestReadFileStatesNoTotalItHasNotCounted(t *testing.T) { t.Run("the header states no total the read did not count", func(t *testing.T) { res := readFileResult(t, workspace, map[string]any{"path": "big.log", "limit": lines}) - total, isLowerBound := headerTotal(t, res.Content) + _, _, total, isLowerBound := headerRange(t, res.Content) switch { case isLowerBound && total > lines: t.Errorf("header claims at least %d lines, and the file has %d", total, lines) @@ -79,6 +83,31 @@ func TestReadFileStatesNoTotalItHasNotCounted(t *testing.T) { } }) + // The metadata is offered so a caller does not have to parse the header + // (docs/tools.md), so every number the header states has to be in it. The + // default limit is where that is easiest to get wrong: it stops the read + // long before the byte cap does, so the last line returned and the lines + // the cap held are different numbers, and only one of them was in Meta. + t.Run("Meta carries every number the default read's header states", func(t *testing.T) { + res := readFileResult(t, workspace, map[string]any{"path": "big.log"}) + first, last, floor, isLowerBound := headerRange(t, res.Content) + if !isLowerBound || last >= floor { + t.Fatalf("this case needs a read the line limit clips before the byte cap does; the header says lines %d-%d of %d", first, last, floor) + } + for key, want := range map[string]int{ + "first_line": first, "last_line": last, "total_lines_at_least": floor, + } { + got, ok := res.Meta[key] + if !ok { + t.Errorf("the header states %d and Meta has no %q, so a caller wanting that number has to parse the header after all: %v", want, key, res.Meta) + continue + } + if got != want { + t.Errorf("Meta[%q] = %v, and the header says %d", key, got, want) + } + } + }) + t.Run("Meta states no total the read did not count", func(t *testing.T) { res := readFileResult(t, workspace, map[string]any{"path": "big.log", "limit": lines}) if got, ok := res.Meta["total_lines"]; ok && got != lines {