diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index db4a6f045..dc70b01da 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -140,7 +140,6 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any return errorResult(fileUnseenMessage(relativePath)) } - previouslySeenWhole := options.FileTracker.SeenWhole(absolutePath) updated := strings.Replace(content, oldString, newString, 1) replacedCount := 1 if replaceAll { @@ -165,15 +164,28 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any // Re-baseline to the content we just wrote so subsequent edits in this session // compare against the current on-disk state, not the pre-edit version. newInfo, _ := os.Stat(absolutePath) - options.FileTracker.Record(absolutePath, []byte(updated), newInfo) if updated == modelKnownContent { - if previouslySeenWhole { - options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(updated), trackedLineTotal(updated)) - } else { - for _, span := range editedSpans { - options.FileTracker.RecordSeenBytes(absolutePath, span.start, span.end, len(updated)) - } + // OUR edit, so we know precisely which lines moved: RecordEdit carries + // across the reads this edit did not disturb instead of dropping them. + // + // Record would drop all of them, and did — a file read in three pieces + // lost every piece to a single two-line edit, and the next six edits into + // regions that had been read were refused as unseen. See RecordEdit. + // + // This SUBSUMES the previouslySeenWhole special-case #956 added here. + // That branch re-recorded 1..total when the file had been read whole; + // RecordEdit does the same thing one level down (its seenWhole arm + // re-baselines as a single covering observation) and additionally keeps + // the partial reads the old else-branch discarded. Two copies of the + // rule would drift, and only one of them sees the pre-edit observation. + options.FileTracker.RecordEdit(absolutePath, []byte(content), []byte(updated), newInfo) + for _, span := range editedSpans { + options.FileTracker.RecordSeenBytes(absolutePath, span.start, span.end, len(updated)) } + } else { + // A formatter rewrote the file after us. We no longer know which line + // holds what was read, so the conservative drop is the right answer here. + options.FileTracker.Record(absolutePath, []byte(updated), newInfo) } suffix := "" diff --git a/internal/tools/edit_preserves_reads_test.go b/internal/tools/edit_preserves_reads_test.go new file mode 100644 index 000000000..3929b092b --- /dev/null +++ b/internal/tools/edit_preserves_reads_test.go @@ -0,0 +1,296 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// editTrackerFixture writes a numbered file and returns its real path plus a +// tracker that has already recorded the whole content as the current version. +// +// EvalSymlinks matters: on macOS t.TempDir() hands back /var/..., the tools +// resolve it to /private/var/..., and a tracker keyed on the unresolved path +// silently misses every lookup — which makes an edit fail for the very reason +// this file is about, and for entirely the wrong cause. +func editTrackerFixture(t *testing.T, lines []string) (string, *FileTracker, Tool) { + t.Helper() + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "index.go") + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")), 0o644); err != nil { + t.Fatal(err) + } + tracker := NewFileTracker() + content, _ := os.ReadFile(path) + info, _ := os.Stat(path) + tracker.Record(path, content, info) + return path, tracker, NewScopedEditFileTool(root, nil) +} + +func numberedLines(total int, special map[int]string) []string { + lines := make([]string, 0, total) + for i := 1; i <= total; i++ { + if text, ok := special[i]; ok { + lines = append(lines, text) + continue + } + lines = append(lines, "filler") + } + return lines +} + +func runTrackedEdit(t *testing.T, tool Tool, tracker *FileTracker, path, oldString, newString string) Result { + t.Helper() + return tool.(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": path, "old_string": oldString, "new_string": newString, + }, RunOptions{FileTracker: tracker}) +} + +// ONE EDIT MUST NOT ERASE EVERY OTHER READ, and this is the run that made the +// point: a 371-line file read in three pieces (40-45, 85-260, 260-371), one +// two-line edit at line 92, and then six consecutive refusals to edit regions +// that HAD been read and that the edit never touched — until the +// repeated-failure guard halted the run. The error told the model to re-read, +// which its next successful edit would have undone again. +func TestASuccessfulEditKeepsTheReadsItDidNotDisturb(t *testing.T) { + path, tracker, tool := editTrackerFixture(t, numberedLines(371, map[int]string{ + 92: "cmp := keyCmp(k, n.split)", + 200: "prio: l.prio, split: l.split,", + })) + tracker.RecordSeenRange(path, 40, 45, 371) + tracker.RecordSeenRange(path, 85, 260, 371) + tracker.RecordSeenRange(path, 260, 371, 371) + + if !tracker.SeenRange(path, 200, 200) { + t.Fatal("setup: line 200 sits inside the 85-260 read and must start out seen") + } + + first := runTrackedEdit(t, tool, tracker, path, "cmp := keyCmp(k, n.split)", "cmp := keyCmp(k, n.pivot)") + if strings.Contains(first.Output, "Error") { + t.Fatalf("the first edit failed, so the test never reaches its subject: %s", first.Output) + } + + if !tracker.SeenRange(path, 200, 200) { + t.Error("line 200 was read, the edit at line 92 did not touch it, and the file still has 371 lines — but it is no longer considered seen") + } + // The assertion that matters: the SECOND edit is what the run could never do. + second := runTrackedEdit(t, tool, tracker, path, "prio: l.prio, split: l.split,", "prio: l.prio, pivot: l.pivot,") + if strings.Contains(second.Output, "not been read exactly in this session") { + t.Fatalf("a second edit into an already-read region was refused as unseen: %s", second.Output) + } + if strings.Contains(second.Output, "Error") { + t.Fatalf("the second edit failed: %s", second.Output) + } +} + +// LINE NUMBERS MOVE WHEN AN EDIT CHANGES THE LINE COUNT, so a range after the +// edit has to be carried across SHIFTED, not left where it was. +func TestReadsAfterAnEditAreShiftedByTheLineDelta(t *testing.T) { + path, tracker, tool := editTrackerFixture(t, numberedLines(100, map[int]string{ + 10: "short", + 80: "target line", + })) + tracker.RecordSeenRange(path, 5, 20, 100) + tracker.RecordSeenRange(path, 70, 90, 100) + + // Replace one line with three: everything after line 10 moves down by two. + res := runTrackedEdit(t, tool, tracker, path, "short", "one\ntwo\nthree") + if strings.Contains(res.Output, "Error") { + t.Fatalf("setup edit failed: %s", res.Output) + } + + // "target line" was line 80 and is now line 82. It was read either way. + content, _ := os.ReadFile(path) + moved := strings.Count(strings.SplitN(string(content), "target line", 2)[0], "\n") + 1 + if moved != 82 { + t.Fatalf("fixture moved the target to line %d, expected 82", moved) + } + if !tracker.SeenRange(path, moved, moved) { + t.Errorf("the read at lines 70-90 was not shifted with the content: line %d reads as unseen", moved) + } + second := runTrackedEdit(t, tool, tracker, path, "target line", "target changed") + if strings.Contains(second.Output, "Error") { + t.Fatalf("editing a line that was read and merely moved was refused: %s", second.Output) + } +} + +// THE GUARD STILL GUARDS. Carrying reads across must not credit the model with +// content it never saw — that is the whole reason the check exists. +func TestAnUnreadRegionIsStillRefusedAfterAnEdit(t *testing.T) { + path, tracker, tool := editTrackerFixture(t, numberedLines(300, map[int]string{ + 50: "seen line", + 250: "never read line", + })) + // Only 40-60 is read. Line 250 is not. + tracker.RecordSeenRange(path, 40, 60, 300) + + if res := runTrackedEdit(t, tool, tracker, path, "seen line", "seen line edited"); strings.Contains(res.Output, "Error") { + t.Fatalf("setup edit failed: %s", res.Output) + } + res := runTrackedEdit(t, tool, tracker, path, "never read line", "sneaky") + if !strings.Contains(res.Output, "not been read exactly in this session") { + t.Fatalf("an edit into a region that was NEVER read was allowed: %s", res.Output) + } +} + +// A RANGE THE EDIT SPANS IS SPLIT, NOT DROPPED — and getting this wrong is not +// hypothetical: dropping on overlap was my first attempt, and it left the +// original defect exactly in place. A read almost always CONTAINS the line it +// is about to edit, because containing it is why it was read. +// +// So the flanks survive and only the rewritten lines stop being known. +func TestARangeTheEditSpansIsSplitAroundIt(t *testing.T) { + path, tracker, tool := editTrackerFixture(t, numberedLines(100, map[int]string{ + 20: "alpha", + 21: "beta", + 22: "gamma", + })) + tracker.RecordSeenRange(path, 15, 30, 100) + + // Replace a three-line span with one line, entirely inside the read range. + if res := runTrackedEdit(t, tool, tracker, path, "alpha\nbeta\ngamma", "merged"); strings.Contains(res.Output, "Error") { + t.Fatalf("setup edit failed: %s", res.Output) + } + + // Before the edit: still known, still at the same line numbers. + if !tracker.SeenRange(path, 15, 19) { + t.Error("lines 15-19 sit before the edit and did not move, but were forgotten") + } + // After it: still known, shifted up by the two lines the edit removed. + if !tracker.SeenRange(path, 21, 28) { + t.Error("lines 23-30 were read and merely moved to 21-28, but were forgotten") + } + // The rewritten span itself is no longer described by that read. + if tracker.SeenRange(path, 15, 30) { + t.Error("the whole original range still reads as seen, so the model is credited with content the edit replaced") + } +} + +// AN EXTERNAL CHANGE STILL INVALIDATES EVERYTHING. RecordEdit is only for edits +// we made; a file changed behind our back is exactly the case the blanket drop +// is right for, and it must keep working. +func TestAnExternalChangeStillDropsEveryRead(t *testing.T) { + path, tracker, _ := editTrackerFixture(t, numberedLines(100, map[int]string{50: "hello"})) + tracker.RecordSeenRange(path, 40, 60, 100) + if !tracker.SeenRange(path, 50, 50) { + t.Fatal("setup: line 50 should be seen") + } + + // Something outside Zero rewrites the file. + changed := strings.Repeat("different\n", 100) + if err := os.WriteFile(path, []byte(changed), 0o644); err != nil { + t.Fatal(err) + } + info, _ := os.Stat(path) + tracker.Record(path, []byte(changed), info) + + if tracker.SeenRange(path, 50, 50) { + t.Error("an external rewrite left the old read ranges in place") + } +} + +// A file read in FULL stays read in full: an edit of ours does not make that +// untrue, and re-reading a whole file after every edit is the cost this avoids. +func TestAWhollyReadFileStaysWhollyRead(t *testing.T) { + path, tracker, tool := editTrackerFixture(t, numberedLines(40, map[int]string{5: "one"})) + tracker.RecordSeenRange(path, 1, 40, 40) + if !tracker.SeenWhole(path) { + t.Fatal("setup: the file should read as wholly seen") + } + + if res := runTrackedEdit(t, tool, tracker, path, "one", "one\ntwo"); strings.Contains(res.Output, "Error") { + t.Fatalf("edit failed: %s", res.Output) + } + if !tracker.SeenWhole(path) { + t.Error("a wholly-read file stopped being wholly read after an edit") + } +} + +// The span helpers, directly: these decide what survives, so their edges are +// worth pinning independently of the tool. +func TestChangedSpanHelpers(t *testing.T) { + t.Run("a single line replaced in place", func(t *testing.T) { + first, last, delta := changedLineSpan("a\nb\nc", "a\nB\nc") + if first != 2 || last != 2 || delta != 0 { + t.Fatalf("got (%d,%d,%d), want (2,2,0)", first, last, delta) + } + }) + t.Run("one line becomes three", func(t *testing.T) { + first, last, delta := changedLineSpan("a\nb\nc", "a\nx\ny\nz\nc") + if first != 2 || last != 2 || delta != 2 { + t.Fatalf("got (%d,%d,%d), want (2,2,2)", first, last, delta) + } + }) + t.Run("lines removed", func(t *testing.T) { + first, last, delta := changedLineSpan("a\nb\nc\nd", "a\nd") + if first != 2 || last != 3 || delta != -2 { + t.Fatalf("got (%d,%d,%d), want (2,3,-2)", first, last, delta) + } + }) + t.Run("bytes", func(t *testing.T) { + first, last, delta := changedByteSpan([]byte("abcdef"), []byte("abXYef")) + if first != 2 || last != 4 || delta != 0 { + t.Fatalf("got (%d,%d,%d), want (2,4,0)", first, last, delta) + } + }) +} + +// A file read whole IN PIECES must stay seen-whole after an edit. SeenWhole +// derives its answer from the ranges precisely because the raw flag is only set +// by a single covering read; RecordEdit branching on the flag instead sent this +// case down the split path, and the write_file refusal the derived answer exists +// to prevent came back within one edit. +func TestAFileReadWholeInPiecesStaysWholeAfterAnEdit(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "chunked.go") + content := "l1\nl2\nl3\nl4\n" + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + tracker := NewFileTracker() + info, _ := os.Stat(path) + tracker.Record(path, []byte(content), info) + // Two reads that together cover the file, neither covering it alone. + tracker.RecordSeenRange(path, 1, 2, 4) + tracker.RecordSeenRange(path, 3, 4, 4) + if !tracker.SeenWhole(path) { + t.Fatal("two chunked reads did not add up to seen-whole") + } + + updated := "l1\nl2X\nl3\nl4\n" + if err := os.WriteFile(path, []byte(updated), 0o600); err != nil { + t.Fatal(err) + } + newInfo, _ := os.Stat(path) + tracker.RecordEdit(path, []byte(content), []byte(updated), newInfo) + + if !tracker.SeenWhole(path) { + t.Error("a file read whole in two chunks stopped being seen whole after one edit") + } +} + +// observation.total is compared against read ranges, so it has to be the line +// count a READER would give. strings.Split leaves an empty final element for +// content ending in a newline, which counted one line too many — and since +// SeenWhole asks whether the ranges cover 1..total, that made full coverage +// unreachable for a file that had just been read in full. +func TestCountLinesMatchesWhatAReaderWouldSay(t *testing.T) { + for _, testCase := range []struct { + content string + want int + }{ + {"a\nb\nc\n", 3}, // the common case: trailing newline opens no line + {"a\nb\nc", 3}, + {"\n", 1}, + {"", 0}, + } { + if got := countLines([]byte(testCase.content)); got != testCase.want { + t.Errorf("countLines(%q) = %d, want %d", testCase.content, got, testCase.want) + } + } +} diff --git a/internal/tools/file_tracker.go b/internal/tools/file_tracker.go index 3444816eb..1902045a9 100644 --- a/internal/tools/file_tracker.go +++ b/internal/tools/file_tracker.go @@ -1,11 +1,13 @@ package tools import ( + "bytes" "crypto/sha256" "encoding/hex" "errors" "os" "sort" + "strings" "sync" "time" ) @@ -165,6 +167,241 @@ func (tracker *FileTracker) RecordSeenBytes(absPath string, start, end, total in tracker.seen[absPath] = observation } +// RecordEdit re-baselines absPath after an edit THIS SESSION made, keeping the +// reads the edit did not disturb. +// +// WHY THIS EXISTS RATHER THAN Record. RecordHash drops every recorded range when +// the content hash moves, which is right for a change we did not make: we cannot +// say which lines still hold what was read. After our own edit we can say +// exactly. The content before the first changed line is byte-identical and sits +// at the same line numbers; the content after the last changed line is +// byte-identical and has moved by a known delta. Only the lines the edit +// actually spans stop describing the file. +// +// Dropping the lot instead cost a real run. A 371-line file was read in three +// pieces (40-45, 85-260, 260-371) and one two-line edit at line 92 erased the +// credit for all three: the next six edits — into regions that had been read, +// that the edit did not touch, in a file whose line count had not changed — were +// each refused as content "not read in this session", and the repeated-failure +// guard halted the run. The error even told the model to re-read, which would +// have been undone by its next successful edit. The guard is right that a model +// must not edit what it has not seen; it was wrong about what it had seen. +func (tracker *FileTracker) RecordEdit(absPath string, before, after []byte, info os.FileInfo) { + if tracker == nil { + return + } + firstLine, lastLineBefore, lineDelta := changedLineSpan(string(before), string(after)) + firstByte, lastByteBefore, byteDelta := changedByteSpan(before, after) + + tracker.mu.Lock() + defer tracker.mu.Unlock() + + version := FileVersion{Hash: HashContent(after)} + if info != nil { + version.Size = info.Size() + version.MTime = info.ModTime() + } + tracker.versions[absPath] = version + + observation, tracked := tracker.seen[absPath] + if !tracked { + return + } + // The DERIVED answer, not the raw flag. Branching on observation.whole here + // sent a file read whole in two chunks — seenWhole true, flag false — into + // the split path below, where it stopped being seen whole after one edit. + if seenWhole(observation) { + // Still whole: every line was read, and an edit of ours does not make + // that untrue. Re-baseline as a single covering observation so the + // answer survives regardless of how the ranges shifted. + observation.whole = true + observation.ranges = nil + observation.byteRanges = nil + observation.total = countLines(after) + observation.totalBytes = len(after) + tracker.seen[absPath] = observation + return + } + + // SPLIT AROUND THE EDIT, never drop the whole range. A read almost always + // SPANS the line it is about to edit — that is why it was read — so dropping + // on overlap would have thrown away 85-260 to change line 92 and left the + // original defect in place under a longer implementation. + kept := make([]lineRange, 0, len(observation.ranges)+1) + for _, seen := range observation.ranges { + if end := min(seen.end, firstLine-1); seen.start <= end { + kept = append(kept, lineRange{start: seen.start, end: end}) + } + if start := max(seen.start, lastLineBefore+1); start <= seen.end { + kept = append(kept, lineRange{start: start + lineDelta, end: seen.end + lineDelta}) + } + } + observation.ranges = kept + if observation.total != 0 { + observation.total = countLines(after) + } + + // Same split, on half-open byte intervals. + keptBytes := make([]lineRange, 0, len(observation.byteRanges)+1) + for _, seen := range observation.byteRanges { + if end := min(seen.end, firstByte); seen.start < end { + keptBytes = append(keptBytes, lineRange{start: seen.start, end: end}) + } + if start := max(seen.start, lastByteBefore); start < seen.end { + keptBytes = append(keptBytes, lineRange{start: start + byteDelta, end: seen.end + byteDelta}) + } + } + observation.byteRanges = keptBytes + if observation.totalBytes != 0 { + observation.totalBytes = len(after) + } + tracker.seen[absPath] = observation +} + +// changedLineSpan reports the 1-based first line that differs between before and +// after, the 1-based last line of BEFORE that differs, and the line-count delta. +// +// Computed from a common prefix and suffix rather than from the caller's +// replacement spans: one edit_file call with replace_all can rewrite many +// scattered occurrences, and the span between the outermost two is the only +// region that is honestly unknown afterwards. +// The span is derived by scanning BYTES, not by materialising lines. +// +// The first version split both versions into []string. That allocates one string +// header per line for each version — work that scales with the SIZE OF THE FILE +// rather than the size of the edit, and it ran before RecordEdit had even checked +// whether there was a tracked observation to update. @jatmn measured 32,036,640 +// bytes for two 2 MB versions of a million short lines, and extrapolated roughly +// 1.6 GB of headers for a 100 MB file before counting the content and hashes +// already live alongside them. +// +// That mattered beyond memory pressure: edit_file writes the updated bytes BEFORE +// calling RecordEdit, so an out-of-memory kill lands after the user's file has +// changed and before the tracker baseline catches up — the file and the record of +// it disagree, and nothing says so. +// +// Bytes give the same answer in bounded memory. Two identical byte prefixes share +// every line that ends inside them, so counting newlines in the common prefix +// counts the unchanged leading lines directly. +func changedLineSpan(before, after string) (firstChanged, lastChangedBefore, delta int) { + beforeCount := countTrackedLines(before) + afterCount := countTrackedLines(after) + + prefix := commonLinePrefix(before, after) + // Never more leading lines than the shorter version has. The byte scan can + // otherwise claim a line that only one side possesses — two empty versions + // share a whole "line" that neither actually contains. + prefix = min(prefix, min(beforeCount, afterCount)) + // The suffix may not reach back past the lines the prefix already claimed, + // exactly as the line-array loop bounded itself. + suffix := commonLineSuffix(before, after, min(beforeCount-prefix, afterCount-prefix)) + return prefix + 1, beforeCount - suffix, afterCount - beforeCount +} + +// commonLinePrefix counts the leading lines that are byte-identical in both. +func commonLinePrefix(before, after string) int { + limit := min(len(before), len(after)) + at := 0 + for at < limit && before[at] == after[at] { + at++ + } + lines := strings.Count(before[:at], "\n") + // A LINE THAT ENDS EXACTLY WHERE THE SCAN STOPPED IS STILL SHARED. Scanning + // halts where the bytes diverge OR where the shorter version runs out, and in + // the second case the line containing that point can be complete and equal on + // both sides — "a\nb" against "a\nb\nc" diverges only because the first ended, + // and "b" is a whole matching line in each. + if lineBoundary(before, at) && lineBoundary(after, at) { + lines++ + } + return lines +} + +// commonLineSuffix counts the trailing lines that are byte-identical in both, up +// to the ceiling the prefix leaves. +func commonLineSuffix(before, after string, ceiling int) int { + if ceiling <= 0 { + return 0 + } + limit := min(len(before), len(after)) + back := 0 + for back < limit && before[len(before)-1-back] == after[len(after)-1-back] { + back++ + } + beforeAt, afterAt := len(before)-back, len(after)-back + // Each newline inside the shared tail closes a line that lies wholly within + // it. The line the tail STARTS in is shared only when the tail begins on a + // line boundary in both versions — otherwise its opening bytes differ and it + // is a changed line that merely ends the same way. + lines := strings.Count(before[beforeAt:], "\n") + if lineStart(before, beforeAt) && lineStart(after, afterAt) { + lines++ + } + return min(lines, ceiling) +} + +// lineBoundary reports whether at is the end of a line: the end of the text, or +// the position of the newline that closes it. +func lineBoundary(text string, at int) bool { return at == len(text) || text[at] == '\n' } + +// lineStart reports whether at begins a line. +func lineStart(text string, at int) bool { return at == 0 || text[at-1] == '\n' } + +// changedByteSpan is changedLineSpan in bytes: the first differing offset, the +// end offset of the changed region in BEFORE, and the size delta. +func changedByteSpan(before, after []byte) (firstChanged, lastChangedBefore, delta int) { + prefix := 0 + for prefix < len(before) && prefix < len(after) && before[prefix] == after[prefix] { + prefix++ + } + suffix := 0 + for suffix < len(before)-prefix && suffix < len(after)-prefix && + before[len(before)-1-suffix] == after[len(after)-1-suffix] { + suffix++ + } + return prefix, len(before) - suffix, len(after) - len(before) +} + +// countLines reports the line count a READER would give the content, which is +// what observation.total is compared against. +// +// The trailing newline does not open a line. strings.Split leaves an empty final +// element for "a\nb\nc\n" and so counted 4 where a read reports 3 — and because +// SeenWhole asks whether the ranges cover 1..total, an inflated total made full +// coverage unreachable for the very file that had just been read in full. Almost +// every text file ends in a newline, so this was the common case rather than an +// edge one. +// +// countTrackedLines keeps the OTHER rule deliberately: the span arithmetic has to +// match the indices strings.Split produced, so it counts the empty element after +// a trailing newline where this one does not. Two rules, two names, both stated — +// they were one function with an if, and that is how they were confused. +func countLines(content []byte) int { + // Counted, not split. This only ever needed a number, and building a slice of + // every line to take its length was the third full per-line allocation in the + // same call path. + if len(content) == 0 { + return 0 + } + lines := bytes.Count(content, []byte{'\n'}) + if content[len(content)-1] != '\n' { + lines++ + } + return lines +} + +// countTrackedLines is countLines' rule for the string form: the number of +// elements strings.Split would produce, which counts the empty element after a +// trailing newline. The two differ deliberately — observation.total compares +// against what a READER reports, while the span arithmetic has to match the +// indices the split produced. +func countTrackedLines(text string) int { + if text == "" { + return 0 + } + return strings.Count(text, "\n") + 1 +} + // coversFully reports whether ranges together cover every line in [start, end]. // // Ranges are merged rather than scanned line by line: a caller asking about a @@ -274,14 +511,27 @@ func (tracker *FileTracker) SeenWhole(absPath string) bool { if !ok { return false } + return seenWhole(observation) +} + +// seenWhole is the DERIVED answer to "has every line been seen", and the single +// place that decides it. Callers must hold the lock. +// +// Derived from the ranges rather than read off the flag, because the flag is +// only ever set by a SINGLE read covering the file: a file read in two halves +// stayed "not seen whole" forever even though SeenRange agreed every line had +// been seen, and write_file, which gates on this, refused the overwrite with +// advice to read the file that could not change the answer. +// +// IT HAS TO BE ONE FUNCTION. RecordEdit branched on the raw flag while this +// derived the answer, so the very case the derivation exists to rescue fell into +// the wrong arm and lost: a file read whole in two chunks reported SeenWhole +// true, then reported false after a single edit, putting the write_file refusal +// back within one edit of where it was fixed. +func seenWhole(observation fileObservation) bool { if observation.whole { return true } - // Derived from the ranges rather than tracked as its own flag. The flag was - // only ever set by a SINGLE read covering the file, so a file read in two - // halves stayed "not seen whole" forever even though SeenRange agreed every - // line had been seen — and write_file, which gates on this, then refused the - // overwrite with advice to read the file that could not change the answer. if observation.total > 0 && coversFully(observation.ranges, 1, observation.total) { return true } diff --git a/internal/tools/file_tracker_test.go b/internal/tools/file_tracker_test.go index 1da216073..d31bd50d2 100644 --- a/internal/tools/file_tracker_test.go +++ b/internal/tools/file_tracker_test.go @@ -3,6 +3,7 @@ package tools import ( "os" "path/filepath" + "strings" "testing" ) @@ -153,3 +154,85 @@ func TestNilFileTrackerCreatedFilesIsANoop(t *testing.T) { t.Fatalf("CreatedFiles() on nil tracker = %v, want nil", got) } } + +// THE SPAN IS DERIVED IN BOUNDED MEMORY, whatever the file size. +// +// changedLineSpan used to split both versions into []string, so it allocated one +// string header per line for each — work scaling with the SIZE OF THE FILE rather +// than the size of the edit, and it ran before RecordEdit had even checked whether +// there was an observation to update. @jatmn measured 32,036,640 bytes for two +// 2 MB versions of a million short lines. +// +// The cost was not only memory. edit_file writes the updated bytes BEFORE calling +// RecordEdit, so an out-of-memory kill lands after the user's file has changed and +// before the tracker baseline catches up — file and record disagree, silently. +// +// This asserts an absolute ceiling rather than a ratio, so a future rewrite that +// reintroduces per-line slices fails here rather than merely getting slower. +func TestTheChangedSpanDoesNotAllocatePerLine(t *testing.T) { + before := strings.Repeat("x\n", 200_000) + after := before[:len(before)-2] + "y\n" + + spanBytes := testing.Benchmark(func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + changedLineSpan(before, after) + } + }).AllocedBytesPerOp() + if spanBytes > 4096 { + t.Errorf("changedLineSpan allocated %d bytes for a %d-line pair; per-line slices are back", spanBytes, 200_000) + } + + content := []byte(before) + countBytes := testing.Benchmark(func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + countLines(content) + } + }).AllocedBytesPerOp() + if countBytes > 4096 { + t.Errorf("countLines allocated %d bytes to return a number", countBytes) + } +} + +// AND IT GIVES THE SAME ANSWERS THE LINE-ARRAY VERSION DID. The rewrite is only +// safe if it is behaviour-preserving, so the replaced implementation is kept here +// as an oracle and both are asked the same questions. This caught 206 mismatches +// in the first attempt at the byte scan — empty versions and partially shared +// trailing lines, neither of which the hand-written cases below would have found. +func TestTheByteScanAgreesWithTheLineSplit(t *testing.T) { + oracle := func(before, after string) (int, int, int) { + split := func(text string) []string { + if text == "" { + return nil + } + return strings.Split(text, "\n") + } + beforeLines, afterLines := split(before), split(after) + prefix := 0 + for prefix < len(beforeLines) && prefix < len(afterLines) && beforeLines[prefix] == afterLines[prefix] { + prefix++ + } + suffix := 0 + for suffix < len(beforeLines)-prefix && suffix < len(afterLines)-prefix && + beforeLines[len(beforeLines)-1-suffix] == afterLines[len(afterLines)-1-suffix] { + suffix++ + } + return prefix + 1, len(beforeLines) - suffix, len(afterLines) - len(beforeLines) + } + + versions := []string{ + "", "a", "\n", "a\n", "a\nb", "a\nb\n", "a\nb\nc", "a\nb\nc\n", + "\n\n", "x\n\ny", "a\nb\nc\nd\ne", "same\nsame\nsame\n", "ab\nc", "b\nc", + } + for _, before := range versions { + for _, after := range versions { + wantFirst, wantLast, wantDelta := oracle(before, after) + gotFirst, gotLast, gotDelta := changedLineSpan(before, after) + if wantFirst != gotFirst || wantLast != gotLast || wantDelta != gotDelta { + t.Errorf("changedLineSpan(%q, %q) = (%d, %d, %d), want (%d, %d, %d)", + before, after, gotFirst, gotLast, gotDelta, wantFirst, wantLast, wantDelta) + } + } + } +}