You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
EditTool reflows a whole file when its CRLF/LF endings are mixed #12792
EditTool rewrites every line ending in a file whose endings are mixed. A mostly-LF file with one CRLF line comes back entirely CRLF, so git diff shows the whole file changed when the model edited a single line.
A file on disk containing one\ntwo\nthree\r\nfour\n, with an edit of two -> TWO, is written back as one\r\nTWO\r\nthree\r\nfour\r\n. Two of those lines (one, four) were never touched, and the edited line silently gained a \r it did not have before.
What did you expect to happen?
Only the edited span changes; untouched lines keep their own bytes, so one stray CRLF cannot reflow the file:
one\nTWO\nthree\r\nfour\n
The LF normalisation in the read path is what makes matching and the confirmation diff line-ending agnostic, and that part works as intended. The problem is that it is allowed to reach disk.
Root cause
detectLineEnding returns one verdict for the whole file, and the verdict is "crlf if the content contains any\r\n" (packages/core/src/services/fileSystemService.ts:246). edit.ts:220 records it, edit.ts:222 normalises the content it holds in memory, and edit.ts:393 ships the verdict out as CalculatedEdit.lineEnding.
On write, prepareTextFileContent re-expands the whole content when that verdict is crlf (fileSystemService.ts:287-290), and ensureCrlfLineEndings (:236) is total: it converts every\n to \r\n. So one CRLF anywhere means all LF terminators get rewritten. The mirror case is the same code: a mostly-CRLF file with one LF line comes back uniformly CRLF.
The mechanism comes from f5349d80b9 ("preserve original line endings (CRLF/LF) when editing files", fixes #2704). A single per-file verdict preserves endings correctly for uniformly-CRLF and uniformly-LF files; a mixed file is the case it cannot express.
Reproduction
A regression test in packages/core/src/tools/edit.test.ts fails on main (1060b9e269):
FAIL src/tools/edit.test.ts > EditTool > execute > keeps untouched line endings when the file mixes CRLF and LF
AssertionError: expected 'one\r\nTWO\r\nthree\r\nfour\r\n' to be 'one\nTWO\nthree\r\nfour\n'
A second test in the same file — a uniform-CRLF file whose edit inserts a second line — passes on main and pins the behaviour that must not regress: inserted text takes the ending of the text it replaced.
How these were run (please read before judging the evidence)
pnpm install --frozen-lockfile cannot complete in my environment: three unrelated web-shell/mobile tarballs (echarts, mermaid, mobilecli) are unreachable, so npm run build is unavailable and scripts/vitest-global-setup.js — which refuses to run when packages/core/dist is missing — had to be bypassed. edit.test.ts imports nothing through a package specifier (every import is a relative path inside packages/core/src), so the dist/ build is not in its dependency graph. Everything else in the file ran unmodified against main; only the globalSetup guard was skipped.
Exact failure lines for reference:
AssertionError: expected 'one\r\nTWO\r\nthree\r\nfour\r\n' to be 'one\nTWO\nthree\r\nfour\n' // Object.is equality
❯ src/tools/edit.test.ts:959:49
Which behaviour should this have? (the actual decision)
Two ways out, and the choice is a policy call rather than a mechanical fix.
1. Preserve original bytes for untouched lines. Keep matching, the confirmation diff, the snippet and the telemetry on the LF text exactly as now, but build a separate on-disk payload from the original bytes: map normalised offsets back to raw offsets in one pass (the normalisation only removes \r before \n, so the map is monotonic), splice the original prefix and suffix verbatim, and convert only the inserted text's \n to the ending of the region it replaced. Pure-LF and pure-CRLF files stay byte-identical to today's output, so nothing changes outside the mixed case.
Costs: safeLiteralReplace (packages/core/src/utils/textUtils.ts:31) always replaces every match, so the mapping must cover all of them, not just the first; the write must stop passing _meta.lineEnding so the write path stops re-deriving endings; and needsCrlfLineEndings is platform- and extension-gated, so a mixed .ps1/.bat on Windows would still be flattened by prepareTextFileContent unless that path is handled too.
2. Normalise deliberately, and say so. Make detectLineEnding report crlf only when CRLF actually dominates (or is the first ending seen), so the file collapses to one style. One-line change, every file stays uniform — but it still reflows lines nobody asked to change, and it needs a decision about which style wins.
I lean towards (1): an edit should not rewrite the rest of the file. (2) is defensible if uniform endings are the intended invariant, in which case the tool description should warn the model so it can tell the user. Happy to be told which, and to send a PR for it — per CONTRIBUTING.md this issue is meant to be linked from that PR.
Client information
Could not paste /about output: running the installed client needs npm run build, which is unavailable here (see Reproduction). What I do have:
What happened?
EditToolrewrites every line ending in a file whose endings are mixed. A mostly-LF file with one CRLF line comes back entirely CRLF, sogit diffshows the whole file changed when the model edited a single line.A file on disk containing
one\ntwo\nthree\r\nfour\n, with an edit oftwo->TWO, is written back asone\r\nTWO\r\nthree\r\nfour\r\n. Two of those lines (one,four) were never touched, and the edited line silently gained a\rit did not have before.What did you expect to happen?
Only the edited span changes; untouched lines keep their own bytes, so one stray CRLF cannot reflow the file:
The LF normalisation in the read path is what makes matching and the confirmation diff line-ending agnostic, and that part works as intended. The problem is that it is allowed to reach disk.
Root cause
detectLineEndingreturns one verdict for the whole file, and the verdict is "crlf if the content contains any\r\n" (packages/core/src/services/fileSystemService.ts:246).edit.ts:220records it,edit.ts:222normalises the content it holds in memory, andedit.ts:393ships the verdict out asCalculatedEdit.lineEnding.On write,
prepareTextFileContentre-expands the whole content when that verdict iscrlf(fileSystemService.ts:287-290), andensureCrlfLineEndings(:236) is total: it converts every\nto\r\n. So one CRLF anywhere means all LF terminators get rewritten. The mirror case is the same code: a mostly-CRLF file with one LF line comes back uniformly CRLF.The mechanism comes from
f5349d80b9("preserve original line endings (CRLF/LF) when editing files", fixes #2704). A single per-file verdict preserves endings correctly for uniformly-CRLF and uniformly-LF files; a mixed file is the case it cannot express.Reproduction
A regression test in
packages/core/src/tools/edit.test.tsfails onmain(1060b9e269):A second test in the same file — a uniform-CRLF file whose edit inserts a second line — passes on
mainand pins the behaviour that must not regress: inserted text takes the ending of the text it replaced.How these were run (please read before judging the evidence)
pnpm install --frozen-lockfilecannot complete in my environment: three unrelated web-shell/mobile tarballs (echarts,mermaid,mobilecli) are unreachable, sonpm run buildis unavailable andscripts/vitest-global-setup.js— which refuses to run whenpackages/core/distis missing — had to be bypassed.edit.test.tsimports nothing through a package specifier (every import is a relative path insidepackages/core/src), so thedist/build is not in its dependency graph. Everything else in the file ran unmodified againstmain; only theglobalSetupguard was skipped.Exact failure lines for reference:
Which behaviour should this have? (the actual decision)
Two ways out, and the choice is a policy call rather than a mechanical fix.
1. Preserve original bytes for untouched lines. Keep matching, the confirmation diff, the snippet and the telemetry on the LF text exactly as now, but build a separate on-disk payload from the original bytes: map normalised offsets back to raw offsets in one pass (the normalisation only removes
\rbefore\n, so the map is monotonic), splice the original prefix and suffix verbatim, and convert only the inserted text's\nto the ending of the region it replaced. Pure-LF and pure-CRLF files stay byte-identical to today's output, so nothing changes outside the mixed case.Costs:
safeLiteralReplace(packages/core/src/utils/textUtils.ts:31) always replaces every match, so the mapping must cover all of them, not just the first; the write must stop passing_meta.lineEndingso the write path stops re-deriving endings; andneedsCrlfLineEndingsis platform- and extension-gated, so a mixed.ps1/.baton Windows would still be flattened byprepareTextFileContentunless that path is handled too.2. Normalise deliberately, and say so. Make
detectLineEndingreportcrlfonly when CRLF actually dominates (or is the first ending seen), so the file collapses to one style. One-line change, every file stays uniform — but it still reflows lines nobody asked to change, and it needs a decision about which style wins.I lean towards (1): an edit should not rewrite the rest of the file. (2) is defensible if uniform endings are the intended invariant, in which case the tool description should warn the model so it can tell the user. Happy to be told which, and to send a PR for it — per
CONTRIBUTING.mdthis issue is meant to be linked from that PR.Client information
Could not paste
/aboutoutput: running the installed client needsnpm run build, which is unavailable here (see Reproduction). What I do have:QwenLM/qwen-codeat1060b9e269("fix(serve): stop validating load-only restore fields on resume (fix(serve): stop validating load-only restore fields on resume #12768)")packageManagerdetectLineEndingandensureCrlfLineEndings, neither of which looks at the OS or the path.Login information
Not applicable — the test drives
EditTooldirectly through the tool test harness and never contacts an API.