Skip to content

fix(cli): read and write CLI text files as UTF-8 - #6763

Open
LHMQ878 wants to merge 1 commit into
google:mainfrom
LHMQ878:fix/cli-utf8-encoding
Open

fix(cli): read and write CLI text files as UTF-8#6763
LHMQ878 wants to merge 1 commit into
google:mainfrom
LHMQ878:fix/cli-utf8-encoding

Conversation

@LHMQ878

@LHMQ878 LHMQ878 commented Aug 17, 2026

Copy link
Copy Markdown

Description

open() without an explicit encoding uses the platform's locale encoding. That is not UTF-8 on a large share of Windows installs — cp936 (zh-CN), cp932 (ja-JP), cp1252 (much of the West) — while the CLI's JSON and Markdown files carry agent prompts, model responses and display names, so they routinely hold non-ASCII text. JSON is additionally defined as UTF-8 for interchange (RFC 8259 §8.1), so decoding it through a locale codec is wrong independently of platform.

There are two failure modes, and the quieter one is worse:

  1. Loud — the byte sequence is invalid in the locale codec and the read raises UnicodeDecodeError.
  2. Silent — the UTF-8 bytes happen to form a valid sequence in the locale codec, so the read succeeds and yields mojibake, which then gets written back into the eval set or test file.

Both are reproduced below on a zh-CN Windows box (cp936).

Reproduction

import json, locale, os, tempfile
print("locale:", locale.getpreferredencoding(False))   # -> cp936

payload = {"events": [{"author": "user",
                       "content": {"parts": [{"text": "请帮我查一下北京的天气"}]}}]}
p = os.path.join(tempfile.mkdtemp(), "test_zh.json")
with open(p, "w", encoding="utf-8") as f:
    json.dump(payload, f, ensure_ascii=False, indent=2)

with open(p, "r") as f:        # how the CLI reads it today
    json.load(f)
locale: cp936
UnicodeDecodeError: 'gbk' codec can't decode byte 0x80 in position 138: illegal multibyte sequence

The silent variant: "北京" as UTF-8 is E5 8C 97 E4 BA AC, and every one of those byte pairs is a legal cp936 sequence, so that same read returns three wrong characters and no exception.

Already visible in this repo's own test suite

On a cp936 machine, all 8 tests in tests/unittests/cli/conformance/test_generate_markdown_utils.py fail before this change:

E   UnicodeDecodeError: 'gbk' codec can't decode byte 0x85 in position 363: illegal multibyte sequence
...\Lib\pathlib.py:1028: UnicodeDecodeError
7 failed, 1 passed

The report was written through the locale, and the test read it back through the locale too. Both sides are fixed here, and all 8 now pass.

Changes

encoding="utf-8" at the 13 affected call sites under src/google/adk/cli/:

Area File
adk deploy ignore files + agent-engine config cli_deploy.py (2)
adk eval session input / scenarios / simulation config cli_tools_click.py (3)
dev-server test-file create/read endpoints dev_server.py (2)
agent test runner replay + rebuild agent_test_runner.py (3)
API server runtime config read/write api_server.py (2)
conformance Markdown report conformance/_generate_markdown_utils.py (1)

Plus, for the invariant to hold package-wide, the two telemetry lock-file sites (_metrics_collector.py, _metrics_reporter.py). These two are not bugs — the lock file holds an ASCII float timestamp — and are called out as consistency-only.

Deliberately not touched:

  • skills/_utils.py and tools/load_artifacts_tool.py — those are ZipFile.open, which is binary and takes no encoding.
  • The ~170 locale-dependent read_text()/write_text() calls elsewhere in tests/unittests/cli/. Only the one that reads a file written by code changed here is fixed, to keep this diff reviewable; the rest is a separate cleanup.

Relationship to existing work

This defect class has been fixed piecemeal at least four times — #2049, #5820, #6288, #6298 — and #6690 is open for the evaluation module. #6690 now appears stale: the four sites it targets already carry encoding="utf-8" on main (agent_evaluator.py:97,368,379,427, evaluation_generator.py:599), presumably landed internally via Copybara.

Because the same class keeps returning, the regression test asserts it for all of cli/ with an AST scan rather than per call site, so a future open() without encoding fails CI wherever it is added.

Testing plan

tests/unittests/cli/test_cli_utf8_encoding.py (new, 4 tests).

CI runs on a UTF-8 default, so a test that merely reads a UTF-8 file cannot catch a missing encoding=. Patching locale.getpreferredencoding does not help either — CPython resolves the default text encoding internally and open() ignores the patch. The tests therefore wrap builtins.open to force a non-UTF-8 codec onto any call that omits encoding, which reproduces the locale deterministically on every platform:

  • test_non_utf8_default_locale_helper_actually_bites — guards the guard: the helper must really change open()'s behavior, and an explicit encoding= must still win.
  • test_locale_codec_can_corrupt_silently — documents failure mode 2 with a real cp936 round trip: no exception, wrong text.
  • test_get_ignore_patterns_reads_utf8_ignore_file — behavioral: a .gitignore with a non-ASCII pattern parses correctly under a simulated non-UTF-8 locale.
  • test_cli_package_has_no_implicit_encoding_open — AST scan over cli/, reporting file:line for any offender.

Verified these actually fail without the fix. Reverting a single site (cli_deploy.py:646) fails two of them and names the exact line:

FAILED tests/unittests/cli/test_cli_utf8_encoding.py::test_get_ignore_patterns_reads_utf8_ignore_file
FAILED tests/unittests/cli/test_cli_utf8_encoding.py::test_cli_package_has_no_implicit_encoding_open
E   AssertionError: text-mode open() without encoding= (locale-dependent, breaks on
    non-UTF-8 locales): ['...\src\google\adk\cli\cli_deploy.py:646']
2 failed, 2 passed

Results

$ pytest tests/unittests/cli/test_cli_utf8_encoding.py \
         tests/unittests/cli/conformance/test_generate_markdown_utils.py -q
12 passed, 4 warnings in 3.98s

Full CLI suite on this machine, before → after: 19 failed, 850 passed → 12 failed, 857 passed (7 fixed, 0 new failures).

The 12 remaining failures are pre-existing on main on Windows and unrelated to encoding — verified by re-running them on a clean checkout:

  • test_cli_deploy_to_cloud_run.py (10) — TypeError: <lambda>() got an unexpected keyword argument 'onexc' (a test double not matching the shutil.rmtree signature).
  • test_path_normalizer.py (1) — assert 'tools\web.yaml' == 'tools/web.yaml' (path-separator assumption).
  • test_cleanup_unused_files.py (1).

Formatted with pyink==25.12 and isort==8.0.1 per pyproject.toml.

`open()` without an explicit `encoding` uses the platform's locale encoding.
That is not UTF-8 on a large share of Windows installs (`cp936` for zh-CN,
`cp932` for ja-JP, `cp1252` for much of the West), while the CLI's JSON and
Markdown files carry agent prompts, model responses and display names, so they
routinely hold non-ASCII text. JSON is also defined as UTF-8 for interchange
(RFC 8259 8.1), so reading it through a locale codec is wrong regardless of
platform.

Two failure modes follow, and the quieter one is worse: the read either raises
`UnicodeDecodeError`, or -- when the UTF-8 bytes happen to form a valid sequence
in the locale codec -- silently decodes to mojibake and persists it back into
the eval set or test file.

Pass `encoding="utf-8"` at the 13 affected call sites in `cli/`, covering
`adk deploy` config and ignore files, `adk eval` session/scenario inputs, the
dev-server test-file endpoints, the agent test runner, the API server runtime
config and the conformance Markdown report. The two telemetry lock-file sites
hold an ASCII timestamp and are unaffected in practice; they are included so the
invariant holds for the whole package.

This is the same defect class fixed piecemeal before (google#2049, google#5820, google#6288,
google#6298) and still open for the evaluation module in google#6690, so the new tests
assert it for `cli/` as a whole via an AST scan rather than per call site.

`tests/unittests/cli/conformance/test_generate_markdown_utils.py` read the
report back through the locale too, so all 8 of its tests failed on a cp936
machine; it now reads UTF-8 and passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants