fix(file): preserve CRLF during file edits#1953
fix(file): preserve CRLF during file edits#1953Kingwl wants to merge 3 commits intoMoonshotAI:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbae02fd92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Read the file content | ||
| content = await p.read_text(errors="replace") | ||
| content = await p.read_text(errors="replace", newline="") |
There was a problem hiding this comment.
Reading with newline="" keeps \r\n in content, but replacement still uses exact string matching. On CRLF files, an edit payload that uses LF ("...\n") no longer matches and returns “No replacements were made,” which regresses common agent/client behavior where generated text is LF-normalized. This means edits can fail even when the visible text matches; consider a compatibility fallback that normalizes line endings for matching while preserving original endings on write.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| newline: Newline = None, | ||
| ) -> str: | ||
| async with self._sftp.open(str(path), "r", encoding=encoding, errors=errors) as f: | ||
| return await f.read() |
There was a problem hiding this comment.
🔴 SSHKaos.readtext silently ignores newline parameter, breaking CRLF preservation over SSH
The newline parameter was added to SSHKaos.readtext to match the Kaos protocol (packages/kaos/src/kaos/__init__.py:184), but it is never forwarded to self._sftp.open(). The SFTP file is always opened with default (universal) newline handling, which normalizes \r\n to \n. When tools like ReadFile and StrReplaceFile pass newline="" to preserve CRLF line endings (as done in src/kimi_cli/tools/file/read.py:181 and src/kimi_cli/tools/file/replace.py:132), the request is silently ignored on the SSH backend. This means the entire CRLF-preservation fix introduced by this PR does not work over SSH — CRLF files will still have their line endings silently converted to LF when read, and StrReplaceFile edits will fail to match CRLF content.
Prompt for agents
The SSHKaos.readtext method accepts a newline parameter but never passes it to self._sftp.open(). This means CRLF line endings are always normalized to LF during reads over SSH, defeating the purpose of this PR.
The fix depends on whether asyncssh's SFTPClient.open() supports a newline parameter. If it does, simply pass newline=newline to the open() call at line 240. If it does not, you'll need to read the file as binary (mode='rb'), decode the bytes manually with the given encoding/errors, and return the result without newline translation. The same approach should be mirrored in readlines().
Files to check:
- packages/kaos/src/kaos/ssh.py: SSHKaos.readtext (line 232-241) and SSHKaos.readlines (line 243-254)
- asyncssh SFTPClient.open() API documentation for newline parameter support
Was this helpful? React with 👍 or 👎 to provide feedback.
| response = await self._client.read_text_file(path=abs_path, session_id=self._session_id) | ||
| return response.content |
There was a problem hiding this comment.
🔴 ACPKaos.readtext silently ignores newline when ACP client handles reads
When self._supports_read is True, ACPKaos.readtext calls self._client.read_text_file() which does not accept a newline parameter — the newline argument is silently discarded. According to src/kimi_cli/acp/AGENTS.md:84, ReadFile uses KaosPath.read_lines which ACPKaos implements via ACP reads. So when an ACP session reads a CRLF file with newline="" (as ReadFile and StrReplaceFile now do), the line-ending preservation requested by the caller is ignored if the ACP client normalizes line endings. This breaks the CRLF-preservation fix for IDE integrations that use ACP (e.g. Zed).
Prompt for agents
When self._supports_read is True, ACPKaos.readtext delegates to self._client.read_text_file() which has no newline parameter. The newline argument passed by callers like ReadFile (newline='') is silently ignored.
Possible approaches:
1. When newline is not None (i.e. caller requests non-default newline handling), fall back to self._fallback.readtext() which reads from the local filesystem and respects the newline parameter. This would work when the ACP client and server share a filesystem.
2. If ACP clients may be on a different machine, consider reading via readbytes() and decoding manually when newline='' is requested.
3. Extend the ACP read_text_file protocol to support a newline parameter.
File: src/kimi_cli/acp/kaos.py, method ACPKaos.readtext (line 227-241)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac3721daa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| newline: Newline = None, | ||
| ) -> str: | ||
| async with self._sftp.open(str(path), "r", encoding=encoding, errors=errors) as f: |
There was a problem hiding this comment.
SSHKaos.readtext() now accepts a newline argument, but the value is never passed into _sftp.open(). Callers introduced in this commit (ReadFile/StrReplaceFile) now rely on newline="" to preserve original line endings, so SSH-backed sessions still use backend defaults instead of the requested behavior and can continue normalizing CRLF content during read/edit flows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Nope. Model response should keep line feed
| if not self._supports_read: | ||
| return await self._fallback.readtext(abs_path, encoding=encoding, errors=errors) | ||
| return await self._fallback.readtext( | ||
| abs_path, encoding=encoding, errors=errors, newline=newline | ||
| ) | ||
| response = await self._client.read_text_file(path=abs_path, session_id=self._session_id) |
There was a problem hiding this comment.
In ACPKaos.readtext(), the new newline parameter is only honored in the local fallback branch; when ACP fs.readTextFile is available, the method returns response.content directly and ignores newline. This makes the CRLF-preservation behavior backend-dependent, so file-edit/read calls that pass newline="" can still lose expected line-ending semantics depending on the ACP client implementation.
Useful? React with 👍 / 👎.
…ile-edits # Conflicts: # CHANGELOG.md # docs/en/release-notes/changelog.md # docs/zh/release-notes/changelog.md
1 participant
Related Issue
Fixed #1952
Description
Summary
Fix CRLF line-ending churn in file tools.
ReadFileandStrReplaceFilenow preserve original line endings when reading files, so editing a CRLF file no longer rewrites the whole file as LF. KAOS text read APIs now accept anewlineparameter and pass it through local/SSH/ACP backends.Changes
newlinesupport to KAOSreadtext/readlinesAPIs.ReadFileandStrReplaceFile.ReadFile, andStrReplaceFile.Testing
make gen-changelogmake gen-docsuv run pytest packages/kaos/tests/test_local_kaos.py tests/tools/test_read_file.py tests/tools/test_str_replace_file.pymake format-pykaosmake check-pykaosmake format-kimi-climake check-kimi-cliChecklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.