Restore verbatim read_file and exact edit_file - #32
Open
nvdorman wants to merge 21 commits into
Open
Conversation
edit_file grew 500 lines of similarity scoring, edit distance and prefix stripping to recover from anchors that do not match. Driven through the real tool, three of those recovery paths write to disk and report success: a space-indented anchor against a tab-indented file produces Python that will not compile, prefix stripping deletes real data out of new_string, and an anchor matching zero times rewrites every row under replace_all. All of it descends from one line in a commit about CRLF, which changed read_file's separator from a tab to a pipe. Both collide with real file content. This spec removes the separator instead. Co-authored-by: Cursor <cursoragent@cursor.com>
Four tasks: give the file tools one idea of what a line is, remove the line-number prefix from read_file's output, delete the machinery that existed only to recover from that prefix, and make the prompt describe the result. The four acceptance tests are committed red on purpose. Each was observed driving the real tool into writing a file it should have refused. Co-authored-by: Cursor <cursoragent@cursor.com>
read_file split on \n and so reported a phantom trailing line for any file ending in a newline, grep counted with a bufio.Scanner that did not, and on a file terminated with lone \r the two disagreed entirely. A line number one tool reported meant a different line in the next. lineSpans is now the only place a line boundary is decided. read_file's numbering and total, grep's match and context lines, and edit_file's occurrence and near-miss lines all come from it. grep reads the file whole, because telling a lone CR terminator from a CR byte inside a line is a property of the file rather than of any one line. The size gate moves to where the read happens, so a file named directly on the command is bounded by it too. Co-authored-by: Cursor <cursoragent@cursor.com>
The gate stated a file's size and then read without a limit, so anything whose stat understates what it yields walked straight through: character devices, most of /proc and /sys, and a file being appended to during the read. /dev/zero reports zero bytes and never ends, and a symlink to it in the workspace is an ordinary entry to filepath.WalkDir, so a plain recursive grep never returned. The bufio.Scanner this replaced stopped after 1 MB, so it was a regression, and it was the exact hazard cited to justify gating a directly named file in the first place. The LimitReader is the gate now and applies whether or not the fstat succeeded. Stat stays as an early-out so a 200 MB file is not read 8 MB deep before being rejected, and taken on the open descriptor it follows a symlink to its target, which the walk's Lstat did not. Also pins two behaviours that were carried without cover: an empty file stays readable, which is the other half of the offset guard, and a last line ending in a lone CR keeps that CR, which bufio.ScanLines dropped. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Two claims added in the previous commit were false, which is the defect class this branch exists to remove: - "Everything below that blank line is file content" ignored the trailing "… N more lines" and "… file truncated" notes read_file appends, on exactly the large files the same bullet tells the model to page through. - "new_string is written byte for byte" is not true on the LF-to-CRLF recovery, which translates new_string because old_string had to be. docs/tools.md asserted it and then contradicted itself four lines later. The prompt tests now establish each fact from the real tool before asserting the prompt states it, and judge the claim one sentence at a time, so a true clause can no longer keep a false neighbour green. Co-authored-by: Cursor <cursoragent@cursor.com>
The clause added with the last fix said the trailing notes are preceded by a blank line. read_file writes a note as "\n…" onto whatever body ends with, so that blank line exists only when the body already ended in its own newline. On the byte-cap path it does not: a file over 400 KB whose first 400 KB holds one long line gets "…xxx\n… file truncated at 400 KB\n", and the newline in front of the note is the tool's, not the file's. Telling the model otherwise says the last content line is terminated when it is not, which is the retry loop the third bullet exists to prevent. The marker is the rule that holds on both branches, so it is the only one offered. The read test now drives the byte cap as well as the line clip, asserts the real tail of each, and forbids the prompt from describing what precedes a note unless every branch puts it there. Co-authored-by: Cursor <cursoragent@cursor.com>
The 400 KB cap ran after os.ReadFile had already loaded the file, so it bounded what reached the model and nothing bounded what reached the process. An ordinary large workspace file was held whole to be trimmed, and a file that understates its size — a character device, most of /proc, a file being appended to, /dev/zero — ended the process in a fatal out-of-memory. grep's gate was moved onto the read for exactly this reason; read_file's now is too. Co-authored-by: Cursor <cursoragent@cursor.com>
Past 400 KB read_file trimmed the bytes and then counted the lines it had left, so it reported the head's line count as the file's total. grep counts the whole file, so the two disagreed above the cap: grep found NEEDLE at line 15001 of a 600 KB log and read_file called the same file 10240 lines long and refused that offset as past its end — the retry loop this branch exists to remove, moved one tool along. Learning the real total means reading past the cap, which is what the cap is for, so the tool states a floor instead of a count: the header reads "of ≥N", the continuation note "at least N more lines", and Meta drops total_lines for truncated. An offset beyond the cap is still refused, now naming the cap and pointing at grep, which reads the whole file. Co-authored-by: Cursor <cursoragent@cursor.com>
strings.Count(content, "") is the rune count plus one, so an empty old_string was "found" between every pair of characters: replace_all interleaved new_string with the file a character at a time and reported eighteen replacements on a two-line config, and a single replacement was refused for appearing as many times as the file has characters. It was the last input for which an exact match wrote anyway. Co-authored-by: Cursor <cursoragent@cursor.com>
write_file counted "\n" and added one, so "a\nb\n" was reported as three lines where read_file, grep and edit_file all say two, and a CR- terminated file as one where they say two. It is the same file a moment apart, so both counts land in the same conversation. Co-authored-by: Cursor <cursoragent@cursor.com>
The advisory asked whether new_string contained a CRLF anywhere, so a replacement that mixed the flavors — "one\r\ntwo\nthree" into a CRLF file — put a bare LF on disk with nothing said, while docs/tools.md promised the note. The question worth asking is whether any break reached the file as a bare LF. Co-authored-by: Cursor <cursoragent@cursor.com>
The exclusion dates from the NUMBER| format, where a token naming the tool meant the anchor had been copied out of read_file's own decoration. Nothing decorates a line now, so all it does is withhold the hint from anyone editing code that has a read_file identifier in it. Co-authored-by: Cursor <cursoragent@cursor.com>
The tab-diagnostic regression test asserted "tab" against an input the tab branch never fires on: the generic advice contains the word too, so deleting the whole branch left the test green. It now asserts the diagnostic's own sentence against an anchor detabbed at eight, and the file is checked unchanged. The note-shape test drew its verdict from a variable that starts at the permissive answer, so a case that fataled before contributing evidence skipped the check on the prompt entirely. Cases that reached the end are now counted, and an incomplete run says the claim cannot be judged. readFileTotalLines accepted the removed "lines" key as a fallback, so the key this branch replaced could come back with the test that looks like it owns the question still passing. Co-authored-by: Cursor <cursoragent@cursor.com>
The guard on the removed prefix instruction was a list of the phrases it used, and the same order rewritten around a colon passes all of them — "a line starting with a digit and a colon is read_file's own numbering" was appended to the bullet and every prompt test stayed green. grep prints "%6d:\t" from the package next door, so that shape is a real line number somewhere in this repository, just not in read_file's output. The list is replaced by the tool: a file whose lines open with grep's format and with a pipe-separated row is read, the bytes that come back are the file's, a region copied out of them is an anchor edit_file accepts, and following a stripping rule on the same region rebuilds the row around the prefix it dropped and reports success. Co-authored-by: Cursor <cursoragent@cursor.com>
The docs and the comment beside the code said last_line carried the floor on a clipped read. It does when the line limit reaches as far as the byte clip, and not otherwise: a default read of a 600 KB file states "of ≥10240" in the header and returns Meta with last_line 2000 and the floor nowhere in it. The metadata is offered so a caller need not parse the header, so the number is now carried as total_lines_at_least, named for what it is, and both sentences say what is true. The mixed-endings note said "new_string used LF", which is true of a replacement that used both and describes a different one. It now says new_string broke at least one line with a bare LF. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
read_filenow returns the file's bytes verbatim under a line-range header. TheNUMBER|prefix is gone, so there is nothing for the model to strip and no separator that can collide with file content.edit_filewrites only whenold_stringis present exactly, and writes exactlynew_string. The guessing stack (adjacent insertion, token similarity, edit distance, prefix stripping) is removed.grep/read_file/edit_fileshare one line splitter. Reads are bounded; a clipped read no longer states a total it did not count.This is the reversal of the post-v0.2.0 recovery machinery that grew out of the
%6d\t→%d|prefix change. That machinery could write when the anchor was absent (tab vs space Python files,TabErrorreported as success) and could drop bytes fromnew_stringwhile claiming a clean edit.Sync
Rebased onto current
mainvia merge (3b25142): includes PR #30, the Cursor Cloud Agents removal, and the live/compactwork.Test plan
go test ./internal/tools ./internal/agent ./internal/server -count=1after mergingmainedit_fileon a tab-indented file with a space-indentedold_string— must refuse, not writeread_filea small file — body matches disk; header only, no per-line prefixread_filea file above 400 KB — header uses≥N, metadata hastruncated+total_lines_at_least, no false exact totalgrephit past the 400 KB cap —read_filerefuses and names the capold_string/replace_all— refused, file unchanged