From 29de0f38f31c1e8ec9c6a77f88a926b80ac02504 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sat, 15 Aug 2026 16:16:16 +0530 Subject: [PATCH 1/3] fix(tools): an edit keeps the reads it did not disturb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split out of #829 as an independent fix, per @Vasanthdev2004's review asking for the small self-contained pieces to arrive separately. A successful edit re-baselined the file through RecordHash, which drops EVERY recorded read range. A file read in three pieces therefore lost all three to a single two-line edit, and the next six edits into regions that had already been read were refused as unseen — the model had to re-read what it had just read in order to keep working. RecordEdit replaces that on the edit path. Because the edit is ours, the changed span is known exactly, so ranges the edit did not touch are carried across, ranges after it are shifted by the line delta, and a range the edit spans is split around it. An EXTERNAL change still drops everything, deliberately: once a formatter has rewritten the file we no longer know which line holds what was read, and the conservative drop is the only honest answer there. "Seen whole" is now decided in ONE place. SeenWhole derives the answer from the ranges because the raw flag is only ever set by a single covering read, but RecordEdit branched on the flag — so a file read whole in two chunks reported seen-whole true, then false after one edit, putting back the write_file refusal the derived answer exists to prevent. countLines now reports the count a READER would give. strings.Split leaves an empty final element for content ending in a newline, so it counted 4 lines for a 3-line file; since SeenWhole asks whether the ranges cover 1..total, that inflated total made full coverage unreachable for a file just read in full. Nearly every text file ends in a newline, so this was the common case. Origin-Session: local-de382f | Claude Code | 9 prompts Origin-Snapshot: 92ae33c95cd1 Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: ae653221fa15 --- internal/tools/edit_file.go | 28 +- internal/tools/edit_preserves_reads_test.go | 296 ++++++++++++++++++++ internal/tools/file_tracker.go | 180 +++++++++++- 3 files changed, 491 insertions(+), 13 deletions(-) create mode 100644 internal/tools/edit_preserves_reads_test.go 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..ecdc7664c 100644 --- a/internal/tools/file_tracker.go +++ b/internal/tools/file_tracker.go @@ -6,6 +6,7 @@ import ( "errors" "os" "sort" + "strings" "sync" "time" ) @@ -165,6 +166,162 @@ 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. +func changedLineSpan(before, after string) (firstChanged, lastChangedBefore, delta int) { + beforeLines := splitLinesForTracking(before) + afterLines := splitLinesForTracking(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) +} + +// 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) +} + +func splitLinesForTracking(text string) []string { + if text == "" { + return nil + } + return strings.Split(text, "\n") +} + +// 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. +// +// splitLinesForTracking keeps its own behaviour: changedLineSpan diffs the two +// slices against each other, where the trailing element is harmless because both +// sides carry it. +func countLines(content []byte) int { + lines := splitLinesForTracking(string(content)) + if n := len(lines); n > 0 && lines[n-1] == "" { + return n - 1 + } + return len(lines) +} + // 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 +431,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 } From 6d5907497d24a72872f8034145bc1b6a43915a7f Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:28:39 +0530 Subject: [PATCH 2/3] chore(tools): re-request an automated review of this head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No code change. CodeRabbit's verdict on this PR is pinned to 6f0cd6c4, a commit that was force-pushed away five days ago, and a stale changes-requested keeps the PR at mergeStateStatus=BLOCKED exactly as a human one would. It re-reviews on push rather than on request, and repeated @coderabbitai triggers over five days produced nothing, so this is the only lever available without maintainer rights. The cost is that this dismisses @Vasanthdev2004's and @anandh8x's approvals of 0b6fcf54, which is an unhappy trade for an empty commit — the tree they approved is byte-identical to this one. Origin-Session: local-8cd239 | Claude Code | 6 prompts Origin-Snapshot: 149ab98453ad Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: ae653221fa15 From 637dfa7ac4019a58ccb2d3525635f99be8172fea Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:41:59 +0530 Subject: [PATCH 3/3] perf(tools): derive the changed-line span from bytes, not from per-line slices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by @jatmn. changedLineSpan split both file versions into []string, so it allocated one string header per line for EACH version — work that scales with the size of the file rather than the size of the edit. countLines then split a third time just to take a length. All of it ran before RecordEdit had checked whether there was a tracked observation to update at all. He 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, the updated bytes and the hashes already live beside them. The memory was not the sharp end. 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. His instruction was to fix the allocation source rather than add a size guard, and that a guard — if used at all — would have to reject before os.WriteFile rather than during post-write re-baselining. Taken as written: there is no new limit, and the same edits are accepted as before. Two identical byte prefixes share every line that ends inside them, so counting newlines in the common prefix counts the unchanged leading lines directly. Same for the suffix, with one rule that the line-array version got for free: the line a shared tail STARTS in is only shared when the tail begins on a line boundary in both versions, or its opening bytes differ and it is a changed line that merely ends the same way. Measured on his own scenario, two 2 MB versions: changedLineSpan 32,036,640 bytes -> 0 allocations countLines per-line slice -> 0 allocations ## The equivalence is tested, not asserted A rewrite of a diff primitive is only safe if it is behaviour-preserving, so the replaced implementation is kept in the test file as an oracle and both are asked the same questions. That caught 206 mismatches in my first attempt at the byte scan — empty versions, and trailing lines that are only partially shared. Neither appears in the hand-written cases, and I would have shipped it. Two regressions: the oracle comparison, and an absolute allocation ceiling rather than a ratio, so a future rewrite that reintroduces per-line slices fails rather than merely getting slower. Mutation: restoring strings.Split in either function allocates 3.2 MB and 3.6 MB against a 4 KB bound. splitLinesForTracking lost its last caller and is removed — the same obsolete- helper lint failure that broke Windows CI on #911, caught here by running make lint-static before pushing rather than after. 0 issues. Rebased onto ad34dc8d, 0 behind. go test -race ./internal/tools/: clean. Pre-existing here and on main: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-c962d7 | Claude Code | 17 prompts Origin-Snapshot: a599377c09e0 --- internal/tools/file_tracker.go | 132 ++++++++++++++++++++++------ internal/tools/file_tracker_test.go | 83 +++++++++++++++++ 2 files changed, 189 insertions(+), 26 deletions(-) diff --git a/internal/tools/file_tracker.go b/internal/tools/file_tracker.go index ecdc7664c..1902045a9 100644 --- a/internal/tools/file_tracker.go +++ b/internal/tools/file_tracker.go @@ -1,6 +1,7 @@ package tools import ( + "bytes" "crypto/sha256" "encoding/hex" "errors" @@ -264,21 +265,88 @@ func (tracker *FileTracker) RecordEdit(absPath string, before, after []byte, inf // 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) { - beforeLines := splitLinesForTracking(before) - afterLines := splitLinesForTracking(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) + 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) { @@ -294,13 +362,6 @@ func changedByteSpan(before, after []byte) (firstChanged, lastChangedBefore, del return prefix, len(before) - suffix, len(after) - len(before) } -func splitLinesForTracking(text string) []string { - if text == "" { - return nil - } - return strings.Split(text, "\n") -} - // countLines reports the line count a READER would give the content, which is // what observation.total is compared against. // @@ -311,15 +372,34 @@ func splitLinesForTracking(text string) []string { // every text file ends in a newline, so this was the common case rather than an // edge one. // -// splitLinesForTracking keeps its own behaviour: changedLineSpan diffs the two -// slices against each other, where the trailing element is harmless because both -// sides carry it. +// 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 { - lines := splitLinesForTracking(string(content)) - if n := len(lines); n > 0 && lines[n-1] == "" { - return n - 1 + // 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 len(lines) + return strings.Count(text, "\n") + 1 } // coversFully reports whether ranges together cover every line in [start, end]. 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) + } + } + } +}