Fix/output encoding fallback replace - #304
Open
Jacob Bundgaard (kimsey0) wants to merge 2 commits into
Open
Conversation
… supported characters
`OutputProducer.out` falls back to `output.encode('ascii', 'ignore')` when the
destination stream cannot encode the output. Because the retry targets ASCII
rather than the stream's own encoding, a single unrepresentable character
anywhere in the document discards every non-ASCII character in it, including
characters the destination encoding represents perfectly well.
The first test writes a payload of `ae oe aa em-dash infinity` to a cp1252
stream. cp1252 supports all of those except U+221E INFINITY, but the current
fallback drops the four supported characters along with the unrepresentable one.
The second test guards the property that the fallback must not turn valid JSON
into something a parser rejects, including for characters outside the Basic
Multilingual Plane such as emoji.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WfoMDHj25s5BDVD4HacTd5
… encoding
When the destination stream could not encode the output, the fallback re-encoded
the whole document as ASCII with `errors='ignore'`. Because the retry targeted
ASCII rather than the stream's actual encoding, one unrepresentable character
anywhere in the output silently discarded every non-ASCII character in the whole
document, including characters the destination encoding represents perfectly
well. On a Windows console in a non-UTF-8 locale, a single emoji or CJK
character in a result set stripped every accented Latin character from the rest
of it.
The fallback now encodes with the stream's own encoding, so only genuinely
unrepresentable characters are affected. It uses `replace`, which substitutes
'?' and therefore keeps the document parseable in every output format. The
alternative, `backslashreplace`, would preserve more information, but it emits
`\U0001f600` for characters outside the Basic Multilingual Plane, and since JSON
permits only `\uXXXX` that would make emoji-containing output fail to parse.
Trading silent corruption for a parse failure seemed the wrong trade for a path
shared by every command of every knack-based CLI.
The warning text is corrected to match: it previously named `out_file.encoding`,
which the fallback did not actually use, and claimed that only unsupported
characters were discarded when supported ones were discarded too.
Also drops the `.decode('utf-8', 'ignore')` round trip, which was a no-op on
bytes that had just been produced by `.encode('ascii', 'ignore')`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WfoMDHj25s5BDVD4HacTd5
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
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.
Fixes #303.
When the destination stream cannot encode the output,
OutputProducer.outretried by encoding the whole document as ASCII witherrors='ignore'. Because the retry targeted ASCII rather than the stream's own encoding, a single unrepresentable character anywhere in the output discarded every non-ASCII character in the document. On a Windows console in a non-UTF-8 locale, one emoji or CJK character stripped every accented Latin character from the rest of the result set, quietly replacing correct data with plausible-looking wrong data.The retry now encodes with
out_file.encoding, so only characters the destination genuinely cannot represent are affected. Those become?viaerrors='replace', which keeps the document parseable in every output format.backslashreplacewould preserve more information, but it emits\U0001f600for characters outside the Basic Multilingual Plane and JSON permits only\uXXXX, so emoji-containing output would stop parsing altogether; the issue sets out that trade-off, and switching the handler is a one-word change if you prefer it.This also corrects the warning text, which named an encoding the fallback did not actually use and said "Unsupported characters are discarded" when supported ones were discarded too, and drops the
.decode('utf-8', 'ignore')round trip, which was a no-op on bytes just produced by.encode('ascii', 'ignore').Nothing changes for streams that can encode the output: the fallback only runs when a
UnicodeEncodeErroris raised, so UTF-8 consoles are unaffected.Tests
Two regression tests in
tests/test_output.py, both failing before this change:test_out_json_non_ASCII_unencodablewritesæ ø å — ∞to a cp1252 stream and asserts the four characters cp1252 supports survive, rather than being discarded along with U+221E.test_out_json_non_ASCII_unencodable_stays_parseablewrites an emoji payload and asserts the output still parses as JSON, guarding the property that motivatedreplaceoverbackslashreplace.Disclosure: the investigation behind this change and this description were produced with Claude Code. The full suite (247 tests), flake8 and pylint were run against
devat52f769a.