Skip to content

fix(acp): normalize CRLF/CR to LF in decoded text resources - #80690

Open
Cossackx wants to merge 1 commit into
NousResearch:mainfrom
Cossackx:fix/acp-normalize-decoded-newlines
Open

fix(acp): normalize CRLF/CR to LF in decoded text resources#80690
Cossackx wants to merge 1 commit into
NousResearch:mainfrom
Cossackx:fix/acp-normalize-decoded-newlines

Conversation

@Cossackx

@Cossackx Cossackx commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Problem

Editors on Windows attach CRLF files over ACP. _decode_text_bytes passed the raw \r bytes through, so stray carriage returns flowed into prompt context and downstream diffs (visible as ^M artifacts and wasted tokens).

Fix

Normalize decoded text to LF (\r\n\n, then bare \r\n) at the decode boundary. Binary detection and truncation behavior are unchanged.

The encoding loop is restructured into for … break … else so normalization happens once on the way out, rather than being written twice. (The trailing errors="replace" arm is preserved as-is; note it is already unreachable on main, since latin-1 decodes every byte and never raises — this PR neither fixes nor relies on that.)

Tests

Five new tests in tests/acp/test_server.py: CRLF + bare-CR normalization, UTF-8-sig content, binary rejection unchanged, LF-only passthrough, and a NUL byte kept when the mime type forces text. Full file green on native Windows 11: 35 passed.

Platforms tested: native Windows 11 (Python 3.11); Linux covered by CI — the change is platform-independent.

Related: #42775 also normalizes CRLF in _decode_text_bytes as part of a broader Windows-compatibility patch. This PR is narrower and additionally normalizes bare CR, with dedicated decode-boundary coverage. #40649 handles the sibling native file-URI path issue.

🤖 Generated with Claude Code

Editors on Windows attach CRLF files over ACP. _decode_text_bytes passed
the raw \r bytes through, embedding stray carriage returns into prompt
context and downstream diffs. Normalize decoded text to LF at the decode
boundary; binary detection and truncation behavior are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P4 Best-effort: we will get to it when we get to it (no commitment) comp/acp Agent Communication Protocol adapter platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 7, 2026
@spfcraze

spfcraze commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary:
The description states no existing PR covers CRLF normalization, but open PR #42775 already normalizes CRLF in _decode_text_bytes — the same function this PR changes.

Problems:

Solution:
Link #42775 in the description so the overlap is visible; both PRs touch the same function.


Checked against 369d94c — the tip of fix/acp-normalize-decoded-newlines when this was written.

@Cossackx

Copy link
Copy Markdown
Contributor Author

Thanks — you’re right that #42775 already covers the CRLF case, and I missed that overlap. I’ve updated the description to link it.

I don’t think this is an exact duplicate: #42775 normalizes CRLF as one part of a broader Windows patch, while this PR is scoped to text decoding, also normalizes bare CR, and adds focused coverage for UTF-8 BOM decoding, binary rejection, LF-only input, and MIME-forced text.

Given the focused scope, clean merge state, and green checks, I’d prefer to keep this open as the standalone newline fix. I’m happy to close it if maintainers prefer #42775 to own the behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P4 Best-effort: we will get to it when we get to it (no commitment) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants