fix: specify utf-8 encoding for all text file I/O - #382
Conversation
Signed-off-by: Marco De Vincenzi <md6796@nyu.edu>
mlieberman85
left a comment
There was a problem hiding this comment.
Confirmed the sweep is complete and correct:
Scope check -- searched for text-mode I/O missing encoding= after applying the diff. Only remaining open() calls without encoding= are urllib.request.urlopen (network, not filesystem) and open(os.devnull, "w") (my own device-write in mcp_pool._resolve_child_stderr). All text file I/O under packages/*/src/ and scripts/ is now UTF-8-explicit. Path.read_text / write_text grep also clean once you follow multi-line calls.
Preserved semantics -- errors="ignore" retained wherever it was present (dependencies.py::_read_file, adapters/builtin.py::check_pattern_*), so scan tools still tolerate mixed-encoding source files rather than crashing.
Local verification:
pytest tests/darnit/remediation/test_helpers.py-- 22 pass (includingtest_writes_unicode)python scripts/validate_sync.py --verbose-- all 3 validations run and passruff check .-- clean- Full workspace
pytest tests/ --ignore=tests/integration-- 2709 pass, 10 skip, 0 fail
Correctness of the tricky cases:
os.fdopen(fd, "w", encoding="utf-8")inaudit_cache.py-- legal,fdopenforwards kwargs toopen.open("/dev/tty", "r+", buffering=1, encoding="utf-8")--buffering=1(line-buffered) is only valid in text mode; addingencodingkeeps it text mode. Fine.- YAML load/dump in
dot_project.py-- ruamel handles utf-8 text streams natively, no double-decode.
On the PR body's "Why this is a bug not a style change" summary: the write_file_safe -> UnicodeEncodeError -> unhandled bubble-up analysis is correct (UnicodeEncodeError subclasses ValueError, not OSError), and the scripts/validate_sync.py "silently skipped two thirds of its checks on Windows" observation is exactly the kind of thing that makes me want this landed. validate_sync is a CI gate; a Windows CI runner would have been reporting green while checking one third of what it advertised.
Nit (non-blocking): no new tests were added for the crash-on-Windows behavior itself. That's acceptable here because the existing test_writes_unicode failed on Windows before the fix and passes after -- it's a natural regression witness -- and platform-conditional tests would add complexity that outweighs the marginal signal.
LGTM.
Summary
Text-mode file I/O across
packages/*/srcandscripts/relied on Python's platform-default encoding. On Linux/macOS that's UTF-8, so this is invisible in CI. On Windows it's the locale codepage (cp1252), and any character outside it raisesUnicodeEncodeErroron write orUnicodeDecodeErroron read.51 call sites across 18 files now pass
encoding="utf-8"explicitly. Binary-mode opens andurlopenare untouched.Why this is a bug, not a style change:
write_file_safecrashed callers instead of returning an error. It catchesOSError, butUnicodeEncodeErrorsubclassesValueError— so on Windows, remediation writing a template containing a curly quote, em dash, or emoji raised through the safety net rather than returning(False, msg).scripts/validate_sync.pysilently skipped two thirds of its checks. On Windows it died at the firstread_text(), so the handler-name registry and SARIF-source validations never ran at all. All three now completeaudit_cache.pyandverification.pywrote JSON with the platform codepage and read it back the same wayType of Change
Framework Changes Checklist
If this PR modifies the darnit framework (
packages/darnit/):docs/architecture/framework-design.md) if behavior changed — n/a, no behavior changeuv run python scripts/validate_sync.py --verboseand it passesControl/TOML Changes Checklist
If this PR modifies controls or TOML configuration:
Testing
uv run ruff check .)Windows 11, Python 3.12.13. Baseline measured on
4da483abefore the change;tests/integrationexcluded and the CNCF.project/hash canary deselected in both runs.TestWriteFileSafe::test_writes_unicodenow passesuv run ruff check .— cleanuv run python scripts/validate_sync.py --verbose— PASS (previously crashed at the first check):Additional Notes
Remaining Windows failures are pre-existing and out of scope: path-separator assertions,
TestExecHandlershelling out tosleep, and symlink privileges. Happy to take those in follow-ups.