Pin text I/O to UTF-8 and fail CI on locale-dependent reads/writes - #3296
Pin text I/O to UTF-8 and fail CI on locale-dependent reads/writes#3296maxisbey wants to merge 10 commits into
Conversation
Path.read_text() without an encoding argument uses the locale preferred encoding, which on Windows is the ANSI code page (e.g. GBK on Chinese-locale systems). manifest.toml and the story sources are UTF-8, so tests/examples failed to collect or run there. Fixes #3244 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Text-mode open()/Path.read_text()/write_text() without encoding= use the process locale, which is the ANSI code page on Windows. CI can't see the difference (ubuntu is UTF-8, windows-latest is cp1252 and happens to decode our em dashes), but a cp936/cp932/cp949 machine raises, and `mcp install` would read claude_desktop_config.json with the wrong codec and write any pre-existing non-ASCII entries back as mojibake. - Pass encoding="utf-8" at every remaining call site (cli, stories harness, scripts, tests, examples); subprocess captures decode as UTF-8 with errors="replace"; drop a non-ASCII glyph from a script's piped output. - Run pytest with PYTHONWARNDEFAULTENCODING=1 in CI and scripts/test so any future omission raises EncodingWarning under the existing "error" filter, run the two repo scripts with -X warn_default_encoding, and enable ruff's PLW1514 as the edit-time counterpart. - test_other_servers_preserved now seeds non-ASCII entries so the config round-trip is covered. Follows #3245.
📚 Documentation preview
|
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Beyond the inline nits, two candidates were examined and ruled out this run: (1) gen_surface_types.py --check writing the drift diff through sys.stderr.writelines on a non-UTF-8 console — that path only executes when the check is already failing (exit 1 either way), and CI's stderr is UTF-8; (2) the encoding-less read_text() at tests/docs_src/test_uri_templates.py:132 — it is docstring shorthand referencing tutorial002 (whose real code this PR fixes), not runnable guidance.
Extended reasoning...
All five posted findings are nit-level (comment accuracy, forward-looking enforcement-coverage gaps, a BOM edge case, and a docstring copy of the old pattern); none indicates incorrect behavior in the shipped code, and the core change — adding encoding="utf-8" at every call site plus the PEP 597 / PLW1514 guards — is mechanically consistent and well tested per the PR description. This note only records the two additional candidates that finder agents raised and verifiers refuted, so a later review pass does not re-explore them from scratch; it is informational and not a correctness guarantee. Given the PR's breadth (21 files, repo-wide lint/CI policy, and the user-facing mcp install path), the inline comments plus a human maintainer's judgment on the enforcement-policy choices are the right gate, so no approval is issued.
…ruff preview note - scripts/test: forward arguments as "$@". - scripts/update_readme_snippets.py: a per-block error now exits non-zero instead of printing and keeping the stale block, which let --check pass; the missing-file special case that worked around that goes away. - pyproject.toml: say what lint.preview actually turns on. - path_security docstring: the copy-paste example passes encoding= too.
…te a BOM in the Claude config - pyproject.toml: remove PLW1514 and lint.preview. Preview mode also changes stable rules on ruff upgrades (0.15.x flags a load-bearing `import mcp` as a fixable F401), which costs more than an incomplete edit-time check buys; the EncodingWarning gate is the enforcement. - .pre-commit-config.yaml: run the README snippet check with the same -X warn_default_encoding -W error::EncodingWarning flags as CI. - mcp install: read claude_desktop_config.json as utf-8-sig so a BOM written by Windows tooling doesn't make a valid config unreadable; the rewrite stays plain UTF-8. The existing test seeds a BOM.
There was a problem hiding this comment.
I re-reviewed the latest revision (b8e97a0 + dbf7dca) and found no new issues — the fixes for the earlier inline findings all check out. Since this PR sets repo-wide CI enforcement policy (the PEP 597 gates in the workflow, pre-commit, and scripts/test) and touches the mcp install path, a human sign-off is still worthwhile.
What was reviewed this pass:
- The full encoding= sweep against the diff — every call site pins utf-8 (utf-8-sig on the Claude config read, with the BOM round-trip now covered by test_other_servers_preserved).
- The reworked except OSError → sys.exit handler in update_readme_snippets.py: a UnicodeDecodeError or an escalated EncodingWarning is not caught by it, but both propagate and exit non-zero, so --check stays fail-closed either way.
- Flag parity between the CI steps, the pre-commit hook, and scripts/test, plus the scoped pytest_examples EncodingWarning ignore.
Extended reasoning...
Overview
This PR pins text I/O to UTF-8 across 21 files: the one user-facing site (src/mcp/cli/claude.py, which mcp install uses to read/write claude_desktop_config.json), two repo scripts, the stories harness, an example client, a docs snippet, and a spread of tests. It also arms enforcement: PYTHONWARNDEFAULTENCODING=1 on the pytest matrix and scripts/test (fatal via the existing filterwarnings error filter), and -X warn_default_encoding -W error::EncodingWarning on the two gated script invocations in CI and the matching pre-commit hook.
Security risks
None identified. The claude.py change only alters the codec used to decode/encode an existing local config file; the write path still goes through json.dumps with ASCII escaping, so output bytes are unchanged for pre-existing ASCII content. utf-8-sig is a strict read-superset of utf-8. No auth, network, or path-handling behavior changes.
Level of scrutiny
The individual edits are mechanical (adding encoding= arguments), but the PR is large and makes repo policy decisions: which surfaces the CI guarantee covers (the author deliberately scoped it to the test suite and the two gated scripts, leaving the docs build and example packages out), dropping the ruff PLW1514 preview rule after the stable-rule preview-behavior cost was demonstrated, and the utf-8-sig read / plain-utf-8 write asymmetry (a BOM'd config comes out BOM-free, which the author flagged explicitly). Those are judgment calls a maintainer should own, and one earlier nit (the AGENTS.md targeted-check snippet not setting the env var) is still open — so this is a defer rather than a shadow-approval.
Other factors
All four substantive findings from earlier review rounds were fixed in b8e97a0/dbf7dca8 and verified against the current diff. The one candidate raised this run — that except OSError does not catch UnicodeDecodeError, contrary to the author's comment — was verified and refuted as a bug: UnicodeDecodeError subclasses ValueError, not OSError, so it propagates uncaught and the script exits non-zero, meaning --check fails closed either way; only the comment's stated mechanism is slightly off. Test coverage is strong: the upgraded test_other_servers_preserved seeds non-ASCII entries plus a BOM and fails on the pre-PR code under cp1252, GBK, and the warning guard, and the PR description documents manual verification on Windows runners at code pages 1252 and 936.
There was a problem hiding this comment.
I re-reviewed the revision with the fix-up commits (b8e97a0, dbf7dca, 9572a85) and found no new bugs; given the breadth of the change (CI enforcement mechanics plus the mcp install read path), a human look before merge is still worthwhile.
What was reviewed this pass:
- The reworked
except OSErrorhandler inupdate_readme_snippets.py: a candidate issue that it no longer catchesUnicodeDecodeError(aValueErrorsubclass) or an escalatedEncodingWarningwas examined and ruled out — both propagate uncaught, so--checkstill exits nonzero (fail-closed, just via traceback rather than the tidysys.exitmessage). - The utf-8-sig read + plain-utf-8 write round-trip in
claude.pyand the upgraded BOM/non-ASCII test. - The lenient stderr decode in 9572a85, consistent with the other two capture sites.
Extended reasoning...
Overview
This pass covers the PR at 9572a85, i.e. after the author addressed the earlier review findings: "$@" quoting in scripts/test, the docstring example in path_security.py, the swallowed-warning handler in update_readme_snippets.py (now except OSError → sys.exit), the pre-commit hook flags, dropping the preview-only PLW1514 config entirely, and the utf-8-sig read of the Claude Desktop config with a BOM-seeding test. The bug hunting system found no new bugs on this revision.
Security risks
None identified. The one shipped-code path touched is src/mcp/cli/claude.py, where the change is strictly a decoding fix (locale codec → utf-8-sig read, explicit utf-8 write); the JSON structure handling is unchanged and json.dumps still ASCII-escapes output. The path_security.py change is docstring-only.
Level of scrutiny
Moderate-to-high, and not self-approvable: 21 files spanning CI workflow enforcement (PYTHONWARNDEFAULTENCODING, -X warn_default_encoding), pre-commit, repo scripts, and a user-facing CLI code path. The mechanism design (where the warning guard does and does not apply) involves judgment calls the author has documented in-thread but that a maintainer should sign off on. There is also one still-open nit from a prior pass (the AGENTS.md targeted-check snippet) that the author has not yet responded to.
Other factors
The candidate issue raised this run — that except OSError contradicts the fix commit's claim about UnicodeDecodeError — was verified to be a non-bug: neither UnicodeDecodeError nor an exception-ified EncodingWarning is an OSError subclass, so both escape the handler and crash the script with a nonzero exit, preserving the fail-closed property the handler rework was meant to establish. Test coverage for the user-visible behavior change is direct (test_other_servers_preserved now seeds non-ASCII entries plus a BOM), and the author reports green runs across cp1252/cp936/GBK environments. Not approving due to size, CI-mechanism scope, and the open comment; deferring rather than staying silent because the fix-up commits post-date the prior review bodies and this is the first pass to confirm them clean.
…oding - Drop the -X warn_default_encoding wrappers from the two script steps and the pre-commit hook: the pytest step and scripts/test are the guard, the scripts' own reads are pinned, and an interpreter flag every entry point has to remember has no owner. The README script still fails closed. - mcp install: json.loads(config_file.read_bytes()), so BOM'd UTF-8 and the UTF-16 that PowerShell 5's `>` writes are detected by the stdlib rather than by a codec choice here; a separate test covers both, and test_other_servers_preserved goes back to stating one behaviour. - tests/test_types.py: run the child with -X utf8 and decode strictly, since its stdout is the assertion subject. - AGENTS.md: the targeted-coverage snippet sets PYTHONWARNDEFAULTENCODING=1 like scripts/test, and the rule names encoding="locale" as the deliberate escape hatch.
There was a problem hiding this comment.
Beyond the two inline nits, this run also examined and ruled out one candidate: that the stricter config read in src/mcp/cli/claude.py would newly reject a locale-(ANSI-)encoded claude_desktop_config.json the old locale-codec read accepted. It does not regress: the read is now json.loads(config_file.read_bytes()), whose detect_encoding handles UTF-8/16/32 (with or without BOM), and an ANSI-encoded config was already broken or mojibake'd under the old locale read on any non-matching machine — the test additions cover the utf-8-sig and utf-16 cases directly.
Extended reasoning...
This run's two findings are documentation-only nits already posted as inline comments; nothing here restates them. The one candidate worth recording is the locale-encoded-config concern in claude.py, which a verifier refuted: json.loads over raw bytes covers the Unicode family of encodings, ANSI-encoded configs were not readable correctly before this PR either (locale-dependent, mojibake or decode error), and the PR's parametrized BOM test (utf-8-sig, utf-16) pins the newly supported cases. Recording it so a later review pass does not re-explore it. This note is informational only and not a correctness guarantee.
…est and CI carry it
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
CONTRIBUTING.md:89-93— CONTRIBUTING step 4 now says./scripts/test"runs them the way CI does", but the script's last line runsstrict-no-coverunconditionally while CI gates that same check behindif: runner.os != 'Windows'— so a Windows contributor following the documented workflow gets pytest+coverage green, then a spurious strict-no-cover failure on win32-only pragmas in files they never touched. Fix by gating the strict-no-cover line in scripts/test on the OS the way CI does (e.g. skip whenunamereports MINGW/MSYS), or by softening the parity claim with a Windows caveat.Extended reasoning...
What the bug is. This PR's commit 1bc8670 changes CONTRIBUTING.md Development Workflow step 4 from the cross-platform
uv run pytestto./scripts/test, with the parenthetical "scripts/test runs them the way CI does, with coverage." That parity claim does not hold on Windows:scripts/testline 12 runsUV_FROZEN=1 uv run --frozen strict-no-coverunconditionally underset -ex, while CI runs the identical check behind an explicit per-OS gate —.github/workflows/shared.yml, step "Check for unnecessary no cover pragmas",if: runner.os != 'Windows'.Why the gate exists (and why the script fails without it).
strict-no-coverfails whenever a line marked# pragma: no coveris actually executed — AGENTS.md documents this exactly: "CI'sstrict-no-cover(skipped on Windows runners) fails if it IS executed." The codebase marks Windows-only branches with plain# pragma: no cover(notlax no cover, so strict-no-cover checks them) precisely because they never run on the non-Windows platforms where the check runs:src/mcp/client/stdio.py:296,322,339andsrc/mcp/server/stdio.py:74,100,152are allif sys.platform == \"win32\": # pragma: no cover. Neither file is in the coverage omit list (onlysrc/mcp/os/{posix,win32}/utilities.pyare). On a Windows box the full suite exercises those branches — the stdio subprocess tests (tests/client/test_stdio.py,tests/interaction/transports/test_stdio.py) drive_get_executable_commandandcreate_windows_process, and both run on the Windows CI cells — so coverage records hits on excluded lines and strict-no-cover fails.Step-by-step proof. (1) A contributor on a Windows machine — the exact audience this PR exists to protect, per its own motivation about cp936/cp1252 code pages — opens Git Bash (the normal way to run a
#!/bin/shscript there) and runs./scripts/testper the new step 4. (2) pytest passes and the coverage report passes — CI's Windows matrix cells prove those steps are Windows-clean. (3) coverage data now records hits on the win32-only# pragma: no coverlines above. (4) The script's final line runs strict-no-cover, which flags those pragmas as "excluded but executed" and exits non-zero; underset -ethe whole script fails. (5) The contributor sees step 4 fail on pragmas in files they never touched, right after being told the script "runs them the way CI does" — while CI itself would have skipped that check on their platform.Why nothing catches this. CI never executes
scripts/test— the workflow inlines the coverage commands and applies the per-OS gate itself — and the test runs reported on this PR were on Linux (./scripts/testgreen) or onwindows-latestvia the gated CI steps. The divergence is only observable on a contributor's own Windows machine.Not a duplicate of the refuted bug_001. That claim was that the script cannot be run at all from cmd/PowerShell, and its proposed fix (a "use Git Bash" note) would route contributors straight into this failure. This finding is about what happens when the script does run: it enforces strictly more than CI does for Windows, contradicting the new parity claim.
Impact and fix. Nothing shipped breaks and CI is unaffected — the cost is one confusing local failure per Windows contributor following the primary contributor doc, hence not merge-blocking. Fix is small: gate the last line of
scripts/testthe way CI does (e.g.case \"$(uname -s)\" in MINGW*|MSYS*|CYGWIN*) ;; *) UV_FROZEN=1 uv run --frozen strict-no-cover ;; esac), or soften the CONTRIBUTING parenthetical to note that on Windows the CI-equivalent run is the coverage steps without strict-no-cover.
…or as a missing one
There was a problem hiding this comment.
LGTM — the sweep is mechanical, the one user-facing change (mcp install config decoding) is well-tested, and every point raised across the earlier review passes has been addressed in the final revision.
What was reviewed this run:
- Re-checked all 20 files at the final commit: no
encoding=-lessread_text/write_text/open/tempfile/subprocesstext call remains insrc/, scripts, or the touched tests. claude.py'sjson.loads(read_bytes())read: stdlibdetect_encodingcovers UTF-8, BOM'd UTF-8, and UTF-16; the plain-UTF-8 rewrite plus the BOM/UTF-16 and non-ASCII preservation tests match the stated behavior.- The
pytest_examples-scopedEncodingWarningignore and the CI/scripts/testenv-var placement: correctly scoped, and"$@"quoting is fixed. update_readme_snippets.py'sexcept (OSError, UnicodeDecodeError)→sys.exithandler: fails closed (non-zero) for--checkon missing and mis-encoded sources alike.
Extended reasoning...
Overview
This PR pins all text I/O in the repo to explicit encodings (encoding="utf-8", or errors="replace" on diagnostic subprocess captures) across 20 files — CI workflow, two maintainer scripts, the stories harness, one docs snippet, an example client, tests, and src/mcp/cli/claude.py — and arms PYTHONWARNDEFAULTENCODING=1 (PEP 597) in CI and scripts/test so the existing filterwarnings = ["error"] makes any future omission fatal. The only user-facing behavior change is mcp install reading claude_desktop_config.json via json.loads(read_bytes()), letting the stdlib detect UTF-8 / BOM'd UTF-8 / UTF-16 instead of the process locale codec.
Security risks
None identified. The path_security.py change is docstring-only. The config read/write path handles a local user-owned file; json.dumps still ASCII-escapes on write, so output bytes are unchanged from before. No auth, crypto, or network-facing code is touched.
Level of scrutiny
Moderate. The bulk of the diff is mechanical encoding="utf-8" additions with near-zero behavioral risk. The three substantive pieces got closer reading: the CI env-var enforcement (env var rather than -X so xdist workers inherit it, with a correctly module-scoped ignore for pytest_examples' own I/O), the claude.py byte-level JSON read (a strict superset of the old locale read — any file the old code decoded, the new code decodes, plus BOM'd and UTF-16 files it previously rejected or mangled), and the update_readme_snippets.py error-handling rework (narrowed from except Exception to (OSError, UnicodeDecodeError) with fail-closed sys.exit, which cannot let --check pass on a stale block).
Other factors
Four earlier automated review passes plus a cubic pass surfaced six findings (all nits: "$@" quoting, a BOM edge case later superseded by the byte-level read, three doc/docstring staleness items, and the UnicodeDecodeError handler gap); the author addressed every one, and the design churn visible in the timeline (PLW1514 added then dropped, interpreter flags on script steps added then dropped, CONTRIBUTING reverted) reflects deliberate maintainer decisions with stated reasoning, not unresolved review debt. Test coverage is strong: new tests pin non-ASCII preservation and BOM/UTF-16 acceptance in test_claude.py, the PR description documents verification on Windows cp1252/cp936 and glibc GBK/CP1252 locales, and the repo's 100%-coverage gate applies. The author is a listed maintainer of this SDK. With every prior thread resolved and nothing new found this run, this is safe to approve.
Follows #3245 (merged).
Motivation and Context
#3244 / #3245 fix the two
read_text()calls the test suite trips over on a Windows machine whose ANSI code page isn't cp1252. The underlying problem is wider than those two lines: any text-modeopen()/Path.read_text()/write_text()/tempfile/subprocess(text=True)withoutencoding=uses the process locale, and CI structurally can't notice — ubuntu is UTF-8, andwindows-latestis cp1252, which happens to decode every non-ASCII byte we currently have (em dashes, box drawing, arrows) as mojibake rather than raising. On cp936/cp932/cp949/cp950 the same bytes are illegal sequences.The one place this reaches users is
mcp install:src/mcp/cli/claude.pyreadclaude_desktop_config.jsonwith the locale codec and wrote it back throughjson.dumps, so on a stock cp1252 or a CJK Windows box, pre-existing entries containing non-ASCII (aC:\Users\书清\…path, a server namedmétéo) were either rejected or silently persisted as mojibake while the command reported success. The stories harness (examples/stories/_harness.py) also reads the same manifest #3245 fixes in the tests, so everypython -m stories.<name>.clientREADME command failed on such a machine, as did the two repo scripts.This PR:
encoding="utf-8"at every remaining call site (cli, stories harness, scripts, tests, simple-chatbot, onedocs_srcsnippet); the two diagnosticsubprocess.run(..., text=True)captures becomeencoding="utf-8", errors="replace"; and drops a✓fromupdate_readme_snippets.py's output, which raisedUnicodeEncodeErrorwhenever stdout was a non-UTF-8 pipe (e.g. under pre-commit on Windows).scripts/testsetPYTHONWARNDEFAULTENCODING=1(PEP 597), so an omittedencoding=raisesEncodingWarningat the call site on every OS, which our existingfilterwarnings = ["error"]makes fatal (env var rather than-Xso pytest-xdist workers inherit it; one scoped ignore forpytest_examples' ownPopen). The two maintainer scripts aren't wrapped in interpreter flags: their reads are pinned here, andupdate_readme_snippets.py --checknow exits non-zero on any per-block error instead of keeping the stale block. (ruff'sPLW1514was considered as an edit-time counterpart and left out: it is preview-only, misses derived-path receivers such as(a / "b").read_text(), and enabling preview mode changes stable-rule behaviour on ruff upgrades.)json.loads(config_file.read_bytes()), so the JSON layer detects the encoding (plain UTF-8, UTF-8 with a BOM, or the UTF-16 PowerShell 5's>writes) instead of the SDK picking a codec; the rewrite stays plain UTF-8.test_other_servers_preservedseeds non-ASCII entries and a new test covers the BOM/UTF-16 files.How Has This Been Tested?
mainwith only the env var set fails deterministically attests/examples/conftest.py:41(EncodingWarning: 'encoding' argument not specified) on Linux and onwindows-latest; with this branch./scripts/testis green (5582 passed, 100% coverage, strict-no-cover clean).PYTHONWARNDEFAULTENCODING=1onwindows-latest(3.10 and 3.12), both at the stock code page 1252 and with the runner switched to 936: green, no additional warning sources.zh_CN.GBK/en_US.CP1252glibc locales (PYTHONUTF8=0): drovemcp installagainst a config seeded with Chinese and accented entries — before: cp1252 rewrites them as文件系统/José, GBK fails with'gbk' codec can't decode byte 0x94; after: entries byte-identical, new server added, re-install merges env vars.python -m stories.tools.clientand both scripts'--checkgo fromUnicodeDecodeErrorto OK under GBK.claude.pyunder cp1252, under GBK, and under the warning guard on a UTF-8 box.Breaking Changes
None.
mcp installwrites the same bytes as before (json.dumpsstill ASCII-escapes); it just decodes the existing file correctly first.Types of changes
Checklist
Additional context
Set-WinSystemLocale zh-CNtakes effect for new processes on the hosted image without a reboot). It would cover third-party code and spawned children, which the warning can't reach; happy to add it if we think it earns its keep.AI Disclaimer