fix: report-write failures should not look like findings in check and lint - #10
Merged
Conversation
`check` wrote --html and --markdown, and `lint --fix` rewrote the user's own source file, with no error handling and no encoding. Both defects escaped as exit 1 -- the code both commands use for real findings -- so an unwritable path was indistinguishable from a failed gate, and CI would block on a healthy run. Two failure modes, folded into one helper: - No encoding. The default is platform-dependent and these outputs carry non-ASCII: the gate verdict is an emoji, measured as ['x'] in the markdown report, so on an ASCII locale every `check --markdown` was a guaranteed crash. `lint --fix` was worse in kind rather than degree -- read_sql_file already reads as UTF-8, so a non-ASCII comment could round-trip into mojibake in a file the user asked us to rewrite in place. - No handler. UnicodeEncodeError is a ValueError, so pinning the encoding makes it unlikely rather than unreachable -- a lone surrogate in an identifier is unencodable in any codec -- hence catching UnicodeError alongside OSError. The renderers now run before the write, so a renderer bug cannot be reported as a write failure. This is the same bug fixed for `advise` in #9 and reported there as follow-up. It is worse in `check` and `lint` precisely because those two use exit 1 legitimately, while `advise` never does. The two encoding tests assert the argument rather than emulating a locale: on this Python neither locale.getpreferredencoding nor locale.getencoding is consulted by write_text, so a locale-emulating test passed with the bug present. Verified by mutation that all four new tests fail without the fix. Co-Authored-By: Claude Opus 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.
Follow-up to #9, which reported this rather than fixing it.
checkwrote--htmland--markdown, andlint --fixrewrote the user's own source file, with no error handling and no encoding. Both defects escaped as exit 1 — the code both commands use for real findings — so an unwritable path was indistinguishable from a failed gate and CI would block on a healthy run.That makes this worse in
checkandlintthan the equivalent bug was inadvise:advisenever exits 1, so there the failure was merely wrong. Here it is actively misleading.Two failure modes, one helper
No encoding.
Path.write_textdefaults to a platform-dependent encoding, and these outputs carry non-ASCII — the gate verdict is an emoji:So on an ASCII locale, every
check --markdownwas a guaranteed crash.lint --fixis worse in kind rather than degree:read_sql_filealready reads as UTF-8, so writing without one could round-trip a non-ASCII comment into mojibake — in a file the user explicitly asked us to rewrite in place.No handler.
UnicodeEncodeErroris aValueError, so pinning the encoding makes it unlikely, not unreachable — a lone surrogate in an identifier is unencodable in any codec. Henceexcept (OSError, UnicodeError).The renderers now run before the write, so a renderer bug cannot be reported as a write failure.
A note on the tests
The two encoding tests assert the
encoding=argument rather than emulating an ASCII locale. That is deliberate: on this Python neitherlocale.getpreferredencodingnorlocale.getencodingis consulted bywrite_text, so my first attempt — a locale-emulating test — passed with the bug still present. Asserting the argument is a weaker kind of test, but an honest one.All four new tests were mutation-checked: reverting
encoding="utf-8"and narrowing the catch back toOSErrorfails them.🤖 Generated with Claude Code