fix(encoding): decode non-UTF-8 source files before review (#987) - #1028
fix(encoding): decode non-UTF-8 source files before review (#987)#1028chethanuk wants to merge 2 commits into
Conversation
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // t.Parallel() \u2014 make test runs -race. | ||
| var detectBest = deterministicBest | ||
|
|
||
| var detector = chardet.NewTextDetector() |
There was a problem hiding this comment.
Concurrency Safety: The package-level detector variable is shared across all calls to deterministicBest. While the current call sites (parser and provider) process files sequentially, this shared state creates a latent data race if future code calls Detect concurrently. The chardet.TextDetector likely maintains internal mutable state (recognizers, detection buffers). Either:
- Create a new detector per call:
res, err := chardet.NewTextDetector().DetectAll(raw), or - Add a mutex around the detector call, or
- Document that
Detectmust not be called concurrently.
The detectBest function variable is already marked as test-only, but detector is production code that should be thread-safe.
There was a problem hiding this comment.
Checked this rather than assumed it, and I don't think it's a race.
| func countReplacement(s string) int { | ||
| return strings.Count(s, string(utf8.RuneError)) | ||
| } |
There was a problem hiding this comment.
False Negative in Decode Validation: When raw bytes contain the sequence 0xEF 0xBF 0xBD (UTF-8 encoding of U+FFFD), they're counted here even though they might represent different characters in the source encoding (e.g., in GBK, 0xEF 0xBF could be a valid two-byte character and 0xBD a separate byte).
This masks genuine decode failures: if a bad decode introduces replacement characters, but raw already had a high count of 0xEF 0xBF 0xBD sequences, the check countReplacement(decoded) > countReplacement(raw) fails to catch the problem. The decode is incorrectly accepted as successful.
Consider validating decodes with a different signal, such as:
- Checking if the decoded length is reasonable relative to the raw byte length for the target encoding
- Using a stricter threshold that accounts for the raw replacement count
- Detecting and excluding
0xEF 0xBF 0xBDsequences that were already present in raw before counting
b0856d4 to
8c80b02
Compare
|
@lizhengfeng101 Rebased onto main and conflicts resolved. Please review and merge this. Thanks :) |
8c80b02 to
88c8b70
Compare
ocr treats every byte it reads as UTF-8. A GBK, GB18030, Big5, Shift-JIS, EUC-JP or EUC-KR source file is not, so json.Marshal of the prompt replaces each undecodable byte with U+FFFD and the model reviews replacement characters. Line resolution fails on top of that: existing_code matched against mojibake never finds the line the comment is about. internal/textenc detects a file's charset once and decodes it in memory. Nothing is written back, so ocr stays a read-only reviewer. Detection sits behind a utf8.Valid fast path, so a UTF-8 repo never reaches the detector at all and this cannot regress the encoding that already works. A diff carries two encodings whenever the commit changes one, so the decode is decided per line from the line's side: the frame already records each payload line's +/-/space prefix. New-side lines are decoded with the file's charset; deleted lines only when they are not already valid UTF-8, since with no base ref their own bytes are the only evidence there is. Conversion is all or nothing, so the model can never see one line decoded beside a raw neighbour. chardet.DetectBest cannot be used: DetectAll fans its recognizers across goroutines and finishes with an unstable sort, so results tied on confidence come back in completion order and the same bytes get a different answer between runs. This ships a deterministicBest that breaks ties on charset name. Files past reviewing as text are excluded with undecodable_encoding and named on stderr with their detected charset; merely imperfect ones are marked and still reviewed. Closes alibaba#987
88c8b70 to
be47dc1
Compare
Main threaded an io.Writer through the preview path while this branch was adding the charset decode, so they collided in three places: - outputPreviewText: kept main's Fprintf(out, ...) with this branch's reason-plus-charset string. - runPreviewContext / runScanPreview: took main's `out` parameter and kept the quiet handle. They are not the same channel — the document goes to `out`, the decode notice to stdout.Writer() — so the handle is still what keeps the notice out of the JSON. Comments updated to say that. - architecture.md (4 locales): main's diagram gained the semantic-grouping node; re-applied this branch's decode line and sixth filter gate on top. Test call sites also drifted: outputPreviewText, runScanPreview and loadCommonContext all gained parameters on main. Claude-Session: https://claude.ai/code/session_01FoMHZQ3qqdt98LHTwSJuTG
|
@lizhengfeng101 Rebased onto main and conflicts resolved again. Please review this. Thanks :) |
1 participant
Description
ocrtreats every byte it reads as UTF-8. A GBK, GB18030, Big5, Shift-JIS, EUC-JP or EUC-KR source file is not, sojson.Marshalof the prompt replaces each undecodable byte with U+FFFD and the model reviews replacement characters. Line resolution fails on top of that:existing_codematched against mojibake never finds the line the comment is about. Measured on a four-file fixture repo (ASCII, GBK, Big5, Shift-JIS), the marshalled review payload carried 60 U+FFFD before this change and 0 after.internal/textencdetects a file's charset once and decodes it in memory. Nothing is written back, soocrstays a read-only reviewer. Detection sits behind autf8.Validfast path, so a UTF-8 repo never reaches the detector at all and this cannot regress the encoding that already works;TestDecodeUTF8RepoNeverDetectsasserts the call count is zero. The confidence gate is 90, measured rather than picked: across the fixture set every correct top-1 answer scored 100 and the highest wrong answer scored 68.The charset-to-decoder mapping is a literal map, deny by default, rather than an
ianaindexlookup.ianaindex.IANA.Encoding("GB-18030")returnsinvalid encoding name, andGB-18030is the exact labelchardetemits for Simplified Chinese, so a name lookup resolves Big5, Shift_JIS, EUC-JP and EUC-KR fine and breaks on the one charset this issue is mostly about. There is a test row per label that fails if anyone swaps the map out later.Both sides of the diff
A diff carries two encodings whenever the commit changes one, which @bailu-ZZ raised on the issue.
-lines are old bytes,+and context lines are new, and no single charset serves both. The decode is therefore decided per line, from the line's side: the frame already records each payload line's+/-/space prefix — a fact about the diff format rather than a guess about its bytes. New-side lines are decoded with the file's charset. Deleted lines are decoded only when they are not already valid UTF-8, since with no base ref their own bytes are the only evidence there is. Nothing here needs the base ref, which matters because the parser only ever has HEAD.The file stays the detection evidence wherever it carries a signal. git runs with
-U3, so a hunk with one short CJK comment scores below the gate while the whole file scores 100. Only when the new file is valid UTF-8, and therefore says nothing about the old side, do the non-UTF-8 deleted lines become the evidence.Decoding a unified diff cannot be a
stringspass over the whole text:--- a/xis a frame line outside a hunk and content inside one, and a.sqlfile's-- 中文注释is payload, not a header.internal/diff/decode.gosplits each section using the sameinHunkstate the parser already tracks, decodes the payload, and rebuilds. If the decode moved the line count the section stays raw — a diff whose line count moved no longer describes the file. Conversion is all or nothing: one line that fails to convert marks the file and leaves both streams raw, so the model can never see one line decoded beside a raw neighbour.Detection has to be deterministic
chardet.DetectBestis not.DetectAllfans its recognizers across goroutines and finishes withsort.Sort, which is not stable, so results tied on confidence come back in completion order. Measured on 800 bytes where Shift_JIS, GB-18030 and Big5 all score 10: 200 calls in one process returned Shift_JIS 179 times, GB-18030 15 and Big5 6. The visible effect wasocr scan --previewreporting a differentdetected_charsetfor the same file between runs. This ships adeterministicBestthat breaks ties on charset name.Files that cannot be decoded
A file whose bytes are past reviewing as text is excluded with
undecodable_encodingand named on stdout with its detected charset. A file that is marked but still reviewed gets a warning on stderr instead. Marking and excluding are kept separate, because collapsing them regresses ordinary files — a UTF-8 Go file with one stray0x92detects as windows-1252 at confidence 57, and a 242-byte Latin-1 French file as ISO-8859-1 at 52. Both would be dropped from review entirely over a handful of bytes. Such a file is marked and still reviewed. Two things make itUnreviewable: a replacement-character density above 20%, or a NUL byte, which is what separates a legacy text file from a binary one. Measured across the fixture set, legitimate files land between 0.57% and 11.04% and genuine garbage between 37.41% and 100%, with nothing in between.This is the one place behaviour changes for a file that is reviewed today: a file that is pure garbage now drops out of review instead of being reviewed as mojibake, and
undecodable_encodingis a new value in the exclusion reasons that consumers of the JSON output can see. It is documented alongside the other reasons inpages/src/content/docs/{en,ja,ru,zh}/{architecture,faq}.md.Limitations
Convertcannot see it. Detecting the deleted lines separately does not rescue it either: under-U3they rarely carry enough evidence to clear the gate.0xC2-0xDFwith trail0x80-0xBF. New-side lines are not exposed to this, because they are decoded from their side rather than their bytes.file_readdecodes files up to 2 MiB and streams anything larger with its bytes raw, unchanged from today. Detection needs whole-file evidence, and a head window is actively harmful: a file whose first non-ASCII line sits past the window collapses from confidence 100 to 55 and turns into a false skip.chardet's single-byte scores are weak, and those files are marked and still reviewed, which is today's behaviour.Two direct dependencies:
github.com/saintfish/chardet(pure Go, no transitive deps) andgolang.org/x/text, already in the module graph as indirect.Type of Change
How Has This Been Tested?
make testpasses locallymake check,make test(2042 tests,-race) andmake coveragepass. Coverage is 91.4% against the 90% gate.Tests are table-driven and grouped by the failure each pins: the charset map row by row, including the
GB-18030row that fails if it becomes anianaindexlookup; the detector asserted at zero calls for valid UTF-8; diff headers, prefixes and line counts byte-identical after decode, withTestDecodePayloadLinesThatLookLikeHeaderscovering the-- 中文注释case; comments resolving to the correct line through decoded Chinese; the mark-versus-exclude boundary; review and scan agreeing on the same file byte for byte;file_readandcode_searchreturning the same UTF-8 as the review path; andTestParseDiffTextNeverWritesToTheWorkingTreefor the read-only guarantee. Each encoding-change direction has its own row, andTestDecodeSameEncodingBothSidesUnchangedcompares GBK output byte-for-byte against the same repo committed in UTF-8.I ran mutation testing over every line this PR adds to non-test source: 73 mutants, plus a 31-mutant re-run of
decode.goafter it was rewritten. Every survivor was either killed with a new test or shown to be equivalent, which is what found the leading-NUL guard, the confidence-gate boundary, and acode_searchmatch on line 1.End to end against a real model (OpenRouter,
nvidia/nemotron-3-super-120b-a12b:free) over a mixed UTF-8 + GBK + Big5 + Shift-JIS fixture repo: 60 U+FFFD in the marshalled payload before, 0 after, hunk headers and prefixes intact, and the UTF-8 file bypassing the detector.Checklist
go fmt,go vet)Related Issues
closes #987