diff --git a/internal/agent/learn.go b/internal/agent/learn.go index a6378f4..e629e5d 100644 --- a/internal/agent/learn.go +++ b/internal/agent/learn.go @@ -104,11 +104,21 @@ func (a *Agent) lessonsBlock(ctx context.Context) string { return "" } var lessons []string + seen := make(map[string]struct{}) for _, m := range items { if m.Source == lessonSource { - lessons = append(lessons, m.Content) + lesson := strings.TrimSpace(m.Content) + if !usableLesson(lesson) { + continue + } + key := strings.ToLower(lesson) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + lessons = append(lessons, lesson) } - if len(lessons) >= 20 { + if len(lessons) >= 12 { break } } @@ -123,3 +133,20 @@ func (a *Agent) lessonsBlock(ctx context.Context) string { } return b.String() } + +// usableLesson keeps malformed auxiliary-model output out of the system +// prompt. Historical rows include fragments such as "NONE", "Wait", and +// duplicated partial sentences; presenting them as instructions makes tool +// behavior less predictable and wastes context. +func usableLesson(s string) bool { + if len(s) < 32 || len(s) > 400 { + return false + } + if strings.EqualFold(s, "none") || strings.HasSuffix(strings.ToLower(s), " none") { + return false + } + if strings.EqualFold(s, "wait") || strings.EqualFold(s, ".") { + return false + } + return true +} diff --git a/internal/agent/learn_test.go b/internal/agent/learn_test.go new file mode 100644 index 0000000..d377a8f --- /dev/null +++ b/internal/agent/learn_test.go @@ -0,0 +1,20 @@ +package agent + +import "testing" + +func TestUsableLessonRejectsAuxiliaryFragments(t *testing.T) { + for _, lesson := range []string{"NONE", "Wait", ".", "When `edit_file"} { + if usableLesson(lesson) { + t.Errorf("malformed lesson accepted: %q", lesson) + } + } + if !usableLesson("When edit_file fails with old_string not found, read the current file and copy exact whitespace before retrying.") { + t.Fatal("valid lesson rejected") + } +} + +func TestUsableLessonRejectsTrailingNone(t *testing.T) { + if usableLesson("When a tool fails, inspect the concrete error and retry with corrected arguments. NONE") { + t.Fatal("auxiliary NONE suffix should not enter the prompt") + } +} diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 39bd158..6a78da2 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -106,6 +106,7 @@ help them now — do not block them. // 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") } if hasTool(active, "vps_upload") || hasTool(active, "vps_download") || hasTool(active, "vps_run") { // Without this, models fall back to terminal rsync/scp and never use diff --git a/internal/tools/background_process_unix.go b/internal/tools/background_process_unix.go index caf7214..e51b68a 100644 --- a/internal/tools/background_process_unix.go +++ b/internal/tools/background_process_unix.go @@ -8,7 +8,16 @@ import ( ) func configureProcessGroup(cmd *exec.Cmd) { - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if cmd == nil { + return + } + // Sandboxed commands may already carry Cloneflags, uid mappings, or a + // parent-death signal. Preserve those settings while adding the process + // group needed to terminate a command tree on timeout. + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.Setpgid = true } func terminateProcessGroup(cmd *exec.Cmd) { diff --git a/internal/tools/file.go b/internal/tools/file.go index dee5481..738c5ae 100644 --- a/internal/tools/file.go +++ b/internal/tools/file.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "sort" + "strconv" "strings" "unicode/utf8" ) @@ -179,12 +180,20 @@ func (readFileTool) Execute(_ context.Context, in Input) Result { if len(data) > maxReadBytes { data = data[:maxReadBytes] truncatedBytes = true + // The cut can land inside a multi-byte rune; trimming up to three + // trailing bytes keeps a genuine text file from reading as binary. + for i := 0; i < 3 && len(data) > 0 && !utf8.Valid(data); i++ { + data = data[:len(data)-1] + } } if !utf8.Valid(data) { return Errorf("%s appears to be a binary file (%d bytes)", args.Path, fi.Size()) } - lines := strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") + normalized := strings.ReplaceAll(string(data), "\r\n", "\n") + // Lone CR (classic Mac) must also split, or the file displays as one line. + normalized = strings.ReplaceAll(normalized, "\r", "\n") + lines := strings.Split(normalized, "\n") offset := args.Offset if offset <= 0 { offset = 1 @@ -323,9 +332,25 @@ 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("old_string appears %d times in %s; add more surrounding context or set replace_all", count, args.Path) + return Errorf("%s", editAmbiguousMessage(args.Path, content, oldString, count)) } var updated string @@ -352,17 +377,40 @@ func (editFileTool) Execute(_ context.Context, in Input) Result { } } -// fileEOL returns the dominant newline sequence used in s. +// fileEOL returns the dominant newline sequence used in s, by majority. A +// single stray CRLF or CR in an otherwise-LF file must not decide the flavor: +// that used to convert every multi-line old_string away from what the file +// actually contains and permanently break edits on such files. func fileEOL(s string) string { - if strings.Contains(s, "\r\n") { + crlf := strings.Count(s, "\r\n") + lf := strings.Count(s, "\n") - crlf + cr := strings.Count(s, "\r") - crlf + if crlf > 0 && crlf >= lf && crlf >= cr { return "\r\n" } - if strings.Contains(s, "\r") { + if cr > lf { return "\r" } return "\n" } +// eolOf reports the single newline flavor used in s, or "" when s has no +// newlines or mixes flavors. +func eolOf(s string) string { + crlf := strings.Count(s, "\r\n") + lf := strings.Count(s, "\n") - crlf + cr := strings.Count(s, "\r") - crlf + switch { + case crlf > 0 && lf == 0 && cr == 0: + return "\r\n" + case lf > 0 && crlf == 0 && cr == 0: + return "\n" + case cr > 0 && crlf == 0 && lf == 0: + return "\r" + } + return "" +} + // toEOL rewrites every newline in s to the given eol sequence. func toEOL(s, eol string) string { s = strings.ReplaceAll(s, "\r\n", "\n") @@ -394,6 +442,7 @@ func stripReadFileLinePrefixes(s string) (string, bool) { } 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 { @@ -404,8 +453,21 @@ func stripReadFileLinePrefixes(s string) (string, bool) { 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" @@ -418,32 +480,45 @@ func stripReadFileLinePrefixes(s string) (string, bool) { // 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. func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string, count int, how string) { - eol := fileEOL(content) + // 0. Verbatim bytes. Mixed-EOL files and stray CR bytes made the old + // normalize-first order fail edits whose old_string was byte-perfect. + if c := strings.Count(content, oldIn); c > 0 { + flav := eolOf(oldIn) + if flav == "" { + flav = fileEOL(content) + } + return oldIn, toEOL(newIn, flav), c, "" + } + + // Candidate flavors for normalized matching: the file's dominant flavor + // first, then the alternatives a mixed-EOL file may need. + flavors := []string{fileEOL(content), "\n", "\r\n"} try := func(oldCand, newCand, label string) bool { - o := toEOL(oldCand, eol) - n := toEOL(newCand, eol) - if o == "" { - return false - } - c := strings.Count(content, o) - if c == 0 { - return false + 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 } - oldString, newString, count, how = o, n, c, label - return true + return false } - // 1. Exact / EOL-normalized (covers LF paste against a CRLF file). - if try(oldIn, newIn, "") { - // Only annotate when the on-disk form actually differs from the input - // (i.e. we rewrote newlines). A pure exact match stays silent. - if oldString != oldIn { - how = "normalized line endings to match file" - } + // 1. EOL-normalized (covers LF paste against a CRLF file and vice versa). + if try(oldIn, newIn, "normalized line endings to match file") { return } @@ -463,6 +538,128 @@ func resolveEditMatch(content, oldIn, newIn string) (oldString, newString string return oldIn, newIn, 0, "" } +// lineSpan is the [start,end) byte range of one line's text in the original +// content, excluding its \n, \r\n, or lone \r terminator. +type lineSpan struct{ start, end int } + +func lineSpans(content string) []lineSpan { + var spans []lineSpan + start := 0 + i := 0 + for i < len(content) { + switch content[i] { + case '\n': + spans = append(spans, lineSpan{start, i}) + i++ + start = i + case '\r': + spans = append(spans, lineSpan{start, i}) + if i+1 < len(content) && content[i+1] == '\n' { + i += 2 + } else { + i++ + } + start = i + default: + i++ + } + } + if start < len(content) { + spans = append(spans, lineSpan{start, len(content)}) + } + 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 + } + + 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 + } + + 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 +} + +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). func editNotFoundMessage(path, content, oldString string) string { @@ -488,10 +685,140 @@ func editNotFoundMessage(path, content, oldString string) 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.") 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) + if lines := occurrenceLines(content, oldString, 12); len(lines) > 0 { + b.WriteString(" Current match line(s): ") + for i, line := range lines { + if i > 0 { + b.WriteString(", ") + } + fmt.Fprintf(&b, "%d", line) + } + b.WriteByte('.') + } + b.WriteString(" Re-read the current file and include enough neighbouring lines for exactly one match.") + return b.String() +} + +func occurrenceLines(content, needle string, max int) []int { + if needle == "" || max <= 0 { + return nil + } + var lines []int + for from := 0; from < len(content) && len(lines) < max; { + i := strings.Index(content[from:], needle) + if i < 0 { + break + } + at := from + i + lines = append(lines, 1+strings.Count(content[:at], "\n")) + from = at + len(needle) + } + return lines +} + +// 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 { + for _, token := range identifierTokens(oldString) { + if len(token) < 8 || strings.Contains(strings.ToLower(token), "read_file") { + continue + } + var hits []string + for i, line := range strings.Split(content, "\n") { + if strings.Contains(line, token) { + line = strings.TrimRight(line, "\r") + if len(line) > 180 { + line = line[:180] + "..." + } + hits = append(hits, fmt.Sprintf("line %d: %s", i+1, line)) + if len(hits) == 3 { + break + } + } + } + if len(hits) > 0 { + return "Near-miss lines sharing a token (re-read them; do not invent identifiers): " + strings.Join(hits, " ") + } + } + return "" +} + +func identifierTokens(s string) []string { + var out []string + start := -1 + flush := func(end int) { + if start >= 0 && end-start >= 8 { + out = append(out, s[start:end]) + } + start = -1 + } + for i := 0; i < len(s); i++ { + c := s[i] + isID := (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' + if isID { + if start < 0 { + start = i + } + } else { + flush(i) + } + } + flush(len(s)) + for i := 0; i < len(out); i++ { + for j := i + 1; j < len(out); j++ { + if len(out[j]) > len(out[i]) { + out[i], out[j] = out[j], out[i] + } + } + } + return out +} + // expandTabs replaces leading and embedded tabs with spaces at the given width // (stop-based), used only for mismatch diagnosis. func expandTabs(s string, width int) string { diff --git a/internal/tools/file_edit_regression_test.go b/internal/tools/file_edit_regression_test.go index 614d34e..b902e07 100644 --- a/internal/tools/file_edit_regression_test.go +++ b/internal/tools/file_edit_regression_test.go @@ -142,6 +142,243 @@ func TestEditFileDiagnosesTabVsSpaceMismatch(t *testing.T) { } } +func TestEditFileAmbiguousListsCurrentMatchLines(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "repeat.go") + content := "func a() {\n\treturn value\n}\nfunc b() {\n\treturn value\n}\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"path": "repeat.go", "old_string": "\treturn value", "new_string": "\treturn other"}) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError || !strings.Contains(result.Content, "Current match line(s): 2, 5") { + t.Fatalf("unexpected ambiguity result: %+v", result) + } +} + +func TestEditFileNotFoundShowsNearMiss(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "names.go") + if err := os.WriteFile(path, []byte("func attachEntity() {}\n"), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"path": "names.go", "old_string": "func attachEntit() {}", "new_string": "func attachEntity2() {}"}) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if !result.IsError || !strings.Contains(result.Content, "attachEntity") { + t.Fatalf("near-miss missing from result: %+v", result) + } +} + +func TestEditFileRecoversUniqueNearInsertionWithoutChangingExistingLine(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" + if err := os.WriteFile(path, []byte(actual), 0o644); err != nil { + t.Fatal(err) + } + old := "| **pool39v2** | 14 hand + 25 vision-audited | 33 train / 6 val | T4 | 85 (ES@55) | **0.009** | `artifacts/pool39v2/` |" + 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) + } + 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) + } +} + +// 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) { + 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" + 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) + } + 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) + } +} + +// replace_all promises "replace every exact occurrence"; a similarity-based +// recovery must never piggyback on it and multiply insertions. +func TestEditFileAdjacentInsertionIgnoredWithReplaceAll(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "ra.txt") + content := "return nil // TODO cleanup\nmiddle\nreturn nil\nend\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + old := "return nil" + args, _ := json.Marshal(map[string]any{ + "path": "ra.txt", "old_string": old, "new_string": old + "\nINSERTED", "replace_all": true, + }) + 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) + } + 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) + } +} + +// A file with mixed line endings must never reject an old_string whose bytes +// match the file exactly. (fileEOL used to pick CRLF because one line used it, +// then converted the LF old_string so it matched nothing.) +func TestEditFileExactMatchOnMixedEOLFile(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "mixed.txt") + content := "alpha\r\nbeta\nGAMMA\ndelta\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{ + "path": "mixed.txt", "old_string": "beta\nGAMMA", "new_string": "beta\nGAMMA2", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("exact byte match rejected on mixed-EOL file: %s", result.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := "alpha\r\nbeta\nGAMMA2\ndelta\n" + if string(got) != want { + t.Fatalf("edited = %q, want %q", got, want) + } +} + +// One stray lone CR byte anywhere in an LF file used to flip fileEOL to "\r" +// and permanently break every multi-line edit in that file. +func TestEditFileExactMatchDespiteStrayCR(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, "cr.txt") + content := "one\ntwo\nnote ends\rrest\nfour\nfive\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{ + "path": "cr.txt", "old_string": "four\nfive", "new_string": "four\nFIVE", + }) + result := (editFileTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) + if result.IsError { + t.Fatalf("stray CR poisoned an exact match: %s", result.Content) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := "one\ntwo\nnote ends\rrest\nfour\nFIVE\n" + if string(got) != want { + t.Fatalf("edited = %q, want %q", got, want) + } +} + +// 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) { + workspace := t.TempDir() + path := filepath.Join(workspace, "big.txt") + // "é" is 2 bytes; place it so the maxReadBytes cut lands inside it. + content := strings.Repeat("a", maxReadBytes-1) + "é" + strings.Repeat("b", 16) + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + in := Input{Workspace: workspace, Args: []byte(`{"path":"big.txt"}`)} + result := (readFileTool{}).Execute(context.Background(), in) + if result.IsError { + t.Fatalf("truncated UTF-8 file misread as binary: %s", result.Content) + } + if !strings.Contains(result.Content, "file truncated") { + t.Fatalf("missing truncation notice: %s", result.Content) + } +} + +// 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) { + workspace := t.TempDir() + path := filepath.Join(workspace, "old.txt") + if err := os.WriteFile(path, []byte("a\rb\rc"), 0o644); err != nil { + t.Fatal(err) + } + in := Input{Workspace: workspace, Args: []byte(`{"path":"old.txt"}`)} + result := (readFileTool{}).Execute(context.Background(), in) + 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) + } +} + +func TestEditFileDoesNotRecoverAmbiguousNearInsertion(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" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + old := "| **pool39v2** | 85 (ES@55) | artifacts/c |" + args, _ := json.Marshal(map[string]any{ + "path": "README.md", "old_string": old, "new_string": old + "\n| new row |", + }) + 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) + } + 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) @@ -160,3 +397,18 @@ func TestStripReadFileLinePrefixes(t *testing.T) { 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) + } +} diff --git a/internal/tools/search.go b/internal/tools/search.go index f7aab36..6c4c082 100644 --- a/internal/tools/search.go +++ b/internal/tools/search.go @@ -47,7 +47,9 @@ func (globTool) Execute(_ context.Context, in Input) Result { if args.Limit <= 0 || args.Limit > 2000 { args.Limit = 200 } - root, err := resolvePath(in.Workspace, args.Path) + // Same read boundary as read_file: workspace-confined in an ordinary + // session, anywhere in a project session. + root, err := resolveRead(in, args.Path) if err != nil { return Errorf("%v", err) } @@ -199,7 +201,9 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { if err != nil { return Errorf("invalid regular expression: %v", err) } - root, err := resolvePath(in.Workspace, args.Path) + // Same read boundary as read_file: workspace-confined in an ordinary + // session, anywhere in a project session. + root, err := resolveRead(in, args.Path) if err != nil { return Errorf("%v", err) } @@ -211,10 +215,11 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { } var ( - b strings.Builder - matches int - files int - stopped bool + b strings.Builder + matches int + files int + stopped bool + warnings []string ) searchFile := func(path, display string) error { @@ -272,6 +277,11 @@ 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 } @@ -307,14 +317,18 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { _ = searchFile(root, relTo(in.Workspace, root)) } + warn := "" + if len(warnings) > 0 { + warn = "\nwarning: " + strings.Join(warnings, "\nwarning: ") + } if matches == 0 { - return Text(fmt.Sprintf("No matches for %q under %s", args.Pattern, relTo(in.Workspace, root))) + return Text(fmt.Sprintf("No matches for %q under %s%s", args.Pattern, relTo(in.Workspace, root), warn)) } header := fmt.Sprintf("%d match(es) in %d file(s) for %q", matches, files, args.Pattern) if stopped { header += " (limit reached)" } - return Text(header + "\n" + b.String()) + return Text(header + "\n" + b.String() + warn) } func truncateLine(s string) string { diff --git a/internal/tools/search_test.go b/internal/tools/search_test.go new file mode 100644 index 0000000..119a423 --- /dev/null +++ b/internal/tools/search_test.go @@ -0,0 +1,63 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// grep and glob must follow the same read boundary as read_file/list_files: +// confined to the workspace in an ordinary session, free to search anywhere in +// a project session (WriteRoots set). +func TestGrepAndGlobFollowProjectReadBoundary(t *testing.T) { + project := t.TempDir() + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "ref.txt"), []byte("needle content\n"), 0o644); err != nil { + t.Fatal(err) + } + + grepArgs, _ := json.Marshal(map[string]any{"pattern": "needle", "path": outside}) + projectIn := Input{Workspace: project, WriteRoots: []string{project}, Args: grepArgs} + result := (grepTool{}).Execute(context.Background(), projectIn) + if result.IsError || !strings.Contains(result.Content, "needle content") { + t.Fatalf("project-session grep outside workspace should match, got: %+v", result) + } + + globArgs, _ := json.Marshal(map[string]any{"pattern": "*.txt", "path": outside}) + result = (globTool{}).Execute(context.Background(), Input{Workspace: project, WriteRoots: []string{project}, Args: globArgs}) + if result.IsError || !strings.Contains(result.Content, "ref.txt") { + t.Fatalf("project-session glob outside workspace should match, got: %+v", result) + } + + // Ordinary sessions keep the old confinement. + result = (grepTool{}).Execute(context.Background(), Input{Workspace: project, Args: grepArgs}) + if !result.IsError { + t.Fatalf("ordinary-session grep outside workspace must be refused, got: %+v", result) + } + result = (globTool{}).Execute(context.Background(), Input{Workspace: project, Args: globArgs}) + if !result.IsError { + t.Fatalf("ordinary-session glob outside workspace must be refused, got: %+v", result) + } +} + +// 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) { + 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 { + t.Fatal(err) + } + args, _ := json.Marshal(map[string]any{"pattern": "NEEDLE", "path": "."}) + 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, "big.txt") || !strings.Contains(result.Content, "stopped") { + t.Fatalf("overlong line not reported: %q", result.Content) + } +} diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 858476b..8df2f54 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -219,6 +219,11 @@ func (m *ShellManager) session(id, workspace string) (*shellSession, error) { out := &lockedBuffer{} cmd.Stdout = out cmd.Stderr = out + // Keep the persistent shell and every foreground command it starts in an + // isolated process group. A timed-out command must be terminated as a unit; + // killing only the shell can leave descendants holding the shell's pipes and + // wedge the session for all subsequent calls. + configureProcessGroup(cmd) if err := cmd.Start(); err != nil { return nil, fmt.Errorf("start shell: %w", err) } @@ -312,8 +317,11 @@ func (m *ShellManager) ReapIdle(lifetime time.Duration) { func (s *shellSession) killLocked() { if s.cmd != nil && s.cmd.Process != nil { _ = s.stdin.Close() - _ = s.cmd.Process.Kill() - // Process.Kill returns before the OS has released the process's + // Kill the whole process group, not just the shell: a timed-out + // command's descendants keep the shell's pipes open and would wedge + // the session even after the shell itself is gone. + killProcessGroup(s.cmd) + // The kill returns before the OS has released the process's // handles (including its working directory), which races callers that // remove the workspace right after CloseAll. Wait for the Wait // goroutine so resources are actually freed; cap the wait so a wedged diff --git a/internal/tools/shell_test.go b/internal/tools/shell_test.go index 1090e0e..2fd9e52 100644 --- a/internal/tools/shell_test.go +++ b/internal/tools/shell_test.go @@ -128,6 +128,46 @@ func TestPersistentShellExitReturnsPromptlyAndRecovers(t *testing.T) { } } +func TestPersistentShellTimeoutKillsCommandAndRecovers(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX process-group behavior does not apply on Windows") + } + + m := NewShellManager(config.Terminal{}) + t.Cleanup(m.CloseAll) + workspace := t.TempDir() + sess, err := m.session("timeout-session", workspace) + if err != nil { + t.Fatal(err) + } + + start := time.Now() + _, _, err = sess.run(context.Background(), "sleep 60", 150*time.Millisecond, nil) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("timeout error = %v, want timed out", err) + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("timeout returned after %s, want prompt cancellation", elapsed) + } + if !sess.dead.Load() { + t.Fatal("timed-out shell was left marked live") + } + + // The next call must receive a replacement shell rather than appending to + // the command that was killed at the timeout boundary. + replacement, err := m.session("timeout-session", workspace) + if err != nil { + t.Fatal(err) + } + if replacement == sess { + t.Fatal("timed-out persistent shell was reused") + } + out, code, err := replacement.run(context.Background(), "printf RECOVERED", 2*time.Second, nil) + if err != nil || code != 0 || out != "RECOVERED" { + t.Fatalf("replacement shell result = (%q, %d, %v)", out, code, err) + } +} + // Commands like `adb shell …` inherit the persistent shell's stdin pipe. Because // that pipe stays open between tool calls, they block reading (or steal the // completion sentinel). Wrapping the user command so its stdin is /dev/null diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index 0dcfefd..2e188d8 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -2052,27 +2052,59 @@ export function StreamingIndicator({ ) } -// Memoised for the same reason as ToolCallCard: on a message that grows to many -// segments during one streaming turn, only the changed segment should re-render. -// `text` is a primitive, so memo compares by value and finished blocks are free. +/** + * Collapsible model-thinking block. + * + * Must NOT run the chat Markdown renderer on expand: reasoning traces are long + * (tens of KB of decompiler/code-like text with many `*`/`[]`), and turning + * that into hundreds of React nodes freezes the tab ("Page Unresponsive"). + * Plain pre-wrap text in a height-capped scroller is one DOM node, cheap to + * open, and matches how thinking logs are meant to be read. + * + * Memoised for the same reason as ToolCallCard: on a message that grows to many + * segments during one streaming turn, only the changed segment should re-render. + * `text` is a primitive, so memo compares by value and finished blocks are free. + */ const ReasoningBlock = memo(function ReasoningBlock({ text }: { text: string }) { const { t } = useI18n() const [open, setOpen] = useState(false) - // A slim inline toggle rather than a boxed card: collapsed reasoning should - // barely take a line, expanding into a quiet left-ruled block when opened. + // Defer mounting the body to the next frame so the click paints first and + // Chrome does not treat the expand as a long task on the same turn. + const [bodyReady, setBodyReady] = useState(false) + useEffect(() => { + if (!open) { + setBodyReady(false) + return + } + const id = requestAnimationFrame(() => setBodyReady(true)) + return () => cancelAnimationFrame(id) + }, [open]) + return (
{open ? ( -
- +
+ {bodyReady ? ( + 
+              {text}
+
+ ) : ( +

…

+ )}
) : null}