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/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. diff --git a/docs/tools.md b/docs/tools.md index 2663876..de32372 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,44 @@ 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. + +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 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. + +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` 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. + +`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. +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..29ea196 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -103,11 +103,16 @@ 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. 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. 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") } 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..b7ef680 --- /dev/null +++ b/internal/agent/prompt_file_notes_test.go @@ -0,0 +1,417 @@ +package agent + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "regexp" + "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. 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 +// 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) + } +} + +// 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. +// +// 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) + 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 a note appears. Taken literally, an +// unqualified claim invites "… 2 more lines" into an anchor. +// +// 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) + + read, ok := tools.Default().Get("read_file") + if !ok { + t.Fatal("read_file is not registered") + } + + // 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", + `{"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", + }, + } + 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 { + 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 + } + 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) { + t.Errorf("prompt claims file content below the header without exempting the %q notes read_file just appended: %q", noteMarker, claim) + } + // 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) + } + } + } +} + +// 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 +// 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) { + 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..7bab1e6 --- /dev/null +++ b/internal/tools/edit_eol_advisory_test.go @@ -0,0 +1,107 @@ +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) + } + } +} + +// 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) + } + // "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) + } + } +} + +// 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/edit_exact_test.go b/internal/tools/edit_exact_test.go new file mode 100644 index 0000000..dfaa1bc --- /dev/null +++ b/internal/tools/edit_exact_test.go @@ -0,0 +1,170 @@ +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) + } +} + +// 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 +// 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) + } +} + +// 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 +// 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 d3abaab..2fe68f6 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -6,10 +6,10 @@ import ( "encoding/hex" "errors" "fmt" + "io" "os" "path/filepath" "sort" - "strconv" "strings" "unicode/utf8" ) @@ -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{ @@ -172,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) } @@ -190,16 +200,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 +211,15 @@ 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) { + 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 @@ -217,17 +227,61 @@ 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, lines[i]) + // 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 + } + + // 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 %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": relTo(in.Workspace, path), "lines": len(lines)}} + meta := map[string]any{ + "path": rel, + "first_line": first, + "last_line": end, + } + if truncatedBytes { + // 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) + } + return Result{Content: b.String(), Meta: meta} } // ---- write_file ------------------------------------------------------------- @@ -298,12 +352,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)}, } } @@ -314,13 +366,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") @@ -336,6 +388,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") } @@ -351,25 +411,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 @@ -390,6 +440,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" && hasBareLF(newString) { + 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, Meta: map[string]any{"path": rel, "replacements": replaced}, @@ -413,6 +473,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 { @@ -440,117 +508,38 @@ 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 +// 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 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, "" + return oldIn, newIn, 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 - } - - // 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. 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 := lfToCRLF(oldIn) + if c := strings.Count(content, crlf); c > 0 { + return crlf, lfToCRLF(newIn), c, + "translated old_string line endings from LF to CRLF to match the file" } } @@ -561,6 +550,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 @@ -599,263 +594,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) @@ -877,6 +644,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 +652,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") { + if len(token) < 8 { 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/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 b902e07..5e1be6d 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) @@ -89,57 +86,41 @@ 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). +// 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) { @@ -169,7 +150,43 @@ func TestEditFileNotFoundShowsNearMiss(t *testing.T) { } } -func TestEditFileRecoversUniqueNearInsertionWithoutChangingExistingLine(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 +// 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" @@ -180,52 +197,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" @@ -238,14 +251,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) } } @@ -302,21 +315,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) { @@ -337,12 +335,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,12 +350,19 @@ 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) } } -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" @@ -368,47 +375,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/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) + } +} diff --git a/internal/tools/grep_skip_test.go b/internal/tools/grep_skip_test.go index ad3bddf..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 @@ -29,6 +30,61 @@ 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) + } + }) + + // 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 new file mode 100644 index 0000000..0737936 --- /dev/null +++ b/internal/tools/line_numbering_test.go @@ -0,0 +1,293 @@ +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. 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}) + 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) + } + 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 Meta[\"total_lines\"] = %v (%T), want a number", v, v) + 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 +} + +// 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. +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}, + {"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}, + } { + 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) + } + } +} + +// 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 +// 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/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] + "…" +} diff --git a/internal/tools/read_truncated_test.go b/internal/tools/read_truncated_test.go new file mode 100644 index 0000000..398bcfa --- /dev/null +++ b/internal/tools/read_truncated_test.go @@ -0,0 +1,169 @@ +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`) + +// 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)) + } + 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[0], n[1], n[2], 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 := headerRange(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) + } + }) + + // 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 { + 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}) +} diff --git a/internal/tools/read_verbatim_test.go b/internal/tools/read_verbatim_test.go new file mode 100644 index 0000000..59eca52 --- /dev/null +++ b/internal/tools/read_verbatim_test.go @@ -0,0 +1,172 @@ +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}) + + // 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, + } { + 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) + } +} + +// 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) { + 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) + } +} diff --git a/internal/tools/search.go b/internal/tools/search.go index 0660271..0c66110 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,37 @@ 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. 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(io.LimitReader(f, maxGrepFileBytes+1)) + if err != nil { + return nil + } + if len(data) > maxGrepFileBytes { + skipped++ + 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 +306,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 +332,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) } }