All reported issues were addressed across 21 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

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.

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.

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.

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.

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 OSError handler in update_readme_snippets.py: a candidate issue that it no longer catches UnicodeDecodeError (a ValueError subclass) or an escalated EncodingWarning was examined and ruled out — both propagate uncaught, so --check still exits nonzero (fail-closed, just via traceback rather than the tidy sys.exit message).
• The utf-8-sig read + plain-utf-8 write round-trip in claude.py and the upgraded BOM/non-ASCII test.
• The lenient stderr decode in 9572a85, consistent with the other two capture sites.

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.

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.

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.

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 runs strict-no-cover unconditionally while CI gates that same check behind if: 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 when uname reports 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 pytest to ./scripts/test, with the parenthetical "scripts/test runs them the way CI does, with coverage." That parity claim does not hold on Windows: scripts/test line 12 runs UV_FROZEN=1 uv run --frozen strict-no-cover unconditionally under set -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-cover fails whenever a line marked # pragma: no cover is actually executed — AGENTS.md documents this exactly: "CI's strict-no-cover (skipped on Windows runners) fails if it IS executed." The codebase marks Windows-only branches with plain # pragma: no cover (not lax 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,339 and src/mcp/server/stdio.py:74,100,152 are all if sys.platform == \"win32\": # pragma: no cover. Neither file is in the coverage omit list (only src/mcp/os/{posix,win32}/utilities.py are). 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_command and create_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/sh script there) and runs ./scripts/test per 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 cover lines above. (4) The script's final line runs strict-no-cover, which flags those pragmas as "excluded but executed" and exits non-zero; under set -e the 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/test green) or on windows-latest via 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/test the 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.

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=-less read_text/write_text/open/tempfile/subprocess text call remains in src/, scripts, or the touched tests.
• claude.py's json.loads(read_bytes()) read: stdlib detect_encoding covers 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-scoped EncodingWarning ignore and the CI/scripts/test env-var placement: correctly scoped, and "$@" quoting is fixed.
• update_readme_snippets.py's except (OSError, UnicodeDecodeError) → sys.exit handler: fails closed (non-zero) for --check on missing and mis-encoded sources alike.

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.