fix(cli): read and write CLI text files as UTF-8 - #6763
Open
LHMQ878 wants to merge 1 commit into
Open
Conversation
`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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
open()without an explicitencodinguses 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:
UnicodeDecodeError.Both are reproduced below on a zh-CN Windows box (
cp936).Reproduction
The silent variant:
"北京"as UTF-8 isE5 8C 97 E4 BA AC, and every one of those byte pairs is a legalcp936sequence, so that same read returns three wrong characters and no exception.Already visible in this repo's own test suite
On a
cp936machine, all 8 tests intests/unittests/cli/conformance/test_generate_markdown_utils.pyfail before this change: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 undersrc/google/adk/cli/:adk deployignore files + agent-engine configcli_deploy.py(2)adk evalsession input / scenarios / simulation configcli_tools_click.py(3)dev_server.py(2)agent_test_runner.py(3)api_server.py(2)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.pyandtools/load_artifacts_tool.py— those areZipFile.open, which is binary and takes noencoding.read_text()/write_text()calls elsewhere intests/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"onmain(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 futureopen()withoutencodingfails 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=. Patchinglocale.getpreferredencodingdoes not help either — CPython resolves the default text encoding internally andopen()ignores the patch. The tests therefore wrapbuiltins.opento force a non-UTF-8 codec onto any call that omitsencoding, which reproduces the locale deterministically on every platform:test_non_utf8_default_locale_helper_actually_bites— guards the guard: the helper must really changeopen()'s behavior, and an explicitencoding=must still win.test_locale_codec_can_corrupt_silently— documents failure mode 2 with a realcp936round trip: no exception, wrong text.test_get_ignore_patterns_reads_utf8_ignore_file— behavioral: a.gitignorewith a non-ASCII pattern parses correctly under a simulated non-UTF-8 locale.test_cli_package_has_no_implicit_encoding_open— AST scan overcli/, reportingfile:linefor 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:Results
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
mainon 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 theshutil.rmtreesignature).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.12andisort==8.0.1perpyproject.toml.