Skip to content

fix: add UTF-8 encoding to read_text/write_text in Copilot ACP fs handlers - #64070

Closed
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/copilot-acp-write-text-encoding
Closed

fix: add UTF-8 encoding to read_text/write_text in Copilot ACP fs handlers#64070
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/copilot-acp-write-text-encoding

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Path.read_text() and Path.write_text() in the Copilot ACP client's fs/read_text_file and fs/write_text_file JSON-RPC handlers omit explicit encoding=. On Windows, these default to the system locale (typically cp1252), silently corrupting non-ASCII content — source code with unicode identifiers, CJK comments, emoji in strings, etc.

Changes

Line Before After
706 path.read_text() path.read_text(encoding="utf-8")
735 path.write_text(str(params.get("content") or "")) path.write_text(str(params.get("content") or ""), encoding="utf-8")

Why this matters

The Copilot ACP client acts as a file I/O bridge for external coding agents. Any non-ASCII content written through this bridge on Windows gets silently corrupted. The corruption is invisible until someone reads the file and sees mojibake.

Follows the same pattern as PR #56940 (os.fdopen encoding) and PR #62667 (write_text + ensure_ascii=False).

Test Plan

  • Existing tests pass: pytest tests/agent/test_copilot_acp_client.py -xvs
  • Verified no false positives — these calls have no kwargs dict or encoding on adjacent lines

The fs/read_text_file and fs/write_text_file handlers in the Copilot
ACP client use Path.read_text() and Path.write_text() without explicit
encoding. On Windows, these default to the system locale encoding
(typically cp1252), which corrupts non-ASCII content (e.g. source code
with unicode identifiers, comments in CJK languages, emoji in strings).

Since ensure_ascii is not used in any surrounding json.dumps and the
content is arbitrary user code, explicit encoding="utf-8" is required
for cross-platform correctness.
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jul 14, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #38121 (the earliest open PR making the identical fix). This PR pins encoding="utf-8" on the exact same two fs/read_text_file / fs/write_text_file calls in agent/copilot_acp_client.py that #38121 (and sibling #38123) already fix; the closed #62090 was previously deduped to #38121 as well. Minor test-mock/comment differences don't change that. Consolidating on #38121 as canonical; linking #38119 (issue) and #38123 (sibling) for context.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

This PR adds UTF-8 encoding to read_text/write_text in Copilot ACP fs handlers. Small, targeted fix.

Please verify:

  • UTF-8 encoding is applied correctly for both read and write operations
  • Existing tests pass with the encoding change

Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (token read-only)

PR 64070: fix: add UTF-8 encoding to read_text/write_text in Copilot ACP fs handlers

Correctness

  • Adds explicit encoding="utf-8" to path.read_text() and path.write_text() calls.
  • Addresses potential encoding issues on Windows (where the default encoding may differ).

No issues found.


Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (token read-only)

PR 64070: fix: add UTF-8 encoding to read_text/write_text in Copilot ACP fs handlers

Correctness

  • Adds explicit encoding="utf-8" to path.read_text() and path.write_text() calls.
  • Addresses potential encoding issues on Windows (where the default encoding may differ).

No issues found.


Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

Changes

Explicit encoding="utf-8" on path.read_text() and path.write_text() in copilot_acp_client.py file operations.

Assessment

  • Minor robustness fix. Explicit encoding prevents platform-dependent default encoding issues.
  • No debug artifacts.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cross-platform fix. The premise is valid on current main: agent/copilot_acp_client.py:706 and agent/copilot_acp_client.py:734 still use locale-default Path.read_text() and Path.write_text() in the two ACP filesystem handlers, and the proposed explicit UTF-8 arguments address those exact calls.

Problems

  • This PR changes only agent/copilot_acp_client.py; it adds no regression test. The current focused test dispatch helper is at tests/agent/test_copilot_acp_client.py:130-142, and its ACP filesystem cases at :158-256 cover safety behavior rather than a successful non-ASCII read/write.
  • This is duplicate work: open PR fix(copilot): use explicit utf-8 for ACP file shim #38121 contains the same fix in commit 020aeef5873d3b3817524888324adcdf182c93e3 and has already been reviewed as including the ACP UTF-8 regression coverage.

Suggested changes

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as resolved by PR #71078 (merged, commit d372fda): the class-wide close-out salvaged your #50655/#54241/#56385/#66856/#65440 series as the backbone (authorship preserved in git log) and swept the remaining sites, so every read_text/write_text call this PR touches is now guarded on current main — verified per-site. A CI linter rule in check-windows-footguns.py plus the AST guard test now prevent regressions. Your overlapping/split variants of the same series are being closed together; the credit for the class rests on your commits.

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

4 participants