Skip to content

fix: specify utf-8 encoding for all text file I/O - #382

Merged
Marc-cn merged 1 commit into
darnitdevorg:mainfrom
Marc-cn:fix/utf8-encoding
Aug 21, 2026
Merged

fix: specify utf-8 encoding for all text file I/O#382
Marc-cn merged 1 commit into
darnitdevorg:mainfrom
Marc-cn:fix/utf8-encoding

Conversation

@Marc-cn

@Marc-cn Marc-cn commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Text-mode file I/O across packages/*/src and scripts/ 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 raises UnicodeEncodeError on write or UnicodeDecodeError on read.

51 call sites across 18 files now pass encoding="utf-8" explicitly. Binary-mode opens and urlopen are untouched.

Why this is a bug, not a style change:

  • write_file_safe crashed callers instead of returning an error. It catches OSError, but UnicodeEncodeError subclasses ValueError — 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.py silently skipped two thirds of its checks. On Windows it died at the first read_text(), so the handler-name registry and SARIF-source validations never ran at all. All three now complete
  • Cache files were non-portable. audit_cache.py and verification.py wrote JSON with the platform codepage and read it back the same way

Type of Change

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Breaking change (fix or feature causing existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)

Framework Changes Checklist

If this PR modifies the darnit framework (packages/darnit/):

  • Updated framework spec (docs/architecture/framework-design.md) if behavior changed — n/a, no behavior change
  • Ran uv run python scripts/validate_sync.py --verbose and it passes

Control/TOML Changes Checklist

If this PR modifies controls or TOML configuration:

  • n/a — no controls or TOML touched

Testing

  • Tests pass locally
  • Added tests for new functionality (if applicable) — n/a, existing test now passes
  • Linting passes (uv run ruff check .)

Windows 11, Python 3.12.13. Baseline measured on 4da483a before the change; tests/integration excluded and the CNCF .project/ hash canary deselected in both runs.

  • Before: 30 failed, 2679 passed
  • After: 29 failed, 2681 passed — TestWriteFileSafe::test_writes_unicode now passes
  • uv run ruff check . — clean
  • uv run python scripts/validate_sync.py --verbose — PASS (previously crashed at the first check):
✓ TOML Schema: TOML schema valid (66 controls)
✓ Pass Types Sync: Handler names in sync (7 handlers)
✓ SARIF Source: SARIF formatter reads from TOML (no catalog references)
PASSED: All validations successful

Additional Notes

Remaining Windows failures are pre-existing and out of scope: path-separator assertions, TestExecHandler shelling out to sleep, and symlink privileges. Happy to take those in follow-ups.

Signed-off-by: Marco De Vincenzi <md6796@nyu.edu>
@Marc-cn
Marc-cn requested a review from mlieberman85 as a code owner August 18, 2026 23:25

@mlieberman85 mlieberman85 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (including test_writes_unicode)
  • python scripts/validate_sync.py --verbose -- all 3 validations run and pass
  • ruff 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") in audit_cache.py -- legal, fdopen forwards kwargs to open.
  • open("/dev/tty", "r+", buffering=1, encoding="utf-8") -- buffering=1 (line-buffered) is only valid in text mode; adding encoding keeps 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.

@Marc-cn
Marc-cn merged commit 349f8f2 into darnitdevorg:main Aug 21, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants