fix: use UTF-8 (not OS locale) for YAML/JSON/text file I/O - #315
Merged
Conversation
Adds a regression test exercising the real endpoint loader core stack (open -> yaml.safe_load -> model_validate -> tool.description) with a UTF-8 tool description containing non-ASCII characters. The loader opens definition files with bare open(f), so on Windows with a cp1252 default locale the UTF-8 bytes are mis-decoded into mojibake (or raise UnicodeDecodeError) before PyYAML sees them. The test skips on platforms whose default encoding is already UTF-8 (Linux/macOS) so a green result there can't be mistaken for coverage, and runs on Windows where it fails until the loader passes encoding="utf-8". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The original test's comment claimed the description bytes were all defined in cp1252 and would mojibake cleanly; in fact "”" (U+201D) is UTF-8 e2 80 9d and byte 0x9d is undefined in cp1252, so the cp1252 decode raises UnicodeDecodeError. On Windows that error is swallowed into None by load_endpoint and surfaced as an error tuple by discover_tools — neither path reaches the equality assertion. Fix the misleading comment/docstrings and add a second, cp1252-safe description (café / em-dash / ©) that decodes WITHOUT raising, so the loader returns a silently corrupted description (— -> â€"). The new test_tool_description_mojibake_round_trip exercises that silent-corruption path, where the equality assertion — not an exception — is what catches the bug. Both failure modes vanish once the loader opens files with encoding="utf-8". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eason The fixtures only set tool.name/description, but ToolDefinitionModel requires a source (exactly one of code/file). On the Windows cp1252 run the mojibake fixture decoded fine and reached validation, which then failed with "tool.source: Field required" — surfacing as a swallowed None instead of the intended equality mismatch. That latent gap affected all three tests: once the loader is fixed with encoding="utf-8", the decode no longer raises, so the hard-error and discovery tests would also reach validation and fail on the missing source rather than going green. Extract a _tool_yaml() helper that emits a complete, schema-valid definition (source.code) and use it for every fixture. Verified by simulating the cp1252 decode locally: - hard-error desc -> open() raises UnicodeDecodeError -> swallowed to None - mojibake desc -> load succeeds, description != original -> equality fails Both pass once the loader opens files with encoding="utf-8". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every file open() in the runtime used text mode with no encoding=, so on
Windows the OS locale codepage (cp1252) was used instead of UTF-8 — corrupting
non-ASCII content (mojibake) or raising UnicodeDecodeError, depending on the
bytes. This is the root cause behind the endpoint-loader encoding test.
Apply a per-purpose policy across all 26 call sites:
- Readers of self-describing formats (YAML, JSON): open in binary mode so the
parser detects the encoding (PyYAML: UTF-8/16; json: UTF-8/16/32 per RFC
8259). This is locale-immune AND spec-faithful — a UTF-16 file stays valid.
- Readers of opaque text (.sql): explicit encoding="utf-8" (no marker to detect).
- Writers: explicit encoding="utf-8" (no library detection on output).
Sites: site_config, user_config, evals/loader, endpoints/loader (x2),
dbt/runner (2 reads + 2 writes), sdk config loader/processor, cli/init
(1 read + 4 writes), drift checker (read) + snapshot (write), and the cli
run/test/evals/query JSON @file readers.
Verified: binary reads round-trip café/em-dash/© through both UTF-8 and UTF-16;
affected test suites pass (the unrelated 1Password validation failure is
pre-existing on this machine).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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
On Windows, every
open()in the runtime used text mode with noencoding=, so Python decoded/encoded files with the OS locale codepage (cp1252) instead of UTF-8. Depending on the bytes, non-ASCII content in YAML/JSON files was either silently corrupted into mojibake or raisedUnicodeDecodeError. The endpoint loader was the entry point that surfaced this (a tooldescriptionwithcafé,—,©, or smart quotes failed to load), but the same bug affected ~26 file-I/O sites across config, dbt, drift, the SDK, and the CLI.This PR adds a regression test that reproduces the bug on Windows, then fixes the whole class of sites with a per-purpose encoding policy.
Fix policy:
json: UTF-8/16/32 per RFC 8259). Locale-immune and spec-faithful — a legitimately UTF-16 file stays valid rather than being rejected by a hardcoded UTF-8 assumption..sql) → explicitencoding="utf-8"(no marker to detect).encoding="utf-8"(no library detection on output; we declare the encoding).Sites fixed (14 files, 26 call sites):
site_config,user_config,evals/loader,endpoints/loader(×2),dbt/runner(2 reads + 2 writes), SDK configloader/processor,cli/init(1 read + 4 writes),drift/checker(read) +drift/snapshot(write), and theclirun/test/evals/queryJSON@filereaders + thequerySQL reader.Test:
tests/server/test_endpoint_loader_encoding.pyexercises the real loader stack with two failure modes — a hardUnicodeDecodeError(cp1252-undefined byte, swallowed toNone) and silent mojibake (cp1252-safe bytes, caught by an equality assertion). It skips where the platform default is already UTF-8 (Linux/macOS) so a green run there isn't mistaken for coverage, and runs on Windows where it fails before the fix and passes after.Type of Change
Testing
uv run pytest— ran the affected suites (tests/sdk/core/test_config.py,tests/server/test_endpoint_loader_encoding.py); not the full suite. One unrelated failure (test_onepassword_resolver_validation) is pre-existing on this machine (1Password creds present) and reproduces on cleanmain.uv run ruff check .(on changed files)uv run black --check .(on changed files)uv run mypy .(on the 14 changed source files)Security Considerations
Breaking Changes
None. Behavior is unchanged for valid UTF-8 files (the existing universe); the fix additionally makes UTF-16/UTF-32 inputs work and corrects Windows non-ASCII handling.
Additional Notes
sdk/validator/decorators/loaders.py(read_text(encoding="utf-8")) and the audit JSONL backend (encoding="utf-8"throughout).🤖 Generated with Claude Code