fix(windows): safely repair legacy mojibake with backups - #2148
Conversation
|
Hey again @fatkobra! Ran into a few hard ones in your latest batch. Could you run them through e2e in mempalace itself? They all show up on the first real run. This one: the dry-run flags text that was already fine and French typography hits it too, since the capital É lands right before the non-breaking space in #2141: against a live chromadb the collection still comes back carrying #2143: the real Worth putting the earlier PRs through e2e as well, the problem and the fix, not just the fix. |
|
Thanks @mvalentsev — this is valid and blocking feedback. The current detector is fundamentally too broad: it accepts any short CP1252 character window whose bytes happen to decode as one non-ASCII UTF-8 character. That means already-correct text can be changed even when it contains no mojibake marker. The examples with I am converting this PR to draft. Before marking it ready again, I will require:
I will also revisit #2141 and #2143 with real end-to-end reproductions rather than mocked versions of the suspected failure. Thank you for running these against the real application. |
|
Thanks again — the initial E2E run found one additional wrapper issue before I pushed the revision: the backup records were correct, but the collection field was That is now fixed and covered as well. The revised PR now:
I reran the complete command against a newly created disposable palace across separate processes:
The PR remains draft pending review of the revised safety contract. |
|
One thing is still open, from the same place. cp1252 leaves five bytes undefined ( On a palace damaged the way a pre-3.1.0 Windows install damaged it, invisible bytes escaped: Three of those four rows get rewritten, counted as updated, and are still damaged. A second run does not finish them. #1055 is a Spanish palace, and the reporter writes there that The six strings from the last round do survive a real |
|
Thanks again @mvalentsev for excellent feedback — this was valid and blocking. The previous revision deliberately omitted the five byte values that Windows-1252 leaves undefined:
That preserved the earlier clean-text safety, but it also meant that old palaces containing those values as invisible C1 controls could only be partially repaired. The row was counted as updated when one visible portion changed even though another damaged sequence remained. The revised decoder now:
New coverage includes:
I also exercised the actual command against a disposable persistent palace with four clean and four damaged rows:
|
|
The current Windows failure is unrelated to this PR's encoding changes. The only failing test is:
It failed inside Chroma with:
The run otherwise completed with 3,357 passed tests, and none of the PR #2148 changes only the encoding-repair module, its command-line script, and its tests. It does not modify the closet/search/HNSW path. The same test and exact HNSW error have also appeared in an earlier Windows CI run. The workflow currently reruns only failures matching:
so this @igorls could you please rerun the failed Windows job? |
I've been testing from mac recently, will switch to windows to test this better |
ee5252c to
047d733
Compare
What does this PR do?
Ä...andÅ...sequences unchangedDetection contract
The original implementation treated any short Windows-1252 character window whose bytes formed valid UTF-8 as damaged.
That was unsafe because legitimate sequences such as:
can also form valid UTF-8 byte sequences.
The revised automatic mode recognizes only the common visible lead families produced by UTF-8-as-Windows-1252 corruption:
The
Ä...andÅ...families are intentionally excluded from automatic repair because they are ambiguous with legitimate Central European and scientific text.This deliberately favors false negatives over destructive false positives.
The old Windows corruption path can also preserve five byte values that Windows-1252 leaves undefined:
Those values appear in stored Python strings as invisible C1 control characters.
The revised decoder accepts them only as continuation positions inside an already-recognized high-confidence mojibake sequence. It maps each control code point back to its original raw byte before UTF-8 decoding.
This repairs characters such as
Á,Í,Ï,Ð,Ý, and the closing curly quote”without enabling the ambiguous visibleÄ...orÅ...lead families.Standalone control characters are not rewritten.
Dry-run behavior
Dry-run is the default.
It scans the collection without writing and prints every proposed drawer ID with a bounded before/after preview.
Users can therefore inspect the actual proposed changes before choosing
--apply.Apply and backup safety
Apply requires a backup path internally. When no path is supplied on the command line, a timestamped path beside the palace is generated.
The backup:
The collection name is resolved through MemPalace backend wrappers, including
ChromaCollection.Restore safety
The same command restores original documents with:
Before the first restore write, the implementation:
Locking
Apply and restore hold the normal palace writer lock for the complete operation.
The underlying backend write lock remains re-entrant, so calls through
ChromaCollection.update()do not deadlock.Tests
Regression coverage includes:
PERÚ,Ų,CAFÉ®, andRÉSUMÉ :review cases;The actual command was also exercised manually across separate processes against a disposable persistent palace:
Additional regression coverage verifies:
ÁandÍ;Ï,Ð, andÝ;repair_collection();Closes #1055
How to test
python3 -m pytest tests/test_encoding_repair.py -qpython3 -m ruff format --check .python3 -m ruff check .python -m pytest tests/ -vChecklist
python -m pytest tests/ -v)ruff check .)