Skip to content

fix(copilot): use explicit utf-8 for ACP file shim - #38121

Closed
xiao-wu-z wants to merge 1 commit into
NousResearch:mainfrom
xiao-wu-z:fix/copilot-acp-utf8-encoding
Closed

fix(copilot): use explicit utf-8 for ACP file shim#38121
xiao-wu-z wants to merge 1 commit into
NousResearch:mainfrom
xiao-wu-z:fix/copilot-acp-utf8-encoding

Conversation

@xiao-wu-z

Copy link
Copy Markdown

What does this PR do?

Uses explicit UTF-8 encoding for the Copilot ACP fs/read_text_file and fs/write_text_file shim so non-ASCII text is handled consistently even when the platform default encoding is not UTF-8.

Fixes #38119

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • Updated agent/copilot_acp_client.py to pass encoding="utf-8" for ACP text file reads/writes.
  • Added a regression test in tests/agent/test_copilot_acp_client.py that simulates a cp1252 default encoding and verifies non-ASCII content is written/read as UTF-8.

How to Test

  1. scripts/run_tests.sh tests/agent/test_copilot_acp_client.py
  2. .venv/bin/ruff check agent/copilot_acp_client.py tests/agent/test_copilot_acp_client.py

Verification

  • scripts/run_tests.sh tests/agent/test_copilot_acp_client.py: 8/8 passed
  • .venv/bin/ruff check agent/copilot_acp_client.py tests/agent/test_copilot_acp_client.py: All checks passed
  • git diff --check: no whitespace errors

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/acp Agent Communication Protocol adapter provider/copilot GitHub Copilot (ACP + Chat) labels Jun 3, 2026
@xiao-wu-z
xiao-wu-z marked this pull request as draft June 19, 2026 07:15
@xiao-wu-z
xiao-wu-z marked this pull request as ready for review June 19, 2026 07:15
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. The premise remains valid on current main: agent/copilot_acp_client.py:706 still calls path.read_text() without an encoding for fs/read_text_file, and agent/copilot_acp_client.py:735 does the same for path.write_text(...) in fs/write_text_file.

The PR commit 020aeef5873d3b3817524888324adcdf182c93e3 changes both exact handlers to explicit UTF-8 and its added test drives both ACP JSON-RPC methods through the existing dispatch helper under a simulated cp1252 default. A repository-wide search found these ACP file-method strings only in this shim and its focused test, so there is no unaddressed sibling ACP handler.

This is an automated hermes-sweeper review.

@teknium1

teknium1 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Closing — the ACP shim site is already explicit-utf-8 on current main (PLW1514 gate). Thanks.

@teknium1 teknium1 closed this Aug 8, 2026
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 P3 Low — cosmetic, nice to have provider/copilot GitHub Copilot (ACP + Chat) 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.

Copilot ACP fs read/write shim omits explicit UTF-8 encoding

3 participants