diff --git a/agent/agentfiles/files.go b/agent/agentfiles/files.go index 0af5d6e5f7..868b4e5fb1 100644 --- a/agent/agentfiles/files.go +++ b/agent/agentfiles/files.go @@ -13,6 +13,7 @@ import ( "strings" "syscall" + "github.com/aymanbagabas/go-udiff" "github.com/google/uuid" "golang.org/x/xerrors" @@ -44,7 +45,16 @@ type HTTPResponseCode = int // pendingEdit holds the computed result of a file edit, ready to // be written to disk. type pendingEdit struct { - path string + // origPath is the caller-supplied path, pre-symlink-resolution. + // Used for response labels so the caller can match responses to + // their original requests. + origPath string + // path is the symlink-resolved path; what actually gets written. + path string + // oldContent is the file content before edits were applied. Used + // for diff computation when the request asked for diffs. + oldContent string + // content is the file content after all edits. content string mode os.FileMode } @@ -375,6 +385,37 @@ func (api *API) HandleEditFiles(rw http.ResponseWriter, r *http.Request) { return } + // Duplicate entries both read the same file and race to write; + // the first entry's edits are silently lost. Resolve symlinks + // before comparing so two paths that alias the same real file + // (e.g. one via a symlink, one direct) don't slip past as + // distinct keys. prepareFileEdit resolves the path again for + // its own use; the double lstat cost is cheap compared to the + // data-loss risk of silent aliasing. + type seenEntry struct { + caller string + } + seenPaths := make(map[string]seenEntry, len(req.Files)) + for _, f := range req.Files { + // On resolve error, use the raw path; phase 1 surfaces + // the error with its proper status code. + key := f.Path + if resolved, err := api.resolvePath(f.Path); err == nil { + key = resolved + } + if prev, dup := seenPaths[key]; dup { + msg := fmt.Sprintf("duplicate file path %q: combine edits into a single entry's \"edits\" list", f.Path) + if prev.caller != f.Path { + msg = fmt.Sprintf("duplicate file path %q aliases %q (same real file): combine edits into a single entry's \"edits\" list", f.Path, prev.caller) + } + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: msg, + }) + return + } + seenPaths[key] = seenEntry{caller: f.Path} + } + // Phase 1: compute all edits in memory. If any file fails // (bad path, search miss, permission error), bail before // writing anything. @@ -426,9 +467,29 @@ func (api *API) HandleEditFiles(rw http.ResponseWriter, r *http.Request) { } } - httpapi.Write(ctx, rw, http.StatusOK, codersdk.Response{ - Message: "Successfully edited file(s)", - }) + resp := workspacesdk.FileEditResponse{} + if req.IncludeDiff { + resp.Files = make([]workspacesdk.FileEditResult, 0, len(pending)) + for _, p := range pending { + // udiff.Unified calls log.Fatalf on its internal error, + // which would kill the agent process. Route through + // Lines + ToUnified so a library bug yields an empty + // diff plus a log line instead. + edits := udiff.Lines(p.oldContent, p.content) + diff, err := udiff.ToUnified(p.origPath, p.origPath, p.oldContent, edits, udiff.DefaultContextLines) + if err != nil { + api.logger.Warn(ctx, "unified diff computation failed", + slog.F("path", p.origPath), + slog.Error(err)) + diff = "" + } + resp.Files = append(resp.Files, workspacesdk.FileEditResult{ + Path: p.origPath, + Diff: diff, + }) + } + } + httpapi.Write(ctx, rw, http.StatusOK, resp) } // prepareFileEdit validates, reads, and computes edits for a single @@ -450,6 +511,7 @@ func (api *API) prepareFileEdit(path string, edits []workspacesdk.FileEdit) (int if err != nil { return http.StatusInternalServerError, nil, xerrors.Errorf("resolve symlink %q: %w", path, err) } + origPath := path path = resolved f, err := api.filesystem.Open(path) @@ -479,6 +541,7 @@ func (api *API) prepareFileEdit(path string, edits []workspacesdk.FileEdit) (int return http.StatusInternalServerError, nil, xerrors.Errorf("read %s: %w", path, err) } content := string(data) + oldContent := content for _, edit := range edits { var err error @@ -489,9 +552,11 @@ func (api *API) prepareFileEdit(path string, edits []workspacesdk.FileEdit) (int } return 0, &pendingEdit{ - path: path, - content: content, - mode: stat.Mode(), + origPath: origPath, + path: path, + oldContent: oldContent, + content: content, + mode: stat.Mode(), }, nil } @@ -555,6 +620,458 @@ func (api *API) atomicWrite(ctx context.Context, path string, mode *os.FileMode, return 0, nil } +// splitEnding separates a line produced by strings.SplitAfter(s, +// "\n") into its content bytes and its line ending. The ending is +// one of "\r\n", "\n", or "" (the last slice when the input lacks a +// trailing newline). +func splitEnding(line string) (content, ending string) { + if strings.HasSuffix(line, "\r\n") { + return line[:len(line)-2], "\r\n" + } + if strings.HasSuffix(line, "\n") { + return line[:len(line)-1], "\n" + } + return line, "" +} + +// endingsMatch decides whether two line endings may pair up during +// fuzzy matching. Identical endings always match. "\n" and "\r\n" +// interchange so LLMs can send LF searches against CRLF content. +// An empty ending (EOF, no terminator) acts as a wildcard and +// matches any ending, which lets the splice later substitute the +// file's actual ending in place of a missing one. +func endingsMatch(a, b string) bool { + // Wildcard: empty ending matches any ending at the matching + // phase. Only valid here, not at the splice phase. + if a == "" || b == "" { + return true + } + if a == b { + return true + } + return isNewlineEnding(a) && isNewlineEnding(b) +} + +// isNewlineEnding reports whether s is one of the newline-class +// endings: "\n" or "\r\n". Shared primitive for endingsMatch +// (matching phase) and endingShapeEqual (splice phase) so a new +// ending class added in one predicate can't silently diverge from +// the other. +func isNewlineEnding(s string) bool { + return s == "\n" || s == "\r\n" +} + +// internalLineEnding returns the shared line ending used across +// lines. An unterminated last line (EOF-no-newline) is excluded. +// Returns ("", false) if any non-last line has no ending, or if +// endings disagree. +func internalLineEnding(lines []string) (string, bool) { + if len(lines) < 2 { + return "", false + } + var want string + for i, l := range lines { + isLast := i == len(lines)-1 + _, e := splitEnding(l) + if isLast && e == "" { + continue + } + if e == "" { + return "", false + } + if want == "" { + want = e + continue + } + if e != want { + return "", false + } + } + return want, want != "" +} + +// dominantFileEnding returns CRLF if CRLF endings outnumber LF in +// contentLines, LF otherwise (including ties and ending-less files). +func dominantFileEnding(contentLines []string) string { + var crlf, lf int + for _, l := range contentLines { + switch { + case strings.HasSuffix(l, "\r\n"): + crlf++ + case strings.HasSuffix(l, "\n"): + lf++ + } + } + if crlf > lf { + return "\r\n" + } + return "\n" +} + +// atNoNewlineEOF reports whether the matched region ends at a +// file that lacks a trailing newline. True when no non-empty lines +// follow the match and the last matched line has no ending. +func atNoNewlineEOF(contentLines []string, end int) bool { + if end == 0 { + return false + } + if end < len(contentLines) { + // Anything non-empty after the match disqualifies. + for _, l := range contentLines[end:] { + if l != "" { + return false + } + } + } + // Last matched content line must itself have no ending. + _, e := splitEnding(contentLines[end-1]) + return e == "" +} + +// leadOnly returns the leading whitespace of line (spaces and +// tabs only), excluding the ending. +func leadOnly(line string) string { + //nolint:dogsled // splitLineParts is the shared decomposer; other parts are genuinely unused here. + lead, _, _, _ := splitLineParts(line) + return lead +} + +// alignSearchReplace returns the count of leading and trailing +// lines that match between searchLines and repLines under +// TrimSpace equality. Between the prefix and suffix ranges lies +// the middle: inserted, deleted, or rewritten lines. TrimSpace +// matches what pass 3 uses for matching, so pair identification +// stays consistent with how the region was found. +func alignSearchReplace(searchLines, repLines []string) (prefix, suffix int) { + eq := func(a, b string) bool { + aContent, _ := splitEnding(a) + bContent, _ := splitEnding(b) + return strings.TrimSpace(aContent) == strings.TrimSpace(bContent) + } + maxPrefix := len(searchLines) + if len(repLines) < maxPrefix { + maxPrefix = len(repLines) + } + for prefix < maxPrefix && eq(searchLines[prefix], repLines[prefix]) { + prefix++ + } + // Suffix must not overlap prefix on either side. + maxSuffix := maxPrefix - prefix + for suffix < maxSuffix && + eq(searchLines[len(searchLines)-1-suffix], repLines[len(repLines)-1-suffix]) { + suffix++ + } + return prefix, suffix +} + +// detectIndentUnit scans leading whitespace across the given lines +// and returns the smallest consistent indentation unit (one tab, or +// N spaces where N is the GCD of observed non-zero lead lengths). +// Returns ("", false) when no useful unit can be detected: no lines +// have indent, indents mix tabs and spaces, or the GCD is zero. +// +// Tabs take priority: any tab-indented line forces unit="\t" and any +// space-only indent on another line marks the sample as mixed. +func detectIndentUnit(lines []string) (string, bool) { + sawTab := false + sawSpace := false + var spaceGCD int + for _, l := range lines { + lead, mid, _, _ := splitLineParts(l) + // Skip body-less lines: a blank line or a line with only + // trailing whitespace has no indent signal. Otherwise a + // 2sp whitespace-only line on a 4sp file would corrupt + // the GCD down to 2sp and emit the wrong unit. + if lead == "" || mid == "" { + continue + } + switch { + case strings.HasPrefix(lead, "\t") && !strings.ContainsAny(lead, " "): + sawTab = true + case !strings.ContainsAny(lead, "\t"): + sawSpace = true + if spaceGCD == 0 { + spaceGCD = len(lead) + } else { + spaceGCD = indentGCD(spaceGCD, len(lead)) + } + default: + // Mixed tab+space in a single lead; bail. + return "", false + } + } + if sawTab && sawSpace { + return "", false + } + if sawTab { + return "\t", true + } + if spaceGCD > 0 { + return strings.Repeat(" ", spaceGCD), true + } + return "", false +} + +// indentGCD returns the greatest common divisor of a and b. Used +// only by detectIndentUnit on positive space-lead lengths. +func indentGCD(a, b int) int { + for b != 0 { + a, b = b, a%b + } + return a +} + +// translateIndentLevel returns the file-side lead for an inserted +// splice line by translating the caller's indent level. rLead is +// the inserted replacement line's lead, sLead is the reference +// search line's lead (the pair the splice would have inherited +// from), cLead is the matched content's lead at that same +// reference slot. Returns ("", false) when any of the leads are +// not clean multiples of their respective units. +func translateIndentLevel(rLead, sLead, cLead, searchUnit, fileUnit string) (string, bool) { + repLevel, ok := indentLevel(rLead, searchUnit) + if !ok { + return "", false + } + searchBase, ok := indentLevel(sLead, searchUnit) + if !ok { + return "", false + } + fileBase, ok := indentLevel(cLead, fileUnit) + if !ok { + return "", false + } + targetLevel := fileBase + (repLevel - searchBase) + if targetLevel < 0 { + return "", false + } + return strings.Repeat(fileUnit, targetLevel), true +} + +// indentLevel returns len(lead) / len(unit) when lead is a clean +// multiple of unit. Returns (0, false) when lead doesn't divide +// evenly by unit. Callers must ensure unit is non-empty; +// detectIndentUnit's second return gates this. +func indentLevel(lead, unit string) (int, bool) { + if len(lead)%len(unit) != 0 { + return 0, false + } + // Verify the lead is actually composed of repetitions of unit. + if strings.Repeat(unit, len(lead)/len(unit)) != lead { + return 0, false + } + return len(lead) / len(unit), true +} + +// non-last line's ending replaced by ending; the last line keeps +// its original ending. Used before pass 1 splicing to normalize +// the replacement to the file's ending style. +func rewriteInternalEnding(lines []string, ending string) string { + var b strings.Builder + for i, l := range lines { + body, e := splitEnding(l) + _, _ = b.WriteString(body) + isLast := i == len(lines)-1 + switch { + case isLast: + _, _ = b.WriteString(e) + case e == "": + // Non-last line without ending is only legal at EOF; + // leave the caller's shape alone. + default: + _, _ = b.WriteString(ending) + } + } + return b.String() +} + +// splitLineParts decomposes a line into its leading whitespace +// (spaces and tabs only), middle body, trailing whitespace +// (spaces and tabs only), and line ending. Used by the fuzzy +// splice to substitute the file's whitespace at each position +// when search and replace agree on what that position should be. +func splitLineParts(line string) (lead, middle, trail, ending string) { + body, ending := splitEnding(line) + i := 0 + for i < len(body) && (body[i] == ' ' || body[i] == '\t') { + i++ + } + lead = body[:i] + rest := body[i:] + j := len(rest) + for j > 0 && (rest[j-1] == ' ' || rest[j-1] == '\t') { + j-- + } + middle = rest[:j] + trail = rest[j:] + return lead, middle, trail, ending +} + +// endingShapeEqual reports whether two line endings occupy the +// same "position class" for the splice substitution: both empty, +// or both in the newline class ({"\n", "\r\n"}). When this is +// true and the pair matched during matching, the splice uses the +// file's ending. When false, the splice keeps the replacement's +// ending verbatim (the caller is signaling an intentional fold +// or split). Unlike endingsMatch, empty is not a wildcard here: +// the splice phase needs a strict "same class" test so interior +// lines don't silently pick up a missing EOF terminator from the +// reference content. +func endingShapeEqual(a, b string) bool { + if a == b { + return true + } + return isNewlineEnding(a) && isNewlineEnding(b) +} + +// buildReplacementLines emits the splice for a fuzzy match by +// per-position substitution at leading-ws, body, trailing-ws, and +// ending. Search and replace agreement at a position -> file's +// bytes win; disagreement -> replacement's bytes are spliced. +// Extra replace lines past the matched region reference the last +// search/content line. +// +// Carve-outs on "file wins on agreement": +// - Empty replacement body: emit the replacement's whitespace +// verbatim so a body-less line doesn't materialize whitespace. +// - Reference content line has no ending and this isn't the +// final replacement line: keep the replacement's newline so a +// multi-line splice at EOF doesn't collapse. +// - Inserted lines (no paired search line) try level-aware +// indent translation: if we can detect both the caller's +// search_unit and the file's fileUnit cleanly, the emitted +// lead is fileUnit * (file_base + (rep_level - search_base)). +// The caller's rep_level is computed from their own indent +// style; output in the file's style so a 4sp LLM inserting +// into a 2sp file emits 2sp indent at the correct depth. If +// detection fails (no indent info, mixed tabs+spaces, or +// a non-unit multiple), fall back to inheriting cLead. +// +// forcedEnding (from internalLineEnding normalization) overrides +// interior endings; the final ending is forced too unless +// atNoNewlineEOF (preserving the file's no-terminator EOF). +// When atNoNewlineEOF is false and the final ending would still +// be empty, force a terminator so unmatched content doesn't +// concatenate onto the splice. +// +// len(matched) == len(searchLines) is the invariant; callers +// slice contentLines before invoking. +// +//nolint:revive // atNoNewlineEOF is a computed match property, not caller control coupling. +func buildReplacementLines(matched, searchLines []string, replace, forcedEnding string, atNoNewlineEOF bool) string { + repLines := strings.SplitAfter(replace, "\n") + // SplitAfter on a string ending in "\n" yields a trailing empty + // element. Drop it so it doesn't pair with a phantom line. + if len(repLines) > 0 && repLines[len(repLines)-1] == "" { + repLines = repLines[:len(repLines)-1] + } + prefix, suffix := alignSearchReplace(searchLines, repLines) + + // Combine search and replace so a zero-width search still + // informs the unit from the replacement's inserted depths. + // Fallback for detection failure lives in the inserted branch. + searchUnit, searchUnitOK := detectIndentUnit(append(append([]string(nil), searchLines...), repLines...)) + fileUnit, fileUnitOK := detectIndentUnit(matched) + var b strings.Builder + for i, rLine := range repLines { + var refIdx int + inserted := false + searchMiddleLen := len(searchLines) - prefix - suffix + switch { + case i < prefix: + refIdx = i + case i >= len(repLines)-suffix: + refIdx = i - (len(repLines) - len(searchLines)) + case i-prefix < searchMiddleLen: + refIdx = prefix + (i - prefix) + default: + // Pure insertion: pick the reference content line by + // the caller's indent signal. An inserted line whose + // lead matches the suffix's first rep line belongs to + // the suffix scope; one matching the prefix's last rep + // line belongs to the prefix scope. Fall back to + // suffix, then prefix, then i-clamped. + inserted = true + rLeadForI := leadOnly(rLine) + switch { + case prefix > 0 && suffix > 0: + prefixRLead := leadOnly(repLines[prefix-1]) + suffixRLead := leadOnly(repLines[len(repLines)-suffix]) + switch { + case rLeadForI == suffixRLead: + refIdx = len(searchLines) - suffix + case rLeadForI == prefixRLead: + refIdx = prefix - 1 + default: + refIdx = len(searchLines) - suffix + } + case suffix > 0: + refIdx = len(searchLines) - suffix + case prefix > 0: + refIdx = prefix - 1 + default: + refIdx = min(i, len(searchLines)-1) + } + } + refContent := matched[refIdx] + sLead, _, sTrail, sEnd := splitLineParts(searchLines[refIdx]) + rLead, rMid, rTrail, rEnd := splitLineParts(rLine) + cLead, _, cTrail, cEnd := splitLineParts(refContent) + + lead := rLead + trail := rTrail + switch { + case rMid == "": + // Body-less: emit the replacement's whitespace verbatim. + case inserted: + // Translate the caller's indent level to the file's + // unit; fall back to cLead when detection fails. + lead = cLead + if searchUnitOK && fileUnitOK { + if translated, ok := translateIndentLevel(rLead, sLead, cLead, searchUnit, fileUnit); ok { + lead = translated + } + } + default: + if sLead == rLead { + lead = cLead + } + if sTrail == rTrail { + trail = cTrail + } + } + ending := rEnd + if !inserted && endingShapeEqual(sEnd, rEnd) { + ending = cEnd + // Interior lines keep their newline when the reference + // content has cEnd="" (no-EOL EOF); only the final + // output line may inherit the empty ending. + if cEnd == "" && i < len(repLines)-1 { + ending = rEnd + } + } + if inserted && i == len(repLines)-1 && atNoNewlineEOF { + ending = "" + } + if forcedEnding != "" && (i < len(repLines)-1 || !atNoNewlineEOF) { + ending = forcedEnding + } + if i == len(repLines)-1 && !atNoNewlineEOF && ending == "" { + if forcedEnding != "" { + ending = forcedEnding + } else { + ending = "\n" + } + } + + _, _ = b.WriteString(lead) + _, _ = b.WriteString(rMid) + _, _ = b.WriteString(trail) + _, _ = b.WriteString(ending) + } + return b.String() +} + // fuzzyReplace attempts to find `search` inside `content` and replace it // with `replace`. It uses a cascading match strategy inspired by // openai/codex's apply_patch: @@ -569,17 +1086,67 @@ func (api *API) atomicWrite(ctx context.Context, path string, mode *os.FileMode, // is returned asking the caller to include more context or set // replace_all. // -// When a fuzzy match is found (passes 2 or 3), the replacement is still -// applied at the byte offsets of the original content so that surrounding -// text (including indentation of untouched lines) is preserved. +// When a fuzzy match is found (passes 2 or 3), buildReplacementLines +// emits the spliced output by per-position substitution at +// leading-whitespace, body, trailing-whitespace, and ending: where +// search and replace agree at a position, the file's bytes win. This +// preserves surrounding text (including indentation of untouched +// lines) while letting the caller drive deliberate rewrites of +// leading whitespace or endings. func fuzzyReplace(content string, edit workspacesdk.FileEdit) (string, error) { search := edit.Search replace := edit.Replace - // Pass 1 – exact substring match. + // An empty search string has no meaningful interpretation: it + // matches at every byte position, which means the caller has not + // told us what they want to replace. Reject explicitly so + // replace_all=true can't silently inject the replacement between + // every byte. + if search == "" { + return "", xerrors.New("search string must not be empty; include the " + + "text you want to match") + } + + // Split up front so the ending-normalization rule can inspect + // all three before any matching pass. + contentLines := strings.SplitAfter(content, "\n") + searchLines := strings.SplitAfter(search, "\n") + // A trailing newline in the search produces an empty final element + // from SplitAfter. Drop it so it doesn't interfere with line + // matching. + if len(searchLines) > 0 && searchLines[len(searchLines)-1] == "" { + searchLines = searchLines[:len(searchLines)-1] + } + replaceLines := strings.SplitAfter(replace, "\n") + if len(replaceLines) > 0 && replaceLines[len(replaceLines)-1] == "" { + replaceLines = replaceLines[:len(replaceLines)-1] + } + + // Ending normalization. If replace has a consistent internal + // ending, force every spliced interior line to the file's + // dominant ending. If search also has a consistent internal + // ending and it disagrees with replace's, the caller signaled + // intent to rewrite endings; restrict the match to pass 1 so + // CRLF/LF interchange at pass 2 can't silently bridge a search + // that doesn't actually occur in the file. + var forcedEnding string + searchInternal, searchOK := internalLineEnding(searchLines) + replaceInternal, replaceOK := internalLineEnding(replaceLines) + if replaceOK { + forcedEnding = dominantFileEnding(contentLines) + } + callerEndingIntent := searchOK && replaceOK && searchInternal != replaceInternal + + // Pass 1 - exact substring match. Normalize replace's interior + // endings to the file's style unless the caller's search/replace + // disagreement signaled intent to rewrite endings. + pass1Replace := replace + if forcedEnding != "" && !callerEndingIntent && replaceInternal != forcedEnding { + pass1Replace = rewriteInternalEnding(replaceLines, forcedEnding) + } if strings.Contains(content, search) { if edit.ReplaceAll { - return strings.ReplaceAll(content, search, replace), nil + return strings.ReplaceAll(content, search, pass1Replace), nil } count := strings.Count(content, search) if count > 1 { @@ -589,37 +1156,39 @@ func fuzzyReplace(content string, edit workspacesdk.FileEdit) (string, error) { "replace_all to true", count) } // Exactly one match. - return strings.Replace(content, search, replace, 1), nil + return strings.Replace(content, search, pass1Replace, 1), nil } - // For line-level fuzzy matching we split both content and search - // into lines. - contentLines := strings.SplitAfter(content, "\n") - searchLines := strings.SplitAfter(search, "\n") - - // A trailing newline in the search produces an empty final element - // from SplitAfter. Drop it so it doesn't interfere with line - // matching. - if len(searchLines) > 0 && searchLines[len(searchLines)-1] == "" { - searchLines = searchLines[:len(searchLines)-1] + if callerEndingIntent { + // Intent signaled but pass 1 missed; reject rather than let + // pass 2's CRLF/LF interchange bridge a mismatched search. + return "", xerrors.New("search string not found in file. Verify the search " + + "string matches the file content exactly, including whitespace, " + + "indentation, and line endings") } trimRight := func(a, b string) bool { - return strings.TrimRight(a, " \t\r\n") == strings.TrimRight(b, " \t\r\n") + aContent, aEnding := splitEnding(a) + bContent, bEnding := splitEnding(b) + return endingsMatch(aEnding, bEnding) && + strings.TrimRight(aContent, " \t") == strings.TrimRight(bContent, " \t") } trimAll := func(a, b string) bool { - return strings.TrimSpace(a) == strings.TrimSpace(b) + aContent, aEnding := splitEnding(a) + bContent, bEnding := splitEnding(b) + return endingsMatch(aEnding, bEnding) && + strings.TrimSpace(aContent) == strings.TrimSpace(bContent) } // Pass 2 – trim trailing whitespace on each line. - if result, matched, err := fuzzyReplaceLines(contentLines, searchLines, replace, trimRight, edit.ReplaceAll); matched { + if result, matched, err := fuzzyReplaceLines(contentLines, searchLines, replace, trimRight, edit.ReplaceAll, forcedEnding); matched { return result, err } // Pass 3 – trim all leading and trailing whitespace // (indentation-tolerant). The replacement is inserted verbatim; // callers must provide correctly indented replacement text. - if result, matched, err := fuzzyReplaceLines(contentLines, searchLines, replace, trimAll, edit.ReplaceAll); matched { + if result, matched, err := fuzzyReplaceLines(contentLines, searchLines, replace, trimAll, edit.ReplaceAll, forcedEnding); matched { return result, err } @@ -670,20 +1239,6 @@ outer: return count } -// spliceLines replaces contentLines[start:end] with replacement text, returning -// the full content as a single string. -func spliceLines(contentLines []string, start, end int, replacement string) string { - var b strings.Builder - for _, l := range contentLines[:start] { - _, _ = b.WriteString(l) - } - _, _ = b.WriteString(replacement) - for _, l := range contentLines[end:] { - _, _ = b.WriteString(l) - } - return b.String() -} - // fuzzyReplaceLines handles fuzzy matching passes (2 and 3) for // fuzzyReplace. When replaceAll is false and there are multiple // matches, an error is returned. When replaceAll is true, all @@ -699,6 +1254,7 @@ func fuzzyReplaceLines( replace string, eq func(a, b string) bool, replaceAll bool, + forcedEnding string, ) (string, bool, error) { start, end, ok := seekLines(contentLines, searchLines, eq) if !ok { @@ -712,11 +1268,22 @@ func fuzzyReplaceLines( "context to make the match unique, or set "+ "replace_all to true", count) } - return spliceLines(contentLines, start, end, replace), true, nil + var b strings.Builder + for _, l := range contentLines[:start] { + _, _ = b.WriteString(l) + } + _, _ = b.WriteString(buildReplacementLines(contentLines[start:end], searchLines, replace, forcedEnding, atNoNewlineEOF(contentLines, end))) + for _, l := range contentLines[end:] { + _, _ = b.WriteString(l) + } + return b.String(), true, nil } - // Replace all: collect all match positions, then apply from last - // to first to preserve indices. + // Replace all: collect all match positions, then emit the + // output forward, interleaving unmatched spans with spliced + // replacements. Each match runs through the same per-position + // splice as single-replace, using its own matched content + // slice as the reference. type lineMatch struct{ start, end int } var matches []lineMatch for i := 0; i <= len(contentLines)-len(searchLines); { @@ -735,19 +1302,16 @@ func fuzzyReplaceLines( } } - // Apply replacements from last to first. - repLines := strings.SplitAfter(replace, "\n") - for i := len(matches) - 1; i >= 0; i-- { - m := matches[i] - newLines := make([]string, 0, m.start+len(repLines)+(len(contentLines)-m.end)) - newLines = append(newLines, contentLines[:m.start]...) - newLines = append(newLines, repLines...) - newLines = append(newLines, contentLines[m.end:]...) - contentLines = newLines - } - var b strings.Builder - for _, l := range contentLines { + prev := 0 + for _, m := range matches { + for _, l := range contentLines[prev:m.start] { + _, _ = b.WriteString(l) + } + _, _ = b.WriteString(buildReplacementLines(contentLines[m.start:m.end], searchLines, replace, forcedEnding, atNoNewlineEOF(contentLines, m.end))) + prev = m.end + } + for _, l := range contentLines[prev:] { _, _ = b.WriteString(l) } return b.String(), true, nil diff --git a/agent/agentfiles/files_indent_internal_test.go b/agent/agentfiles/files_indent_internal_test.go new file mode 100644 index 0000000000..78212c578e --- /dev/null +++ b/agent/agentfiles/files_indent_internal_test.go @@ -0,0 +1,298 @@ +package agentfiles + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// Direct unit tests for the indent-splice helpers. These test the +// functions in isolation so a helper bug surfaces here with a +// descriptive failure instead of as a rendered-file mismatch deep +// in an integration test. + +func TestDetectIndentUnit(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + lines []string + wantUnit string + wantOK bool + }{ + { + name: "Empty", + lines: nil, + wantUnit: "", + wantOK: false, + }, + { + name: "NoIndent", + lines: []string{"foo\n", "bar\n"}, + wantUnit: "", + wantOK: false, + }, + { + name: "TabOnly", + lines: []string{"\tfoo\n", "\t\tbar\n"}, + wantUnit: "\t", + wantOK: true, + }, + { + name: "FourSpaceUniform", + lines: []string{" foo\n", " bar\n"}, + wantUnit: " ", + wantOK: true, + }, + { + name: "TwoSpaceUniform", + lines: []string{" foo\n", " bar\n"}, + wantUnit: " ", + wantOK: true, + }, + { + name: "GCDReducesFourAndSixToTwo", + lines: []string{" foo\n", " bar\n"}, + wantUnit: " ", + wantOK: true, + }, + { + name: "MixedAcrossLinesTabAndSpace", + lines: []string{"\tfoo\n", " bar\n"}, + wantUnit: "", + wantOK: false, + }, + { + name: "MixedWithinLeadTabThenSpace", + lines: []string{"\t foo\n"}, + wantUnit: "", + wantOK: false, + }, + { + name: "MixedWithinLeadSpaceThenTab", + lines: []string{" \tfoo\n"}, + wantUnit: "", + wantOK: false, + }, + { + // DEREM-33 regression: a 2sp whitespace-only line in + // a 4sp-indented region must not pull the GCD down. + name: "WhitespaceOnlyLineSkipped", + lines: []string{" foo\n", " \n", " bar\n"}, + wantUnit: " ", + wantOK: true, + }, + { + name: "OnlyWhitespaceOnlyLines", + lines: []string{" \n", " \n"}, + wantUnit: "", + wantOK: false, + }, + { + name: "BlankLineIgnored", + lines: []string{"\n", " foo\n"}, + wantUnit: " ", + wantOK: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + gotUnit, gotOK := detectIndentUnit(tc.lines) + require.Equal(t, tc.wantUnit, gotUnit) + require.Equal(t, tc.wantOK, gotOK) + }) + } +} + +func TestIndentGCD(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + a, b int + want int + }{ + {"BothZero", 0, 0, 0}, + {"AZero", 0, 4, 4}, + {"BZero", 4, 0, 4}, + {"Equal", 4, 4, 4}, + {"Coprime", 3, 5, 1}, + {"CommonFactorTwo", 4, 6, 2}, + {"CommonFactorFour", 8, 12, 4}, + {"TwoSpaceAndFourSpace", 2, 4, 2}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tc.want, indentGCD(tc.a, tc.b)) + }) + } +} + +func TestIndentLevel(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + lead string + unit string + wantLevel int + wantOK bool + }{ + { + name: "EmptyLead", + lead: "", + unit: " ", + wantLevel: 0, + wantOK: true, + }, + { + name: "CleanMultipleOne", + lead: " ", + unit: " ", + wantLevel: 1, + wantOK: true, + }, + { + name: "CleanMultipleThreeTwoSp", + lead: " ", + unit: " ", + wantLevel: 3, + wantOK: true, + }, + { + name: "CleanMultipleTwoTab", + lead: "\t\t", + unit: "\t", + wantLevel: 2, + wantOK: true, + }, + { + name: "NonMultipleLength", + lead: " ", + unit: " ", + wantLevel: 0, + wantOK: false, + }, + { + // Even when the length divides evenly, the lead must + // be composed of repetitions of the unit. + name: "LengthDividesButCompositionMismatches", + lead: "\t ", + unit: " ", + wantLevel: 0, + wantOK: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + gotLevel, gotOK := indentLevel(tc.lead, tc.unit) + require.Equal(t, tc.wantLevel, gotLevel) + require.Equal(t, tc.wantOK, gotOK) + }) + } +} + +func TestTranslateIndentLevel(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rLead string + sLead string + cLead string + searchUnit string + fileUnit string + want string + wantOK bool + }{ + { + // Caller sends a 4sp search; inserted line is 8sp + // (one level deeper). File uses tabs, matched at + // 1-tab depth. Expected: 2 tabs. + name: "PositiveDeltaWrap", + rLead: " ", + sLead: " ", + cLead: "\t", + searchUnit: " ", + fileUnit: "\t", + want: "\t\t", + wantOK: true, + }, + { + // Inserted line at the same level as its reference. + name: "ZeroDeltaSameLevel", + rLead: " ", + sLead: " ", + cLead: "\t", + searchUnit: " ", + fileUnit: "\t", + want: "\t", + wantOK: true, + }, + { + // Inserted line shallower than the reference's + // level by more than the file_base: target goes + // negative, helper bails. + name: "NegativeDeltaBelowFileBase", + rLead: "", + sLead: " ", + cLead: "\t", + searchUnit: " ", + fileUnit: "\t", + want: "", + wantOK: false, + }, + { + // Malformed rLead (3 spaces under a 4sp unit). + name: "MalformedRLead", + rLead: " ", + sLead: " ", + cLead: "\t", + searchUnit: " ", + fileUnit: "\t", + want: "", + wantOK: false, + }, + { + // 4sp LLM into a 2sp file at matched-4sp baseline. + // rep_level=2, search_base=1, file_base=2, + // target=3, emit " " (6sp). + name: "CrossStyle4spTo2sp", + rLead: " ", + sLead: " ", + cLead: " ", + searchUnit: " ", + fileUnit: " ", + want: " ", + wantOK: true, + }, + { + // 2sp LLM into a tab file. + // rep_level=2, search_base=1, file_base=1, + // target=2, emit "\t\t". + name: "CrossStyle2spToTab", + rLead: " ", + sLead: " ", + cLead: "\t", + searchUnit: " ", + fileUnit: "\t", + want: "\t\t", + wantOK: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, gotOK := translateIndentLevel(tc.rLead, tc.sLead, tc.cLead, tc.searchUnit, tc.fileUnit) + require.Equal(t, tc.want, got) + require.Equal(t, tc.wantOK, gotOK) + }) + } +} diff --git a/agent/agentfiles/files_test.go b/agent/agentfiles/files_test.go index 10908c17d0..cc0df0c96a 100644 --- a/agent/agentfiles/files_test.go +++ b/agent/agentfiles/files_test.go @@ -775,7 +775,15 @@ func TestEditFiles(t *testing.T) { }, }, }, - expected: map[string]string{filepath.Join(tmpdir, "trailing-ws"): "replaced"}, + // The file's trailing whitespace (" " on line 1, + // "\t\t" on line 2) agrees with both search and replace + // (both have no trailing whitespace on their single + // lines), so the splice preserves the file's trailing + // whitespace. File's trailing whitespace on line 1 is + // preserved; the replacement collapses to one line, so + // lines 2 and 3 are consumed and only the first line's + // trailing whitespace remains. + expected: map[string]string{filepath.Join(tmpdir, "trailing-ws"): "replaced "}, }, { name: "TabsVsSpaces", @@ -897,7 +905,11 @@ func TestEditFiles(t *testing.T) { }, }, }, - expected: map[string]string{filepath.Join(tmpdir, "ra-fuzzy-trail"): "bye\nworld\nbye\nagain"}, + // File trailing whitespace " " on "hello " lines is + // preserved because search and replace agree on having + // no trailing whitespace. Replace-all runs the same + // per-position splice as single-replace. + expected: map[string]string{filepath.Join(tmpdir, "ra-fuzzy-trail"): "bye \nworld\nbye \nagain"}, }, { // replace_all with fuzzy indent match (pass 3). @@ -1572,3 +1584,1601 @@ func TestEditFiles_FollowsSymlinks(t *testing.T) { require.NoError(t, err) require.Equal(t, "goodbye world", string(data)) } + +func TestEditFiles_FileResults(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + + t.Run("DiffRequestedSingleFile", func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "diff-single") + require.NoError(t, afero.WriteFile(fs, path, []byte("hello world\n"), 0o644)) + + resp := runEditFiles(t, api, workspacesdk.FileEditRequest{ + IncludeDiff: true, + Files: []workspacesdk.FileEdits{ + { + Path: path, + Edits: []workspacesdk.FileEdit{ + {Search: "hello", Replace: "HELLO"}, + }, + }, + }, + }) + require.Len(t, resp.Files, 1) + require.Equal(t, path, resp.Files[0].Path) + // udiff.Unified emits "--- \n+++ \n@@ ...". + require.Contains(t, resp.Files[0].Diff, "--- "+path+"\n") + require.Contains(t, resp.Files[0].Diff, "+++ "+path+"\n") + require.Contains(t, resp.Files[0].Diff, "-hello world") + require.Contains(t, resp.Files[0].Diff, "+HELLO world") + }) + + t.Run("DiffRequestedNoOpEdit", func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "diff-noop") + require.NoError(t, afero.WriteFile(fs, path, []byte("same\n"), 0o644)) + + resp := runEditFiles(t, api, workspacesdk.FileEditRequest{ + IncludeDiff: true, + Files: []workspacesdk.FileEdits{ + { + Path: path, + Edits: []workspacesdk.FileEdit{ + // Replace with identical text (no-op). + {Search: "same", Replace: "same"}, + }, + }, + }, + }) + require.Len(t, resp.Files, 1) + require.Equal(t, path, resp.Files[0].Path) + require.Empty(t, resp.Files[0].Diff, "no-op edit produces empty diff") + }) + + t.Run("DiffNotRequested", func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "diff-off") + require.NoError(t, afero.WriteFile(fs, path, []byte("hello\n"), 0o644)) + + resp := runEditFiles(t, api, workspacesdk.FileEditRequest{ + // IncludeDiff omitted; default false. + Files: []workspacesdk.FileEdits{ + { + Path: path, + Edits: []workspacesdk.FileEdit{ + {Search: "hello", Replace: "HELLO"}, + }, + }, + }, + }) + require.Nil(t, resp.Files, "Files must be nil when IncludeDiff is false") + }) + + t.Run("DiffRequestedMultiFilePreservesOrder", func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + pathA := filepath.Join(tmpdir, "diff-multi-a") + pathB := filepath.Join(tmpdir, "diff-multi-b") + pathC := filepath.Join(tmpdir, "diff-multi-c") + require.NoError(t, afero.WriteFile(fs, pathA, []byte("A\n"), 0o644)) + require.NoError(t, afero.WriteFile(fs, pathB, []byte("B\n"), 0o644)) + require.NoError(t, afero.WriteFile(fs, pathC, []byte("C\n"), 0o644)) + + resp := runEditFiles(t, api, workspacesdk.FileEditRequest{ + IncludeDiff: true, + Files: []workspacesdk.FileEdits{ + {Path: pathA, Edits: []workspacesdk.FileEdit{{Search: "A", Replace: "a"}}}, + {Path: pathB, Edits: []workspacesdk.FileEdit{{Search: "B", Replace: "b"}}}, + {Path: pathC, Edits: []workspacesdk.FileEdit{{Search: "C", Replace: "c"}}}, + }, + }) + require.Len(t, resp.Files, 3) + expected := []struct { + path string + oldLine string + newLine string + }{ + {pathA, "-A", "+a"}, + {pathB, "-B", "+b"}, + {pathC, "-C", "+c"}, + } + for i, want := range expected { + require.Equal(t, want.path, resp.Files[i].Path) + require.NotEmpty(t, resp.Files[i].Diff, "file %d (%s) has empty diff", i, want.path) + require.Contains(t, resp.Files[i].Diff, want.oldLine) + require.Contains(t, resp.Files[i].Diff, want.newLine) + } + }) + + t.Run("DiffRequestedMultiEditSameFile", func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "diff-multi-edit") + require.NoError(t, afero.WriteFile(fs, path, []byte("one\ntwo\nthree\n"), 0o644)) + + resp := runEditFiles(t, api, workspacesdk.FileEditRequest{ + IncludeDiff: true, + Files: []workspacesdk.FileEdits{{ + Path: path, + Edits: []workspacesdk.FileEdit{ + {Search: "one", Replace: "ONE"}, + {Search: "three", Replace: "THREE"}, + }, + }}, + }) + require.Len(t, resp.Files, 1) + require.Equal(t, path, resp.Files[0].Path) + // Both edits must appear in the diff, computed against the + // file's original content (not the post-first-edit content). + require.Contains(t, resp.Files[0].Diff, "-one") + require.Contains(t, resp.Files[0].Diff, "+ONE") + require.Contains(t, resp.Files[0].Diff, "-three") + require.Contains(t, resp.Files[0].Diff, "+THREE") + }) + t.Run("DiffRequestedSymlinkReportsOriginalPath", func(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("symlinks are not reliably supported on Windows") + } + + dir := t.TempDir() + osFs := afero.NewOsFs() + api := agentfiles.NewAPI(logger, osFs, nil) + + realPath := filepath.Join(dir, "real.txt") + require.NoError(t, afero.WriteFile(osFs, realPath, []byte("hello\n"), 0o644)) + + linkPath := filepath.Join(dir, "link.txt") + require.NoError(t, os.Symlink(realPath, linkPath)) + + resp := runEditFiles(t, api, workspacesdk.FileEditRequest{ + IncludeDiff: true, + Files: []workspacesdk.FileEdits{ + { + Path: linkPath, + Edits: []workspacesdk.FileEdit{ + {Search: "hello", Replace: "HELLO"}, + }, + }, + }, + }) + require.Len(t, resp.Files, 1) + // The response must report the caller-supplied path, not the + // symlink-resolved target. + require.Equal(t, linkPath, resp.Files[0].Path) + require.Contains(t, resp.Files[0].Diff, "--- "+linkPath+"\n") + require.Contains(t, resp.Files[0].Diff, "+++ "+linkPath+"\n") + }) +} + +// runEditFiles issues a single POST /edit-files call against api and +// decodes the success body into FileEditResponse. It requires a 200 +// response; tests for error paths should decode the error shape +// directly. +func runEditFiles(t *testing.T, api *agentfiles.API, req workspacesdk.FileEditRequest) workspacesdk.FileEditResponse { + t.Helper() + + ctx := testutil.Context(t, testutil.WaitShort) + + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var resp workspacesdk.FileEditResponse + require.NoError(t, json.NewDecoder(w.Body).Decode(&resp)) + return resp +} + +// TestFuzzyReplace_EndingAndWhitespace exercises the line-endings +// and per-position whitespace behavior of the fuzzy matcher in +// both single-replace and replace-all modes. +// +// Match rule: content and search lines are compared after +// splitting off trailing (pass 2) or surrounding (pass 3) +// whitespace. The line ending is compared separately: identical, +// "\n" and "\r\n" are interchangeable, and an empty ending (EOF, +// no terminator on a line) matches any ending. +// +// Splice rule: for every matched line, the replacement's leading +// whitespace, trailing whitespace, and line ending are substituted +// with the matched content line's equivalents *when search and +// replace agree* at that position. Disagreement at a position +// means the caller wants to change that position explicitly, and +// the replacement's bytes win there. +// +// Pass 1 (byte-literal substring match) is untouched; tests that +// exercise it are noted. +func TestFuzzyReplace_EndingAndWhitespace(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + type edit struct { + search, replace string + replaceAll bool + } + tests := []struct { + name string + content string + edits []edit + expected string + }{ + // CRLF file, LF search: the ending rule lets "line\n" + // match "line\r\n"; the replacement is empty so the + // matched line is removed entirely. + { + name: "CRLF_Content_LFSearch_Delete", + content: "foo\r\nline\r\nbar\r\n", + edits: []edit{{search: "line\n", replace: ""}}, + expected: "foo\r\nbar\r\n", + }, + // Pass 2 tolerates the file's trailing whitespace on + // the matched line when search omits it. Empty + // replacement removes the line. + { + name: "TrailingWhitespace_Delete", + content: "foo\nline \nbar\n", + edits: []edit{{search: "line\n", replace: ""}}, + expected: "foo\nbar\n", + }, + // Pass 1 handles a search without a trailing newline + // when the content contains an exact substring match: + // strings.Replace preserves the surrounding "\n" bytes + // verbatim. + { + name: "Pass1_SearchNoNewline_ExactSubstring", + content: "foo\nfirst line\nbar\n", + edits: []edit{{search: "first line", replace: "LINE"}}, + expected: "foo\nLINE\nbar\n", + }, + // Fuzzy path, both search and replace lack a newline + // ending AND share a trailing space. The empty ending + // on search is a wildcard against content's "\n"; + // pass 2's content comparator ignores the shared + // trailing space to match "key". At splice time, + // search and replace agree on the trailing space so + // the file's lack of trailing whitespace wins; search + // and replace agree on empty ending so the file's + // "\n" wins. + { + name: "FuzzyMatchingWhitespace_FileEndingWins", + content: "foo\nkey\nbar\n", + edits: []edit{{search: "key ", replace: "KEY "}}, + expected: "foo\nKEY\nbar\n", + }, + // Last-line-no-newline uses pass 1 exact match. + { + name: "Pass1_LastLineNoNewline", + content: "foo\nbar", + edits: []edit{{search: "bar", replace: "BAR"}}, + expected: "foo\nBAR", + }, + // Indent-tolerant matching on a CRLF file: search and + // replace disagree with the file on indent, so passes 1 + // and 2 fail; pass 3 (TrimSpace) matches on body. The + // splice then decides each position by whether search + // and replace agree with each other. These three cases + // vary the caller-side whitespace to enumerate the + // mechanism: + // + // - when the caller agrees with itself on leading + // whitespace, the file's tab wins regardless of + // the space count on the caller side; + // - when the caller disagrees with itself (search + // leads with one thing, replace with another), the + // replacement's leading whitespace wins. That's the + // escape hatch for intentional indent rewrites. + // + // Endings always agree (both newline-class), so the + // file's "\r\n" wins at every emitted line. + { + name: "FuzzyIndent_CRLF_TwoSpaceSearch_FileTabWins", + content: "foo\r\n\tline\r\nbar\r\n", + edits: []edit{{search: " line\n", replace: " LINE\n"}}, + expected: "foo\r\n\tLINE\r\nbar\r\n", + }, + { + name: "FuzzyIndent_CRLF_SevenSpaceSearch_FileTabStillWins", + content: "foo\r\n\tline\r\nbar\r\n", + edits: []edit{{search: " line\n", replace: " LINE\n"}}, + expected: "foo\r\n\tLINE\r\nbar\r\n", + }, + { + name: "FuzzyIndent_CRLF_CallerRewritesIndent_ReplaceLeadingWins", + content: "foo\r\n\tline\r\nbar\r\n", + edits: []edit{{search: " line\n", replace: " LINE\n"}}, + expected: "foo\r\n LINE\r\nbar\r\n", + }, + + // Replace-all must run through the same per-position + // splice as single-replace. + { + // Every matched line keeps the file's trailing + // whitespace shape (""), and its "\n" ending. + name: "ReplaceAll_FuzzyMatchingWhitespace_FileEndingWins", + content: "key\nkey\nother\n", + edits: []edit{{search: "key ", replace: "KEY ", replaceAll: true}}, + expected: "KEY\nKEY\nother\n", + }, + { + // CRLF file, LF search/replace: every splice uses + // the file's "\r\n" so the output is uniformly CRLF. + name: "ReplaceAll_CRLF_LFSearch_FileEndingWins", + content: "line one\r\nother\r\nline one\r\n", + edits: []edit{{search: "line one\n", replace: "LINE\n", replaceAll: true}}, + expected: "LINE\r\nother\r\nLINE\r\n", + }, + + // Caller explicitly folds: the search has a newline + // ending, the replace omits it. Disagreement at the + // ending position means the replace's empty ending + // wins, so the next content line folds in. Pass 1 + // handles this as a byte-literal match. + { + name: "CallerChosenFold", + content: "foo\nline\nbar\n", + edits: []edit{{search: "line\n", replace: "LINE"}}, + expected: "foo\nLINEbar\n", + }, + + // Caller deliberately rewrites indent: search leads with + // a tab, replace leads with two spaces. Disagreement on + // the leading-whitespace position means the replacement's + // spaces win on the edited line. The untouched following + // line keeps its tab. + { + name: "CallerRewritesIndent_ReplaceLeadingWins", + content: "foo\n\tline\n\tbar\n", + edits: []edit{{search: "\tline\n", replace: " line\n"}}, + expected: "foo\n line\n\tbar\n", + }, + + // Expansion: replace has more lines than the matched + // region. Extras reference the last paired search/content + // line, so an extra whose leading whitespace agrees with + // the last paired search line picks up the file's + // leading whitespace. Search uses 4 spaces to force the + // fuzzy path (pass 1 would splice verbatim). + { + name: "Expansion_ExtraLinesTrackLastPair", + content: "foo\n\tline\nbar\n", + edits: []edit{{search: " line\n", replace: " line\n extra\n"}}, + expected: "foo\n\tline\n\textra\nbar\n", + }, + + // Collapse: replace has fewer lines than the matched + // region. Unpaired matched lines are consumed without + // output. + { + name: "Collapse_ReplaceShorterThanSearch", + content: "foo\nkeep\ndrop\nbar\n", + edits: []edit{{search: "keep\ndrop\n", replace: "keep\n"}}, + expected: "foo\nkeep\nbar\n", + }, + + // Empty-ending wildcard: search has no trailing newline + // and leading whitespace that isn't in the file. Pass 1 + // fails (the leading spaces aren't a substring). Pass 3 + // (trim-all) matches. At the splice: search and replace + // both have empty endings, so endingShapeEqual agrees + // and the file's "\r\n" wins. The file's leading tab + // does not win because sLead=" " disagrees with + // rLead="", so the replacement's empty lead wins. + { + name: "EmptyEndingWildcard_CRLFContent_FileEndingWins", + content: "foo\r\nkey\r\nbar\r\n", + edits: []edit{{search: " key", replace: "KEY"}}, + expected: "foo\r\nKEY\r\nbar\r\n", + }, + + // Multi-line replacement at EOF without trailing newline. + // The reference content line at the last index has + // cEnd="", but interior replacement lines must keep their + // "\n" rather than inherit the empty ending. + { + name: "MultiLineReplaceAtEOFNoNewline_InteriorLinesKeepNewline", + content: "foo\nbar", + edits: []edit{{search: "foo\nbar\n", replace: "foo\nbaz\nqux\n"}}, + expected: "foo\nbaz\nqux", + }, + + // Empty replacement body must not inherit the file's + // surrounding whitespace. Search forces the fuzzy path + // via trimming; replace is a single blank line. + { + name: "EmptyBodyFuzzyReplace_NoWhitespaceGhost", + content: "prefix\n code \nsuffix\n", + edits: []edit{{search: "code\n", replace: "\n"}}, + expected: "prefix\n\nsuffix\n", + }, + + // Combined: multi-line replacement at EOF without a + // newline, with an interior empty-body line. Exercises + // both carve-outs in one splice: the empty-body line + // must not inherit file whitespace, and interior lines + // must keep their newline even though the reference + // content line has cEnd="". + { + name: "EmptyBodyInteriorAtEOFNoNewline_BothCarveOuts", + content: "foo\nbar", + edits: []edit{{search: "foo\nbar\n", replace: "mid1\n\nmid2\n"}}, + expected: "mid1\n\nmid2", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "fuzzy-"+tt.name) + require.NoError(t, afero.WriteFile(fs, path, []byte(tt.content), 0o644)) + + sdkEdits := make([]workspacesdk.FileEdit, 0, len(tt.edits)) + for _, e := range tt.edits { + sdkEdits = append(sdkEdits, workspacesdk.FileEdit{ + Search: e.search, + Replace: e.replace, + ReplaceAll: e.replaceAll, + }) + } + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{Path: path, Edits: sdkEdits}}, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, tt.expected, string(data)) + }) + } +} + +// TestFuzzyReplace_EndingNormalization pins the line-ending rule. +// +// Rule: every spliced line gets the file's dominant ending, except +// when the caller signaled intent by making search and replace +// disagree on internal endings (both non-empty, different). Intent +// requires pass 1 to byte-match the file's endings; if it does, +// replace's endings are honored per-line. When only one side has +// internal endings (single-line vs. multi-line), the file wins. +// +// No-EOL at EOF is preserved: the final spliced line keeps its +// ending, so a match covering the file's last line does not +// materialize a newline the file never had. +func TestFuzzyReplace_EndingNormalization(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + type edit struct { + search, replace string + replaceAll bool + } + tests := []struct { + name string + content string + edits []edit + expected string + }{ + // CRLF file, LF search, LF replace with expansion. + // Internal endings agree (both LF), rule fires, every + // spliced line becomes CRLF. + { + name: "CRLFFile_LFSearchReplace_Expansion", + content: "line1\r\nline2\r\nline3\r\n", + edits: []edit{{search: "line1\nline2\n", replace: "line1\nINSERTED\nline2\n"}}, + expected: "line1\r\nINSERTED\r\nline2\r\nline3\r\n", + }, + // CRLF file with no trailing newline, LF search/replace + // with expansion that covers the file's last line. Interior + // spliced lines become CRLF; final spliced line preserves + // the file's no-EOL property. + { + name: "CRLFFileNoEOL_LFSearchReplace_ExpansionAtEOF", + content: "alpha\r\nbeta\r\ngamma", + edits: []edit{{search: "gamma", replace: "gamma\ndelta\nepsilon"}}, + expected: "alpha\r\nbeta\r\ngamma\r\ndelta\r\nepsilon", + }, + // CRLF Go file with no final newline; LLM sends LF + // search/replace that expands the function body. This is + // the motivating real-world case for the rule. + { + name: "CRLFFileNoEOL_LFCallerExpandsFunctionBody", + content: "package main\r\n\r\nfunc main() {\r\n\tprintln(\"hi\")\r\n}", + edits: []edit{{search: "\tprintln(\"hi\")\n}", replace: "\tprintln(\"hi\")\n\tprintln(\"bye\")\n\treturn\n}"}}, + expected: "package main\r\n\r\nfunc main() {\r\n\tprintln(\"hi\")\r\n\tprintln(\"bye\")\r\n\treturn\r\n}", + }, + // LF file, CRLF search/replace (caller sent CRLF, file is + // LF). Internal endings agree (both CRLF). Rule fires, the + // file's LF wins. + { + name: "LFFile_CRLFSearchReplace_FileLFWins", + content: "one\ntwo\nthree\n", + edits: []edit{{search: "one\r\ntwo\r\n", replace: "ONE\r\nTWO\r\n"}}, + expected: "ONE\nTWO\nthree\n", + }, + // Caller got endings right: CRLF in search, replace, and file. + // Pins that normalization doesn't regress this happy path. + { + name: "CRLFFile_CRLFSearchReplace_SanityPreserved", + content: "a\r\nb\r\nc\r\n", + edits: []edit{{search: "a\r\nb\r\n", replace: "A\r\nB\r\n"}}, + expected: "A\r\nB\r\nc\r\n", + }, + // ReplaceAll with expansion on a CRLF file via LF caller. + // Every spliced region must be CRLF throughout. + { + name: "ReplaceAll_CRLFFile_LFCaller_Expansion", + content: "key\r\nother\r\nkey\r\n", + edits: []edit{{ + search: "key\n", + replace: "KEY\nEXTRA\n", + replaceAll: true, + }}, + expected: "KEY\r\nEXTRA\r\nother\r\nKEY\r\nEXTRA\r\n", + }, + // Caller sent CRLF search and LF replace against a CRLF + // file. Different ending styles between search and replace + // signal caller intent to change endings. Search's CRLF + // byte-matches the file's CRLF, so the match succeeds and + // replace's LF endings are honored per-line. The untouched + // trailing line keeps its CRLF. + { + name: "CallerIntent_SearchMatchesFile_ReplaceEndingsHonored", + content: "x\r\ny\r\nz\r\n", + edits: []edit{{search: "x\r\ny\r\n", replace: "X\nY\n"}}, + expected: "X\nY\nz\r\n", + }, + // Single-line search against a CRLF file, multi-line + // replace. Search has no endings, so no caller intent is + // signaled and the file's CRLF wins for every spliced line. + { + name: "SingleLineSearch_MultiLineReplace_FileEndingWins", + content: "a\r\nx\r\nb\r\n", + edits: []edit{{search: "x", replace: "X\nY"}}, + expected: "a\r\nX\r\nY\r\nb\r\n", + }, + // Trivial baseline: neither side has endings, nothing to + // normalize. + { + name: "SingleLineSearch_SingleLineReplace_NoEndingsToNormalize", + content: "a\r\nx\r\nb\r\n", + edits: []edit{{search: "x", replace: "X"}}, + expected: "a\r\nX\r\nb\r\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "endnorm-"+tt.name) + require.NoError(t, afero.WriteFile(fs, path, []byte(tt.content), 0o644)) + + sdkEdits := make([]workspacesdk.FileEdit, 0, len(tt.edits)) + for _, e := range tt.edits { + sdkEdits = append(sdkEdits, workspacesdk.FileEdit{ + Search: e.search, + Replace: e.replace, + ReplaceAll: e.replaceAll, + }) + } + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{Path: path, Edits: sdkEdits}}, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, tt.expected, string(data)) + }) + } +} + +// TestFuzzyReplace_FuzzyCollapse_PreservesNextLine pins that a +// shorter replacement under the fuzzy path does not merge the +// next unmatched content line onto the last spliced line. +func TestFuzzyReplace_FuzzyCollapse_PreservesNextLine(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + type edit struct { + search, replace string + } + tests := []struct { + name string + content string + edits []edit + expected string + }{ + // Minimal: tab-indented file, space-indented caller + // forces pass 3, replace has fewer lines than search. + { + name: "Minimal", + content: "\tone\n\ttwo\n\tthree\n\tafter\n", + edits: []edit{{ + search: " one\n two\n three\n", + replace: " ONE\n TWO\n", + }}, + expected: "\tONE\n\tTWO\n\tafter\n", + }, + // The adversarial harness's reproduction from + // coderd/httpapi/httpapi.go, inline: the original had + // `return valid == nil` on its own line after the + // matched region. The bug merged it onto the last + // replacement line with a tab separator. + { + name: "HarnessHttpapi", + content: "\tnameValidator := func(fl validator.FieldLevel) bool {\n" + + "\t\tf := fl.Field().Interface()\n" + + "\t\tstr, ok := f.(string)\n" + + "\t\tif !ok {\n" + + "\t\t\treturn false\n" + + "\t\t}\n" + + "\t\tvalid := codersdk.NameValid(str)\n" + + "\t\treturn valid == nil\n" + + "\t}\n", + edits: []edit{{ + search: " f := fl.Field().Interface()\n" + + " str, ok := f.(string)\n" + + " if !ok {\n" + + " return false\n" + + " }\n" + + " valid := codersdk.NameValid(str)", + replace: " f := fl.Field().Interface()\n" + + " str, _ := f.(string)\n" + + " valid := codersdk.NameValid(str)", + }}, + expected: "\tnameValidator := func(fl validator.FieldLevel) bool {\n" + + "\t\tf := fl.Field().Interface()\n" + + "\t\tstr, _ := f.(string)\n" + + "\t\tvalid := codersdk.NameValid(str)\n" + + "\t\treturn valid == nil\n" + + "\t}\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "fuzzycollapse-"+tt.name) + require.NoError(t, afero.WriteFile(fs, path, []byte(tt.content), 0o644)) + + sdkEdits := make([]workspacesdk.FileEdit, 0, len(tt.edits)) + for _, e := range tt.edits { + sdkEdits = append(sdkEdits, workspacesdk.FileEdit{ + Search: e.search, + Replace: e.replace, + }) + } + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{Path: path, Edits: sdkEdits}}, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, tt.expected, string(data)) + }) + } +} + +// TestEditFiles_WhitespaceAndLineEndings covers whitespace and +// line-ending behaviors end-to-end through the HTTP handler, +// complementing the matcher-focused TestFuzzyReplace_EndingAndWhitespace. +// Each case has a short comment describing the behavior it pins. +func TestEditFiles_WhitespaceAndLineEndings(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + cases := []struct { + name string + content string + search, replace string + replaceAll bool + expected string // empty => expect an error response + errSub string + }{ + // Tab-indented file, search matches one tab-indented + // line byte-for-byte via pass 1. Tabs on untouched + // lines remain; untouched space-indented lines remain. + { + name: "TabIndentedLine_ExactMatch", + content: "\ttab indented line 1\n\ttab indented line 2\n spaces line 3\n spaces line 4\n\ttab indented line 5\n", + search: "\ttab indented line 1", + replace: "\ttab indented line 1 EDITED", + expected: "\ttab indented line 1 EDITED\n\ttab indented line 2\n" + + " spaces line 3\n spaces line 4\n\ttab indented line 5\n", + }, + + // Trailing whitespace on the content line is preserved + // via pass 1 (byte-substring match) because the search + // is a proper substring that doesn't touch the trailing + // whitespace. + { + name: "TrailingWhitespace_Preserved_ByPass1", + content: "line with trailing spaces \nno trailing ws\n", + search: "line with trailing spaces", + replace: "line with trailing spaces EDITED", + expected: "line with trailing spaces EDITED \nno trailing ws\n", + }, + + // File has two blank lines between "above" and "below"; + // search omits them. Fuzzy passes also reject because + // the search spans fewer lines than the content does, + // so blank lines are preserved significant content. + { + name: "BlankLinesAreSignificant_Rejects", + content: "above\n\n\nbelow\n", + search: "above\nbelow", + replace: "above\nbelow", + errSub: "search string not found", + }, + + // Search matches blank lines exactly; replacement + // collapses the region. + { + name: "RemoveBlankLines", + content: "above\n\n\nbelow\n", + search: "above\n\n\nbelow", + replace: "above\nbelow", + expected: "above\nbelow\n", + }, + + // CRLF file, pass 1 substring match preserves "\r\n" + // boundaries on every line. + { + name: "CRLF_Pass1_PreservesCRLF", + content: "line one\r\nline two\r\nline three\r\n", + search: "line two", + replace: "line two EDITED", + expected: "line one\r\nline two EDITED\r\nline three\r\n", + }, + + // CRLF file, LF search and replace. The ending rule + // accepts the match, and the splice rule promotes the + // replacement's LF endings to the file's "\r\n" + // because search and replace agree on ending shape. + { + name: "CRLF_FuzzyWithLF_FileEndingWins", + content: "line one\r\nline two\r\nline three\r\n", + search: "line one\nline two\n", + replace: "line one EDITED\nline two EDITED\n", + expected: "line one EDITED\r\nline two EDITED\r\nline three\r\n", + }, + + // File has no trailing newline; pass 1 preserves EOF + // shape. + { + name: "NoTrailingNewline_Preserved", + content: "no trailing newline", + search: "no trailing newline", + replace: "no trailing newline EDITED", + expected: "no trailing newline EDITED", + }, + + // Tab-indented content, space-indented search and + // replace. Pass 3 matches the line body ignoring + // leading whitespace. Search and replace agree on + // leading whitespace (both " ") so the file's "\t" + // wins; search and replace agree on ending (both + // "\n") so the file's "\n" wins. The following + // "\titem two\n" is not folded into the replacement. + { + name: "FuzzyIndent_FileIndentWins_NoLineFolding", + content: "\titem one\n\titem two\n", + search: " item one\n", + replace: " item one EDITED\n", + expected: "\titem one EDITED\n\titem two\n", + }, + } + + for _, ct := range cases { + t.Run(ct.name, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "ws-"+ct.name) + require.NoError(t, afero.WriteFile(fs, path, []byte(ct.content), 0o644)) + + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: path, + Edits: []workspacesdk.FileEdit{{ + Search: ct.search, + Replace: ct.replace, + ReplaceAll: ct.replaceAll, + }}, + }}, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + if ct.errSub != "" { + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + got := &codersdk.Error{} + require.NoError(t, json.NewDecoder(w.Body).Decode(got)) + require.ErrorContains(t, got, ct.errSub) + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, ct.content, string(data)) + return + } + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, ct.expected, string(data)) + }) + } +} + +// TestFuzzyReplace_Rejects pins the cases the matcher rejects, so +// regressions that weaken the guardrails get caught. Each case runs +// through the HTTP handler; the handler must return 400 with an +// error message matching errSub, and the file must be unchanged. +// +// Rejection sources: +// +// - Empty search (meaningful search text is required; the old +// behavior matched at every byte position when combined with +// replace_all). +// - Ambiguous match without replace_all (N > 1 occurrences of the +// search text). +// - Search not found in file (after all three passes fail). +// - Content mismatch that cannot be recovered by trimming +// whitespace on either side. +// - Blank-line count mismatch inside the matched region. +func TestFuzzyReplace_Rejects(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + type edit struct { + search, replace string + replaceAll bool + } + tests := []struct { + name string + content string + edits []edit + errSub string + }{ + // Empty search with replace_all=false: reject to prevent + // the ambiguous "prepend at byte 0" behavior. + { + name: "EmptySearch_Rejects", + content: "hello\n", + edits: []edit{{search: "", replace: "X"}}, + errSub: "search string must not be empty", + }, + // Empty search with replace_all=true: historically + // injected the replacement between every byte, silently + // corrupting the file. Reject explicitly. + { + name: "EmptySearch_ReplaceAll_Rejects", + content: "hello\n", + edits: []edit{{search: "", replace: "X", replaceAll: true}}, + errSub: "search string must not be empty", + }, + // Ambiguous single-replace: 3 distinct matches, caller + // did not ask for replace_all. + { + name: "Ambiguous_SingleReplace_Rejects", + content: "a\na\na\nother\n", + edits: []edit{{search: "a", replace: "A"}}, + errSub: "matches 3 occurrences", + }, + // Search text does not appear anywhere in the file. All + // three passes miss. + { + name: "NotFound_Rejects", + content: "hello\nworld\n", + edits: []edit{{search: "nonexistent\n", replace: "X\n"}}, + errSub: "search string not found", + }, + // Content mismatch that trimming cannot recover: search + // has different letters, not just different whitespace. + { + name: "ContentMismatch_Rejects", + content: "hello\n", + edits: []edit{{search: "Hello\n", replace: "HELLO\n"}}, + errSub: "search string not found", + }, + // Blank lines in the file that the search omits: the + // fuzzy window cannot align against the blank lines, so + // the multi-line match fails. + { + name: "BlankLineMismatch_Rejects", + content: "above\n\n\nbelow\n", + edits: []edit{{search: "above\nbelow\n", replace: "above\nbelow\n"}}, + errSub: "search string not found", + }, + // Search/replace disagreement signals intent to rewrite + // endings; search must byte-match the file's. LF search + // against CRLF file fails pass 1 and must reject rather + // than fall through to pass 2's CRLF/LF interchange. + { + name: "CallerIntent_SearchDoesNotMatchFileEnding_Rejects", + content: "x\r\ny\r\nz\r\n", + edits: []edit{{search: "x\ny\n", replace: "X\r\nY\r\n"}}, + errSub: "search string not found", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "reject-"+tt.name) + require.NoError(t, afero.WriteFile(fs, path, []byte(tt.content), 0o644)) + + sdkEdits := make([]workspacesdk.FileEdit, 0, len(tt.edits)) + for _, e := range tt.edits { + sdkEdits = append(sdkEdits, workspacesdk.FileEdit{ + Search: e.search, + Replace: e.replace, + ReplaceAll: e.replaceAll, + }) + } + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{Path: path, Edits: sdkEdits}}, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + got := &codersdk.Error{} + require.NoError(t, json.NewDecoder(w.Body).Decode(got)) + require.ErrorContains(t, got, tt.errSub) + + // File must not have been modified by any partial + // splice or write. + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, tt.content, string(data)) + }) + } +} + +// TestEditFiles_DuplicatePath_Rejects pins that duplicate paths in +// one request are rejected with 400 and the file on disk is +// unchanged. The pre-fix behavior silently dropped the first +// entry's edits while reporting success (last write wins). +func TestEditFiles_DuplicatePath_Rejects(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "dup-path") + original := "one\ntwo\nthree\n" + require.NoError(t, afero.WriteFile(fs, path, []byte(original), 0o644)) + + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{ + {Path: path, Edits: []workspacesdk.FileEdit{{Search: "one", Replace: "ONE"}}}, + {Path: path, Edits: []workspacesdk.FileEdit{{Search: "three", Replace: "THREE"}}}, + }, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + got := &codersdk.Error{} + require.NoError(t, json.NewDecoder(w.Body).Decode(got)) + require.ErrorContains(t, got, "duplicate file path") + + // File on disk must be untouched: no partial edits. + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, original, string(data)) +} + +// TestEditFiles_DuplicatePath_SymlinkAliasRejects pins that two +// request entries pointing to the same real file (one direct, one +// via a symlink) are rejected. Without resolve-before-dedup, the +// raw-path check lets both entries through, and the second write +// silently overwrites the first. +func TestEditFiles_DuplicatePath_SymlinkAliasRejects(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("symlinks are not reliably supported on Windows") + } + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + dir := t.TempDir() + osFs := afero.NewOsFs() + api := agentfiles.NewAPI(logger, osFs, nil) + + realPath := filepath.Join(dir, "real.txt") + original := "one\ntwo\nthree\n" + require.NoError(t, afero.WriteFile(osFs, realPath, []byte(original), 0o644)) + + linkPath := filepath.Join(dir, "link.txt") + require.NoError(t, os.Symlink(realPath, linkPath)) + + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{ + {Path: realPath, Edits: []workspacesdk.FileEdit{{Search: "one", Replace: "ONE"}}}, + {Path: linkPath, Edits: []workspacesdk.FileEdit{{Search: "three", Replace: "THREE"}}}, + }, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + got := &codersdk.Error{} + require.NoError(t, json.NewDecoder(w.Body).Decode(got)) + require.ErrorContains(t, got, "aliases") + + // File on disk must be untouched: the alias collision is caught + // before phase 1 so no write runs. + data, err := afero.ReadFile(osFs, realPath) + require.NoError(t, err) + require.Equal(t, original, string(data)) +} + +// TestEditFiles_ReplaceAll_FuzzyIndentGap locks the CURRENT output +// of a known foot-gun, it doesn't bless it. +// +// Gap: replace_all plus a pass-3 (indent-agnostic) match hits every +// nesting level whose body matches after TrimSpace. A caller aiming +// at one block silently edits the same pattern at other depths. +// The per-position splice preserves each match's local indent, so +// the output is syntactically fine. The foot-gun is that wrong +// SITES get edited. +// +// The right fix is a caller-side opt-out from fuzzy matching, out +// of scope for this PR. When that lands, update the test to assert +// the new behavior. +func TestEditFiles_ReplaceAll_FuzzyIndentGap(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "replaceall-fuzzyindent-gap") + + // File is tab-indented Go, with `if err != nil { return err }` + // at two nesting levels (2 tabs and 3 tabs). Caller sends a + // 4-space-indented search/replace pair with replace_all=true. + // Pass 1 fails (no 4-space prefix in file). Pass 2 fails (trim + // right doesn't touch leading whitespace). Pass 3 (TrimSpace) + // matches at BOTH depths. Current behavior: replace both. + content := "package main\n\nfunc a() {\n" + + "\t\tif err != nil {\n" + + "\t\t\treturn err\n" + + "\t\t}\n" + + "\t\t\tif err != nil {\n" + + "\t\t\t\treturn err\n" + + "\t\t\t}\n" + + "}\n" + require.NoError(t, afero.WriteFile(fs, path, []byte(content), 0o644)) + + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: path, + Edits: []workspacesdk.FileEdit{{ + Search: " if err != nil {\n" + + " return err\n" + + " }\n", + Replace: " if err != nil {\n" + + " return fmt.Errorf(\"wrap: %w\", err)\n" + + " }\n", + ReplaceAll: true, + }}, + }}, + } + + _ = runEditFiles(t, api, req) + + // Both depths got edited. The per-position splice preserved each + // site's local indent, so output is syntactically fine, just + // edited at two places, only one of which the caller likely + // intended. + expected := "package main\n\nfunc a() {\n" + + "\t\tif err != nil {\n" + + "\t\t\treturn fmt.Errorf(\"wrap: %w\", err)\n" + + "\t\t}\n" + + "\t\t\tif err != nil {\n" + + "\t\t\t\treturn fmt.Errorf(\"wrap: %w\", err)\n" + + "\t\t\t}\n" + + "}\n" + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, expected, string(data)) +} + +// TestEditFiles_FuzzyIndent_InsertionLevelAware covers indent- +// propagation bugs that fire when the caller's search/replace +// whitespace differs from the file's (tab vs space, 2sp vs 4sp). +// +// - Red_* cases assert the correct output that the indent-unit +// translation produces for inserted splice lines. +// - Lock_* cases pin output for middle-substitution scenarios +// that the insertion-only fix does not cover; tracked in +// CODAGT-214. +func TestEditFiles_FuzzyIndent_InsertionLevelAware(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + type edit struct { + search, replace string + replaceAll bool + } + tests := []struct { + name string + content string + edits []edit + expected string + }{ + // Wrap an existing line in a new block. Tab file, 4sp caller. + { + name: "Red_WrapInBlock_TabFile_4spLLM", + content: "func main() {\n" + + "\tfmt.Println(\"hello\")\n" + + "\tfmt.Println(\"world\")\n" + + "}\n", + edits: []edit{{ + search: " fmt.Println(\"hello\")\n" + + " fmt.Println(\"world\")", + replace: " fmt.Println(\"hello\")\n" + + " if verbose {\n" + + " fmt.Println(\"world\")\n" + + " }", + }}, + expected: "func main() {\n" + + "\tfmt.Println(\"hello\")\n" + + "\tif verbose {\n" + + "\t\tfmt.Println(\"world\")\n" + + "\t}\n" + + "}\n", + }, + + // Wrap in a new block, 2sp file, 4sp caller. The common + // real-world trigger: Claude/GPT default 4sp into a 2sp file. + { + name: "Red_WrapInBlock_2spFile_4spLLM", + content: "function main() {\n" + + " console.log('hello')\n" + + " console.log('world')\n" + + "}\n", + edits: []edit{{ + search: " console.log('hello')\n" + + " console.log('world')", + replace: " console.log('hello')\n" + + " if (verbose) {\n" + + " console.log('world')\n" + + " }", + }}, + expected: "function main() {\n" + + " console.log('hello')\n" + + " if (verbose) {\n" + + " console.log('world')\n" + + " }\n" + + "}\n", + }, + + // Expand a single line into an error-handling block. + { + name: "Red_SingleToMulti_ErrorHandling", + content: "func main() {\n" + + "\tx := getValue()\n" + + "\tfmt.Println(x)\n" + + "}\n", + edits: []edit{{ + search: " x := getValue()", + replace: " x, err := getValue()\n" + + " if err != nil {\n" + + " log.Fatal(err)\n" + + " }", + }}, + expected: "func main() {\n" + + "\tx, err := getValue()\n" + + "\tif err != nil {\n" + + "\t\tlog.Fatal(err)\n" + + "\t}\n" + + "\tfmt.Println(x)\n" + + "}\n", + }, + + // Insert a new validation block after an existing if-block. + { + name: "Red_InsertNewBlock_AfterExisting", + content: "func loadConfig() (*Config, error) {\n" + + "\tvar cfg Config\n" + + "\terr = json.Unmarshal(data, \u0026cfg)\n" + + "\tif err != nil {\n" + + "\t\treturn nil, err\n" + + "\t}\n" + + "\n" + + "\treturn \u0026cfg, nil\n" + + "}\n", + edits: []edit{{ + search: " var cfg Config\n" + + " err = json.Unmarshal(data, \u0026cfg)\n" + + " if err != nil {\n" + + " return nil, err\n" + + " }\n" + + "\n" + + " return \u0026cfg, nil", + replace: " var cfg Config\n" + + " err = json.Unmarshal(data, \u0026cfg)\n" + + " if err != nil {\n" + + " return nil, fmt.Errorf(\"unmarshal: %w\", err)\n" + + " }\n" + + " if err := cfg.Validate(); err != nil {\n" + + " return nil, fmt.Errorf(\"validate: %w\", err)\n" + + " }\n" + + "\n" + + " return \u0026cfg, nil", + }}, + expected: "func loadConfig() (*Config, error) {\n" + + "\tvar cfg Config\n" + + "\terr = json.Unmarshal(data, \u0026cfg)\n" + + "\tif err != nil {\n" + + "\t\treturn nil, fmt.Errorf(\"unmarshal: %w\", err)\n" + + "\t}\n" + + "\tif err := cfg.Validate(); err != nil {\n" + + "\t\treturn nil, fmt.Errorf(\"validate: %w\", err)\n" + + "\t}\n" + + "\n" + + "\treturn \u0026cfg, nil\n" + + "}\n", + }, + + // replace_all + pass 3 + expansion at two sites. + { + name: "Red_ReplaceAll_Pass3_Expansion", + content: "func handlers() {\n" + + "\thttp.HandleFunc(\"/a\", func(w http.ResponseWriter, r *http.Request) {\n" + + "\t\tdata := readBody(r)\n" + + "\t\tprocess(data)\n" + + "\t})\n" + + "\thttp.HandleFunc(\"/b\", func(w http.ResponseWriter, r *http.Request) {\n" + + "\t\tdata := readBody(r)\n" + + "\t\tprocess(data)\n" + + "\t})\n" + + "}\n", + edits: []edit{{ + search: " data := readBody(r)\n" + + " process(data)", + replace: " data := readBody(r)\n" + + " if data == nil {\n" + + " return\n" + + " }\n" + + " process(data)", + replaceAll: true, + }}, + expected: "func handlers() {\n" + + "\thttp.HandleFunc(\"/a\", func(w http.ResponseWriter, r *http.Request) {\n" + + "\t\tdata := readBody(r)\n" + + "\t\tif data == nil {\n" + + "\t\t\treturn\n" + + "\t\t}\n" + + "\t\tprocess(data)\n" + + "\t})\n" + + "\thttp.HandleFunc(\"/b\", func(w http.ResponseWriter, r *http.Request) {\n" + + "\t\tdata := readBody(r)\n" + + "\t\tif data == nil {\n" + + "\t\t\treturn\n" + + "\t\t}\n" + + "\t\tprocess(data)\n" + + "\t})\n" + + "}\n", + }, + + // Unwrap (decrease nesting). All output lines are + // middle-substitutions; CODAGT-214 covers the fix. + { + name: "Lock_Unwrap_MiddleSubDisagreement", + content: "func main() {\n" + + "\tif condition {\n" + + "\t\tdoSomething()\n" + + "\t\tdoMore()\n" + + "\t}\n" + + "}\n", + edits: []edit{{ + search: " if condition {\n" + + " doSomething()\n" + + " doMore()\n" + + " }", + replace: " doSomething()\n" + + " doMore()", + }}, + // Line 2 leaks 4 literal spaces (middle-sub disagreement + // rule: rLead wins when sLead != rLead). + expected: "func main() {\n" + + "\tdoSomething()\n" + + " doMore()\n" + + "}\n", + }, + + // Middle-rewrite with different nesting, tab file. Mixed + // fate: inserted lines fixed, middle-subs still leak. + { + name: "Lock_MiddleRewrite_DifferentNesting_Tab", + content: "func transform(items []Item) []Result {\n" + + "\tvar results []Result\n" + + "\tfor _, item := range items {\n" + + "\t\tif item.Valid {\n" + + "\t\t\tresults = append(results, convert(item))\n" + + "\t\t}\n" + + "\t}\n" + + "\treturn results\n" + + "}\n", + edits: []edit{{ + search: " var results []Result\n" + + " for _, item := range items {\n" + + " if item.Valid {\n" + + " results = append(results, convert(item))\n" + + " }\n" + + " }\n" + + " return results", + replace: " var results []Result\n" + + " for _, item := range items {\n" + + " result, err := convert(item)\n" + + " if err != nil {\n" + + " continue\n" + + " }\n" + + " results = append(results, result)\n" + + " }\n" + + " return results", + }}, + // Middle-sub lines (i=3, i=4) leak literal 8sp/12sp; + // the inserted } and append lines are tab-correct. + expected: "func transform(items []Item) []Result {\n" + + "\tvar results []Result\n" + + "\tfor _, item := range items {\n" + + "\t\tresult, err := convert(item)\n" + + " if err != nil {\n" + + " continue\n" + + "\t\t}\n" + + "\t\tresults = append(results, result)\n" + + "\t}\n" + + "\treturn results\n" + + "}\n", + }, + + // Same class as lock #7, 2sp file (JS/TS). + { + name: "Lock_MiddleRewrite_DifferentNesting_2sp", + content: "function transform(items) {\n" + + " const results = [];\n" + + " for (const item of items) {\n" + + " if (item.valid) {\n" + + " results.push(convert(item));\n" + + " }\n" + + " }\n" + + " return results;\n" + + "}\n", + edits: []edit{{ + search: " const results = [];\n" + + " for (const item of items) {\n" + + " if (item.valid) {\n" + + " results.push(convert(item));\n" + + " }\n" + + " }\n" + + " return results;", + replace: " const results = [];\n" + + " for (const item of items) {\n" + + " const result = convert(item);\n" + + " if (!result) {\n" + + " continue;\n" + + " }\n" + + " results.push(result);\n" + + " }\n" + + " return results;", + }}, + // Middle-sub lines (i=3, i=4) leak 8sp/12sp; the inserted + // } and push lines translate to 4sp correctly. + expected: "function transform(items) {\n" + + " const results = [];\n" + + " for (const item of items) {\n" + + " const result = convert(item);\n" + + " if (!result) {\n" + + " continue;\n" + + " }\n" + + " results.push(result);\n" + + " }\n" + + " return results;\n" + + "}\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "fuzzyindent-"+tt.name) + require.NoError(t, afero.WriteFile(fs, path, []byte(tt.content), 0o644)) + + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: path, + Edits: make([]workspacesdk.FileEdit, 0, len(tt.edits)), + }}, + } + for _, e := range tt.edits { + req.Files[0].Edits = append(req.Files[0].Edits, workspacesdk.FileEdit{ + Search: e.search, + Replace: e.replace, + ReplaceAll: e.replaceAll, + }) + } + + _ = runEditFiles(t, api, req) + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, tt.expected, string(data)) + }) + } +} + +// TestFuzzyReplace_Expansion_PreservesFileIndent pins that when +// replace has more lines than search, every spliced line keeps +// the file's indent style. Inserted lines especially must not +// carry the caller's literal whitespace into the output. +func TestFuzzyReplace_Expansion_PreservesFileIndent(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + path := filepath.Join(tmpdir, "fuzzy-expansion-gap") + + content := "\tnameValidator := func(fl validator.FieldLevel) bool {\n" + + "\t\tf := fl.Field().Interface()\n" + + "\t\tstr, ok := f.(string)\n" + + "\t\tif !ok {\n" + + "\t\t\treturn false\n" + + "\t\t}\n" + + "\t\tvalid := codersdk.NameValid(str)\n" + + "\t\treturn valid == nil\n" + + "\t}\n" + require.NoError(t, afero.WriteFile(fs, path, []byte(content), 0o644)) + + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: path, + Edits: []workspacesdk.FileEdit{{ + Search: " f := fl.Field().Interface()\n" + + " str, ok := f.(string)\n" + + " if !ok {\n" + + " return false\n" + + " }\n" + + " valid := codersdk.NameValid(str)", + Replace: " f := fl.Field().Interface()\n" + + " str, ok := f.(string)\n" + + " if !ok {\n" + + " log.Println(\"type assertion failed\")\n" + + " return false\n" + + " }\n" + + " valid := codersdk.NameValid(str)", + }}, + }}, + } + + _ = runEditFiles(t, api, req) + + // All lines emitted in the file's tab indent, including the + // inserted log.Println and the following return false (which + // index-pairs with a different search line but shares the same + // 3-tab depth in the file). + expected := "\tnameValidator := func(fl validator.FieldLevel) bool {\n" + + "\t\tf := fl.Field().Interface()\n" + + "\t\tstr, ok := f.(string)\n" + + "\t\tif !ok {\n" + + "\t\t\tlog.Println(\"type assertion failed\")\n" + + "\t\t\treturn false\n" + + "\t\t}\n" + + "\t\tvalid := codersdk.NameValid(str)\n" + + "\t\treturn valid == nil\n" + + "\t}\n" + data, err := afero.ReadFile(fs, path) + require.NoError(t, err) + require.Equal(t, expected, string(data)) +} diff --git a/coderd/x/chatd/chattool/editfiles.go b/coderd/x/chatd/chattool/editfiles.go index d669362fbd..697d179ffc 100644 --- a/coderd/x/chatd/chattool/editfiles.go +++ b/coderd/x/chatd/chattool/editfiles.go @@ -96,8 +96,15 @@ func executeEditFilesTool( } } - if err := conn.EditFiles(ctx, workspacesdk.FileEditRequest{Files: args.Files}); err != nil { + resp, err := conn.EditFiles(ctx, workspacesdk.FileEditRequest{ + Files: args.Files, + IncludeDiff: true, + }) + if err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } - return toolResponse(map[string]any{"ok": true}), nil + return toolResponse(map[string]any{ + "ok": true, + "files": resp.Files, + }), nil } diff --git a/coderd/x/chatd/chattool/editfiles_test.go b/coderd/x/chatd/chattool/editfiles_test.go index 4438a43174..d025d1ca4b 100644 --- a/coderd/x/chatd/chattool/editfiles_test.go +++ b/coderd/x/chatd/chattool/editfiles_test.go @@ -2,6 +2,7 @@ package chattool_test import ( "context" + "encoding/json" "net/http" "testing" @@ -85,14 +86,17 @@ func TestEditFiles(t *testing.T) { planPath := "/home/coder/.coder/plans/PLAN-test-uuid.md" resolvePlanPathCalls := 0 mockConn.EXPECT().ResolvePath(gomock.Any(), planPath).Return(planPath, nil) - request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ - Path: planPath, - Edits: []workspacesdk.FileEdit{{ - Search: "old", - Replace: "new", + request := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: planPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, }}, - }}} - mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + IncludeDiff: true, + } + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(workspacesdk.FileEditResponse{}, nil) tool := chattool.EditFiles(chattool.EditFilesOptions{ GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { @@ -123,14 +127,17 @@ func TestEditFiles(t *testing.T) { mockConn.EXPECT(). ResolvePath(gomock.Any(), planPath). Return("", statusError{statusCode: http.StatusNotFound, message: "missing resolve-path endpoint"}) - request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ - Path: planPath, - Edits: []workspacesdk.FileEdit{{ - Search: "old", - Replace: "new", + request := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: planPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, }}, - }}} - mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + IncludeDiff: true, + } + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(workspacesdk.FileEditResponse{}, nil) tool := chattool.EditFiles(chattool.EditFilesOptions{ GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { @@ -290,14 +297,17 @@ func TestEditFiles(t *testing.T) { ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) chatPlanPath := "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md" - request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ - Path: chatPlanPath, - Edits: []workspacesdk.FileEdit{{ - Search: "old", - Replace: "new", + request := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: chatPlanPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, }}, - }}} - mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + IncludeDiff: true, + } + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(workspacesdk.FileEditResponse{}, nil) resolvePlanPathCalled := false tool := chattool.EditFiles(chattool.EditFilesOptions{ @@ -324,14 +334,17 @@ func TestEditFiles(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ - Path: "/home/coder/myproject/plan.md", - Edits: []workspacesdk.FileEdit{{ - Search: "old", - Replace: "new", + request := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: "/home/coder/myproject/plan.md", + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, }}, - }}} - mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + IncludeDiff: true, + } + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(workspacesdk.FileEditResponse{}, nil) tool := chattool.EditFiles(chattool.EditFilesOptions{ GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { @@ -355,14 +368,17 @@ func TestEditFiles(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ - Path: "/home/coder/myproject/plan.md", - Edits: []workspacesdk.FileEdit{{ - Search: "old", - Replace: "new", + request := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: "/home/coder/myproject/plan.md", + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, }}, - }}} - mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + IncludeDiff: true, + } + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(workspacesdk.FileEditResponse{}, nil) planPathCalled := false tool := chattool.EditFiles(chattool.EditFilesOptions{ @@ -389,14 +405,17 @@ func TestEditFiles(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ - Path: "/home/dev/my-plan.md", - Edits: []workspacesdk.FileEdit{{ - Search: "old", - Replace: "new", + request := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: "/home/dev/my-plan.md", + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, }}, - }}} - mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + IncludeDiff: true, + } + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(workspacesdk.FileEditResponse{}, nil) resolvePlanPathCalled := false tool := chattool.EditFiles(chattool.EditFilesOptions{ @@ -423,14 +442,17 @@ func TestEditFiles(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ - Path: chattool.LegacySharedPlanPath, - Edits: []workspacesdk.FileEdit{{ - Search: "old", - Replace: "new", + request := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: chattool.LegacySharedPlanPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, }}, - }}} - mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + IncludeDiff: true, + } + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(workspacesdk.FileEditResponse{}, nil) tool := chattool.EditFiles(chattool.EditFilesOptions{ GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { @@ -447,3 +469,55 @@ func TestEditFiles(t *testing.T) { assert.False(t, resp.IsError) }) } + +func TestEditFiles_ToolResponseCarriesFileResults(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + targetPath := "/home/coder/target.txt" + expectedFiles := []workspacesdk.FileEditResult{ + { + Path: targetPath, + Diff: "--- " + targetPath + "\n+++ " + targetPath + "\n@@ -1 +1 @@\n-old\n+new\n", + }, + } + // The tool must opt into diffs (IncludeDiff: true) and forward + // the agent's per-file results through to its response. + mockConn.EXPECT(). + EditFiles(gomock.Any(), workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: targetPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, + }}, + IncludeDiff: true, + }). + Return(workspacesdk.FileEditResponse{Files: expectedFiles}, nil) + + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"` + targetPath + `","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + + var decoded struct { + OK bool `json:"ok"` + Files []workspacesdk.FileEditResult `json:"files"` + } + require.NoError(t, json.Unmarshal([]byte(resp.Content), &decoded)) + assert.True(t, decoded.OK) + require.Len(t, decoded.Files, 1) + assert.Equal(t, targetPath, decoded.Files[0].Path) + assert.Equal(t, expectedFiles[0].Diff, decoded.Files[0].Diff) +} diff --git a/codersdk/toolsdk/toolsdk.go b/codersdk/toolsdk/toolsdk.go index 75b8df27b3..a0db2d98bf 100644 --- a/codersdk/toolsdk/toolsdk.go +++ b/codersdk/toolsdk/toolsdk.go @@ -1665,7 +1665,19 @@ type WorkspaceEditFileArgs struct { Edits []workspacesdk.FileEdit `json:"edits"` } -var WorkspaceEditFile = Tool[WorkspaceEditFileArgs, codersdk.Response]{ +// WorkspaceEditFilesResponse is the response shape for the edit-file +// and edit-files tools. Message preserves the existing success text. +// Files carries the per-file results returned by the agent +// (populated when the agent-side IncludeDiff flag was set). The +// field is named Files (matching the agent's FileEditResponse.Files) +// so future per-file error or status fields can be added without a +// second wire break. +type WorkspaceEditFilesResponse struct { + Message string `json:"message"` + Files []workspacesdk.FileEditResult `json:"files,omitempty"` +} + +var WorkspaceEditFile = Tool[WorkspaceEditFileArgs, WorkspaceEditFilesResponse]{ Tool: aisdk.Tool{ Name: ToolNameWorkspaceEditFile, Description: `Edit a file in a workspace.`, @@ -1703,27 +1715,29 @@ var WorkspaceEditFile = Tool[WorkspaceEditFileArgs, codersdk.Response]{ }, MCPAnnotations: mcpDestructiveAnnotations, UserClientOptional: true, - Handler: func(ctx context.Context, deps Deps, args WorkspaceEditFileArgs) (codersdk.Response, error) { + Handler: func(ctx context.Context, deps Deps, args WorkspaceEditFileArgs) (WorkspaceEditFilesResponse, error) { conn, err := newAgentConn(ctx, deps.coderClient, args.Workspace) if err != nil { - return codersdk.Response{}, err + return WorkspaceEditFilesResponse{}, err } defer conn.Close() - err = conn.EditFiles(ctx, workspacesdk.FileEditRequest{ + resp, err := conn.EditFiles(ctx, workspacesdk.FileEditRequest{ Files: []workspacesdk.FileEdits{ { Path: args.Path, Edits: args.Edits, }, }, + IncludeDiff: true, }) if err != nil { - return codersdk.Response{}, err + return WorkspaceEditFilesResponse{}, err } - return codersdk.Response{ + return WorkspaceEditFilesResponse{ Message: "File edited successfully.", + Files: resp.Files, }, nil }, } @@ -1733,7 +1747,7 @@ type WorkspaceEditFilesArgs struct { Files []workspacesdk.FileEdits `json:"files"` } -var WorkspaceEditFiles = Tool[WorkspaceEditFilesArgs, codersdk.Response]{ +var WorkspaceEditFiles = Tool[WorkspaceEditFilesArgs, WorkspaceEditFilesResponse]{ Tool: aisdk.Tool{ Name: ToolNameWorkspaceEditFiles, Description: `Edit one or more files in a workspace.`, @@ -1785,20 +1799,24 @@ var WorkspaceEditFiles = Tool[WorkspaceEditFilesArgs, codersdk.Response]{ }, MCPAnnotations: mcpDestructiveAnnotations, UserClientOptional: true, - Handler: func(ctx context.Context, deps Deps, args WorkspaceEditFilesArgs) (codersdk.Response, error) { + Handler: func(ctx context.Context, deps Deps, args WorkspaceEditFilesArgs) (WorkspaceEditFilesResponse, error) { conn, err := newAgentConn(ctx, deps.coderClient, args.Workspace) if err != nil { - return codersdk.Response{}, err + return WorkspaceEditFilesResponse{}, err } defer conn.Close() - err = conn.EditFiles(ctx, workspacesdk.FileEditRequest{Files: args.Files}) + resp, err := conn.EditFiles(ctx, workspacesdk.FileEditRequest{ + Files: args.Files, + IncludeDiff: true, + }) if err != nil { - return codersdk.Response{}, err + return WorkspaceEditFilesResponse{}, err } - return codersdk.Response{ + return WorkspaceEditFilesResponse{ Message: "File(s) edited successfully.", + Files: resp.Files, }, nil }, } diff --git a/codersdk/workspacesdk/agentconn.go b/codersdk/workspacesdk/agentconn.go index cbdc73c1b6..58abc164d1 100644 --- a/codersdk/workspacesdk/agentconn.go +++ b/codersdk/workspacesdk/agentconn.go @@ -86,7 +86,7 @@ type AgentConn interface { ReadFile(ctx context.Context, path string, offset, limit int64) (io.ReadCloser, string, error) ReadFileLines(ctx context.Context, path string, offset, limit int64, limits ReadFileLinesLimits) (ReadFileLinesResponse, error) WriteFile(ctx context.Context, path string, reader io.Reader) error - EditFiles(ctx context.Context, edits FileEditRequest) error + EditFiles(ctx context.Context, edits FileEditRequest) (FileEditResponse, error) SSH(ctx context.Context) (*gonet.TCPConn, error) SSHClient(ctx context.Context) (*ssh.Client, error) SSHClientOnPort(ctx context.Context, port uint16) (*ssh.Client, error) @@ -1043,6 +1043,32 @@ type FileEdits struct { type FileEditRequest struct { Files []FileEdits `json:"files"` + // IncludeDiff asks the agent to compute a unified diff per file + // and return it in FileEditResponse.Files[i].Diff. When false + // (default) the agent skips diff computation and Files is nil. + IncludeDiff bool `json:"include_diff,omitempty"` +} + +// FileEditResponse is the success response for the edit-files endpoint. +// When the request's IncludeDiff flag is set, Files contains one entry +// per edited file in request order. Each entry's Path matches the +// caller-supplied path (pre-symlink resolution). +// +// The slice is named Files (rather than Diffs) so future work can +// hang per-file errors or status off each element without a second +// wire break. +type FileEditResponse struct { + Files []FileEditResult `json:"files,omitempty"` +} + +// FileEditResult carries the outcome of editing one file. Path is +// the original caller-supplied path, not any symlink-resolved +// target. Diff is the unified-diff string produced when the +// caller set FileEditRequest.IncludeDiff; it is empty for no-op +// edits or when diffs were not requested. +type FileEditResult struct { + Path string `json:"path"` + Diff string `json:"diff"` } // ListMCPToolsResponse is the response from the agent's @@ -1219,24 +1245,26 @@ func (c *agentConn) SignalProcess(ctx context.Context, id string, signal string) } // EditFiles performs search and replace edits on one or more files. -func (c *agentConn) EditFiles(ctx context.Context, edits FileEditRequest) error { +// When edits.IncludeDiff is true, the returned FileEditResponse +// carries a unified diff per edited file. +func (c *agentConn) EditFiles(ctx context.Context, edits FileEditRequest) (FileEditResponse, error) { ctx, span := tracing.StartSpan(ctx) defer span.End() res, err := c.apiRequest(ctx, http.MethodPost, "/api/v0/edit-files", edits) if err != nil { - return xerrors.Errorf("do request: %w", err) + return FileEditResponse{}, xerrors.Errorf("do request: %w", err) } defer res.Body.Close() if res.StatusCode != http.StatusOK { - return codersdk.ReadBodyAsError(res) + return FileEditResponse{}, codersdk.ReadBodyAsError(res) } - var m codersdk.Response - if err := json.NewDecoder(res.Body).Decode(&m); err != nil { - return xerrors.Errorf("decode response body: %w", err) + var resp FileEditResponse + if err := json.NewDecoder(res.Body).Decode(&resp); err != nil { + return FileEditResponse{}, xerrors.Errorf("decode response body: %w", err) } - return nil + return resp, nil } func agentAPIPath(path string, query neturl.Values) string { diff --git a/codersdk/workspacesdk/agentconnmock/agentconnmock.go b/codersdk/workspacesdk/agentconnmock/agentconnmock.go index b782038ea8..5c23246cae 100644 --- a/codersdk/workspacesdk/agentconnmock/agentconnmock.go +++ b/codersdk/workspacesdk/agentconnmock/agentconnmock.go @@ -204,11 +204,12 @@ func (mr *MockAgentConnMockRecorder) DialContext(ctx, network, addr any) *gomock } // EditFiles mocks base method. -func (m *MockAgentConn) EditFiles(ctx context.Context, edits workspacesdk.FileEditRequest) error { +func (m *MockAgentConn) EditFiles(ctx context.Context, edits workspacesdk.FileEditRequest) (workspacesdk.FileEditResponse, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "EditFiles", ctx, edits) - ret0, _ := ret[0].(error) - return ret0 + ret0, _ := ret[0].(workspacesdk.FileEditResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 } // EditFiles indicates an expected call of EditFiles. diff --git a/go.mod b/go.mod index 820bf2bc91..c4852d6fe7 100644 --- a/go.mod +++ b/go.mod @@ -496,6 +496,7 @@ require ( require ( charm.land/fantasy v0.8.1 github.com/anthropics/anthropic-sdk-go v1.19.0 + github.com/aymanbagabas/go-udiff v0.4.1 github.com/brianvoe/gofakeit/v7 v7.14.0 github.com/coder/agentapi-sdk-go v0.0.0-20250505131810-560d1d88d225 github.com/coder/aibridge v1.1.2 diff --git a/go.sum b/go.sum index cfe580af32..454ad0a488 100644 --- a/go.sum +++ b/go.sum @@ -203,8 +203,8 @@ github.com/aws/smithy-go v1.24.2 h1:FzA3bu/nt/vDvmnkg+R8Xl46gmzEDam6mZ1hzmwXFng= github.com/aws/smithy-go v1.24.2/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiErDT4WkJ2k= github.com/aymanbagabas/go-osc52/v2 v2.0.1/go.mod h1:uYgXzlJ7ZpABp8OJ+exZzJJhRNQ2ASbcXHWsFqH8hp8= -github.com/aymanbagabas/go-udiff v0.3.1 h1:LV+qyBQ2pqe0u42ZsUEtPiCaUoqgA9gYRDs3vj1nolY= -github.com/aymanbagabas/go-udiff v0.3.1/go.mod h1:G0fsKmG+P6ylD0r6N/KgQD/nWzgfnl8ZBcNLgcbrw8E= +github.com/aymanbagabas/go-udiff v0.4.1 h1:OEIrQ8maEeDBXQDoGCbbTTXYJMYRCRO1fnodZ12Gv5o= +github.com/aymanbagabas/go-udiff v0.4.1/go.mod h1:0L9PGwj20lrtmEMeyw4WKJ/TMyDtvAoK9bf2u/mNo3w= github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuPk= github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/bahlo/generic-list-go v0.2.0 h1:5sz/EEAK+ls5wF+NeqDpk5+iNdMDXrh3z3nPnH1Wvgk= diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx index de0f3aff28..48ccff8d9e 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx @@ -85,6 +85,7 @@ export const EditFilesTool: React.FC<{ diff ? ( { + const canvas = within(canvasElement); + expect(canvas.getByText(/Edited missing\.ts/)).toBeInTheDocument(); + // On error, no diff body: the synthetic fallback would + // misrepresent a rejected edit as applied. + expect(canvas.queryAllByTestId("edit-file-diff")).toHaveLength(0); + }, +}; + +export const EditFilesServerDiffMultiFile: Story = { + args: { + name: "edit_files", + status: "completed", + args: { + files: [ + { + path: "src/config.ts", + edits: [ + { + search: "const timeout = 30;", + replace: "const timeout = 60;", + }, + ], + }, + { + path: "src/server.ts", + edits: [ + { + search: 'const host = "localhost";', + replace: 'const host = "0.0.0.0";', + }, + ], + }, + ], + }, + result: { + ok: true, + files: [ + { + path: "src/config.ts", + diff: "--- src/config.ts\n+++ src/config.ts\n@@ -1,3 +1,3 @@\n export const settings = {\n-\tconst timeout = 30;\n+\tconst timeout = 60;\n };\n", + }, + { + path: "src/server.ts", + diff: '--- src/server.ts\n+++ src/server.ts\n@@ -1,3 +1,3 @@\n export const server = {\n-\tconst host = "localhost";\n+\tconst host = "0.0.0.0";\n };\n', + }, + ], + }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByText(/Edited 2 files/)).toBeInTheDocument(); + // Both server diffs must render. FileDiff's internals aren't + // queryable from jsdom; count the testid'd wrappers instead. + expect(canvas.getAllByTestId("edit-file-diff")).toHaveLength(2); + }, +}; + +export const EditFilesServerDiffNoOp: Story = { + args: { + name: "edit_files", + status: "completed", + args: { + files: [ + { + path: "src/unchanged.ts", + edits: [ + { + search: "same", + replace: "same", + }, + ], + }, + ], + }, + result: { + ok: true, + files: [{ path: "src/unchanged.ts", diff: "" }], + }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByText(/Edited unchanged\.ts/)).toBeInTheDocument(); + expect(canvas.queryAllByTestId("edit-file-diff")).toHaveLength(0); + }, +}; + +export const EditFilesFallbackToSynthetic: Story = { + args: { + name: "edit_files", + status: "completed", + args: { + files: [ + { + path: "src/legacy.ts", + edits: [ + { + search: "const timeout = 30;", + replace: "const timeout = 60;", + }, + ], + }, + ], + }, + result: { ok: true }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByText(/Edited legacy\.ts/)).toBeInTheDocument(); + expect(canvas.getAllByTestId("edit-file-diff")).toHaveLength(1); + }, +}; + +export const EditFilesServerDiffPartialFallback: Story = { + args: { + name: "edit_files", + status: "completed", + args: { + files: [ + { + path: "src/config.ts", + edits: [ + { + search: "const timeout = 30;", + replace: "const timeout = 60;", + }, + ], + }, + { + path: "src/server.ts", + edits: [ + { + search: 'const host = "localhost";', + replace: 'const host = "0.0.0.0";', + }, + ], + }, + ], + }, + result: { + ok: true, + files: [ + { + path: "src/config.ts", + diff: "--- src/config.ts\n+++ src/config.ts\n@@ -1,3 +1,3 @@\n export const settings = {\n-\tconst timeout = 30;\n+\tconst timeout = 60;\n };\n", + }, + ], + }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByText(/Edited 2 files/)).toBeInTheDocument(); + expect(canvas.getAllByTestId("edit-file-diff")).toHaveLength(2); + }, }; // --------------------------------------------------------------------------- diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx index d1a280459a..b7ce17794e 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx @@ -50,10 +50,13 @@ import { mapSubagentStatusToToolStatus, parseArgs, parseEditFilesArgs, + parseServerEditDiffText, + parseServerEditResults, stripNoNewline, type ToolStatus, toProviderLabel, } from "./utils"; + import { WriteFileTool } from "./WriteFileTool"; interface ToolProps extends Omit, "children"> { @@ -383,9 +386,17 @@ const EditFilesRenderer: FC = ({ }) => { const rec = asRecord(result); const editFiles = parseEditFilesArgs(args); - const editDiffs = editFiles.map((file) => - buildEditDiff(file.path, file.edits), - ); + // On error, render no diff: the agent rejected the edit, so a + // synthetic args-derived diff would misrepresent it as applied. + const serverResults = parseServerEditResults(result); + const editDiffs = isError + ? editFiles.map(() => null) + : editFiles.map((file) => { + const entry = serverResults?.find((d) => d.path === file.path); + return entry + ? parseServerEditDiffText(entry.diff) + : buildEditDiff(file.path, file.edits); + }); return ( { expect(diffViewerCSS).toContain("border-left"); }); }); + +describe("parseServerEditResults", () => { + it("returns null when result is not a record", () => { + expect(parseServerEditResults(null)).toBeNull(); + expect(parseServerEditResults(undefined)).toBeNull(); + expect(parseServerEditResults("foo")).toBeNull(); + }); + + it("returns null when files field is absent", () => { + expect(parseServerEditResults({ ok: true })).toBeNull(); + }); + + it("returns null when files is explicitly null (older agents)", () => { + expect(parseServerEditResults({ files: null })).toBeNull(); + }); + + it("returns an empty array when files is an empty array", () => { + expect(parseServerEditResults({ files: [] })).toEqual([]); + }); + + it("parses well-formed entries", () => { + const result = parseServerEditResults({ + files: [ + { + path: "/abs/a.txt", + diff: "--- /abs/a.txt\n+++ /abs/a.txt\n@@ -1 +1 @@\n-a\n+A\n", + }, + { path: "/abs/b.txt", diff: "" }, + ], + }); + expect(result).toEqual([ + { + path: "/abs/a.txt", + diff: "--- /abs/a.txt\n+++ /abs/a.txt\n@@ -1 +1 @@\n-a\n+A\n", + }, + { path: "/abs/b.txt", diff: "" }, + ]); + }); + + it("skips malformed entries without dropping the rest", () => { + const result = parseServerEditResults({ + files: [ + null, + { diff: "orphan" }, + { path: "", diff: "empty-path" }, + { path: "/ok", diff: "--- /ok\n+++ /ok\n" }, + ], + }); + expect(result).toEqual([{ path: "/ok", diff: "--- /ok\n+++ /ok\n" }]); + }); +}); + +describe("parseServerEditDiffText", () => { + it("returns null for an empty string (no-op edit)", () => { + expect(parseServerEditDiffText("")).toBeNull(); + }); + + it("parses a unified diff into a FileDiffMetadata", () => { + const diff = parseServerEditDiffText( + "--- /abs/a.txt\n+++ /abs/a.txt\n@@ -1 +1 @@\n-hello\n+HELLO\n", + ); + expect(diff).not.toBeNull(); + expect(diff?.name).toBe("/abs/a.txt"); + }); +}); diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts index 1fd11a3591..8fec771eeb 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts @@ -476,6 +476,20 @@ export const getFileContentForViewer = ( return { path, content }; }; +/** + * Parses a unified-diff string (with an optional SVN `Index:` + * banner) into the first FileDiffMetadata it contains. Returns + * null when the input is empty or the parser produces no files. + * Shared between the write_file diff builder, the synthetic + * edit_files diff builder, and the server-supplied diff parser. + */ +const parseSingleFileDiff = (raw: string): FileDiffMetadata | null => { + if (!raw) return null; + const parsed = parsePatchFiles(stripSvnIndexHeaders(raw)); + if (!parsed.length || !parsed[0].files.length) return null; + return parsed[0].files[0]; +}; + /** * Builds a FileDiffMetadata representing a new-file diff (all lines * are additions) from the content written by a write_file tool call. @@ -486,10 +500,7 @@ export const buildWriteFileDiff = ( content: string, ): FileDiffMetadata | null => { if (!content) return null; - const patch = Diff.createPatch(path, "", content, "", ""); - const parsed = parsePatchFiles(stripSvnIndexHeaders(patch)); - if (!parsed.length || !parsed[0].files.length) return null; - return parsed[0].files[0]; + return parseSingleFileDiff(Diff.createPatch(path, "", content, "", "")); }; /** @@ -568,11 +579,54 @@ export const buildEditDiff = ( patches.push(`--- ${diffPath}\n+++ ${diffPath}\n`); } - const parsed = parsePatchFiles(stripSvnIndexHeaders(patches.join(""))); - if (!parsed.length || !parsed[0].files.length) return null; - return parsed[0].files[0]; + return parseSingleFileDiff(patches.join("")); }; +/** + * Per-file result from the agent's FileEditResponse. `path` matches + * the caller-supplied path (pre-symlink resolution). `diff` is a + * unified-diff string, possibly empty for no-op edits. + */ +interface ServerEditResult { + path: string; + diff: string; +} + +/** + * Parses the structured `files` array from an edit_files tool + * response. The field is only populated when the agent observed the + * request's `include_diff` flag; older agents omit it entirely. + * Returns null when no per-file result array is present (callers + * should fall back to the synthetic client-side diff path). Returns + * an empty array when the field is explicitly present but empty. + */ +export const parseServerEditResults = ( + result: unknown, +): ServerEditResult[] | null => { + const rec = asRecord(result); + if (!rec) return null; + const raw = rec.files; + if (raw === undefined || raw === null) return null; + if (!Array.isArray(raw)) return null; + const results: ServerEditResult[] = []; + for (const entry of raw) { + const entryRec = asRecord(entry); + if (!entryRec) continue; + const path = asString(entryRec.path).trim(); + if (!path) continue; + results.push({ path, diff: asString(entryRec.diff) }); + } + return results; +}; + +/** + * Parses a server-supplied unified diff. Returns null for empty + * strings (no-op edits) or when the parser produces no file entries. + */ +export const parseServerEditDiffText = ( + diff: string, +): FileDiffMetadata | null => parseSingleFileDiff(diff); + /** * Converts an MCP-prefixed tool name into a human-readable label. * E.g. "linear__list_issues" with slug "linear" → "List issues"