fix: TextFiles/FileStringPersister read/write UTF-8 explicitly, not the locale - #106
Merged
Merged
Conversation
…he locale FileStringReader/FileStringPersister opened text files with encoding=None, which Python resolves to locale.getpreferredencoding() at call time -- so the bytes a store writes/reads depend on the process's environment ($LANG etc). Consequences: a write of any non-ASCII character can raise on an ASCII-resolving locale (a container or cron/systemd job with LANG unset, an Alpine/musl image), a round trip under different locales can silently corrupt, and a store synced between machines with different locales can become unreadable to the machine that didn't write it. Default encoding to "utf-8" explicitly, matching the exact fix suggested in the issue by its author. A store is a serialization boundary; one whose format depends on an ambient environment variable isn't really specified. Also adds a docstring note to FileBytesPersister.__init__ about the delete_func default (OS trash) being a surprising default for a store of derived/generated data (every deletion leaves a full untracked copy outside the store, and the trash is keyed by basename so same-named keys from different stores can collide there) -- docs only, no behavior change. Reviewed by an independent sub-agent before merge (per crowsnest policy): confirmed the fix applies end-to-end without dropping other inherited open() kwargs, confirmed the pre-fix failure mode (UnicodeEncodeError under a true ASCII locale, forced via PYTHONUTF8=0/PYTHONCOERCECLOCALE=0 since most tested locale strings are coerced to UTF-8 by PEP 538/540 regardless), confirmed reading a pre-existing non-UTF-8 file now fails cleanly (UnicodeDecodeError, not silent corruption) rather than silently, and confirmed no sibling text-mode class in filesys.py was left with the same bug. Closes #97 Co-Authored-By: Claude Sonnet 5 <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.
Summary
FileStringReader/FileStringPersisteropened text files withencoding=None, which Python resolves tolocale.getpreferredencoding()at call time -- so the bytes a store writes/reads depend on the process's environment ($LANGetc). Consequences, per #97: a write of any non-ASCII character can raise on an ASCII-resolving locale (a container orcron/systemdjob withLANGunset, an Alpine/musl image); a round trip under different locales can silently corrupt; and a store synced between machines with different locales can become unreadable to the machine that didn't write it.Defaults
encodingto"utf-8"explicitly -- the exact fix suggested by the issue's author (the repo maintainer) in the issue body. A store is a serialization boundary; one whose format depends on an ambient environment variable isn't really specified.Also adds a docstring note to
FileBytesPersister.__init__about thedelete_funcdefault (OS trash) being a surprising default for a store of derived/generated data, per a smaller separate observation in the same issue -- docs only, no behavior change.Closes #97
Review
Independent sub-agent review (per crowsnest policy) confirmed:
open()kwargs (buffering,errors,newline,closefd,opener).PYTHONUTF8=0 PYTHONCOERCECLOCALE=0, since most testedLANG/LC_ALLvalues are coerced to UTF-8 by PEP 538/540 regardless of what they say) --UnicodeEncodeErrorwriting non-ASCII text.UnicodeDecodeErrorrather than silently -- an accepted, informative tradeoff.filesys.pywas left with the same bug (onlyFileStringReader/FileStringPersisteropen in text mode;FileBytesReader/FileBytesPersisterare binary and ignoreencoding).Dependents
~23 on-box fleet packages import
TextFiles/FileStringPersister. This box's locale is already UTF-8, so their test suites can't observe a difference (the change is behaviorally identical here); the whole point of the bug is that it's locale-dependent, so no on-box test run can validate the fix's actual purpose beyond "doesn't break anything on a UTF-8 box," which is confirmed. Full dol suite green.Test plan
pytest --doctest-modules dol/filesys.py-> 7 passedpytest(full suite) -> 592 passed, 3 skipped🤖 Generated with Claude Code
https://claude.ai/code/session_011HSBVhDjRU4apSLcRkavv9