Repository navigation
write_file/patch can no longer corrupt a SQLite WAL/SHM sidecar (salvage #114091) - #114220
Merged
kshitijk4poor merged 8 commits intoSep 17, 2026
Conversation
The text tools' read->modify->write round-trip re-encodes binary bytes
lossily: the terminal transport decodes stdout with errors=replace, so
a patch on a SQLite database reads mojibake and writes it back, silently
destroying the file (observed in the wild: a kanban board DB lost 550k
bytes / 26 days of history this way on 2026-09-15).
read_file already refuses to display binary files (extension + byte
sniffing), but write_file/patch had no write-side equivalent, and
patch_replace reads via plain cat so the read-side guard never fires
for it.
Guard rules (extends _check_binary_document_write, the existing
binary-document write guard):
- .db/.sqlite/.sqlite3 and their -wal/-shm/-journal sidecars: ALWAYS
refused (even new-file creation) — plain text can never be a valid
database, mirroring the opaque-document rule. The sidecars need a
basename regex because os.path.splitext('x.db-wal') yields '.db-wal',
which is not in BINARY_EXTENSIONS.
- Other BINARY_EXTENSIONS (.png, .zip, ...): refused when OVERWRITING an
existing file, mirroring the existing .pdf-overwrite rule — the model
can only have read mojibake, so writing back destroys it. Creating a
NEW file with such an extension stays allowed.
Tests: new test_binary_db_write_guard.py covers the regex (main names,
sidecars, non-db names), the guard function, and the write_file/patch
tool paths against real SQLite files (integrity_check still ok, bytes
untouched). 29 new+existing guard tests pass; file-tools/patch suites
green (196 tests).
… write-guard regex Move the .db-wal/-shm/-journal detection from a write-guard-local regex into tools/binary_extensions._has_extension_in so has_binary_extension (read guard AND write guard) agree: a sidecar counts as its database's extension. read_file now refuses sidecars instead of returning lossy text, which also gives write_file the no-baseline overwrite refusal. Behaviour change vs the contributor's commit: creating a NEW .db / .sqlite file via write_file stays allowed (main allows it and text fixtures named *.db exist) — only overwriting an existing binary is refused. The generic has_binary_extension overwrite branch is kept and merged with the PDF branch because patch has no full-read baseline check: on main, patch on an existing .db with a matching old_string rewrites the header in place. Tests folded into the existing guard test classes (one write_file, one patch refusal on a real WAL sidecar); the PR's 12-test file and its private-regex assertions are dropped.
… sidecar Any error would also be produced by the no-baseline overwrite guard on main, so the test could not detect a regression of the sidecar detection. Asserting the binary-file refusal makes it fail when binary_extensions stops recognising .db-wal.
…e exists yet
The refactor that moved sidecar detection into binary_extensions dropped the
contributor's unconditional refusal for the db family and made every binary
extension a plain overwrite guard. That is right for the MAIN .db/.sqlite
(text fixtures named *.db exist), but a -wal/-shm/-journal path is never a
legitimate text target: a checkpointed database has no sidecar on disk, so
write_file("kanban.db-wal", text) silently dropped a garbage WAL next to a
live database.
Add is_sqlite_sidecar(path) and refuse such paths in
_check_binary_document_write regardless of is_file(). Scope the marker
stripping in _has_extension_in to SQLite suffixes so report.docx-wal no
longer counts as an opaque document. Hoist the duplicated is_pdf_path call.
Parametrize the write_file WAL test over sidecar existing/absent.
file_operations.py checked `os.path.splitext(path)[1].lower() in BINARY_EXTENSIONS` at five sites, so a `.db-wal` / `.sqlite3-shm` sidecar (final suffix never in the set) was read and edited as text in the sandbox paths. Route all five through has_binary_extension(), which strips the sidecar marker. Behaviour is otherwise identical: same lower-cased final suffix membership (rfind vs splitext differ only on leading-dot basenames like `.bashrc`, which are in neither set). binary_extensions imports nothing from tools, so no cycle.
_make_wal_db claimed sqlite keeps the -wal until checkpoint, but SQLite deletes -wal/-shm on last-connection close, so the fake-bytes fallback was what actually ran. Make the fixture a context manager that holds the connection open across the tool call so the sidecar is a real WAL, and drop the PRAGMA integrity_check assertion (SQLite ignores a garbage WAL, so it passed either way); keep the byte-identity check. In the patch test assert "binary" in the error so the no-baseline guard cannot mask a regression in sidecar detection.
…rop the dead .db3 member
_has_extension_in and is_sqlite_sidecar each re-implemented the same
rfind('.') / -1 / .lower() suffix extraction, and the write guard used a
third spelling (filepath[filepath.rfind("."):]) for its refusal messages
next to os.path.splitext in the overwrite branch. Add _lower_suffix()
("" when no dot) and route both predicates through it; the write guard
now uses os.path.splitext for every message's displayed extension.
_SQLITE_EXTENSIONS was `{.db,.sqlite,.sqlite3,.db3} & BINARY_EXTENSIONS`,
which silently dropped .db3 (not a binary extension anywhere in the
codebase, so `x.db3-wal` was never a sidecar). Spell the set as the three
members that actually take effect and assert the subset invariant instead
of hiding it behind an intersection. Behaviour is unchanged.
… binary The test trim dropped the headline case: text written over an EXISTING .db. Sidecar paths return at is_sqlite_sidecar before the overwrite branch, so a mutant that drops has_binary_extension from the overwrite condition survived every remaining test. Add a third parametrize case that targets the held-open WAL database file itself and asserts the binary refusal with bytes unchanged. Same commit: the _check_binary_document_write docstring now names SQLite sidecars (always) and every BINARY_EXTENSIONS suffix (on overwrite), and the suffix is computed once at the top instead of three times.
alt-glitch
added
type/bug
This was referenced Sep 17, 2026
Author
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
3 participants
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.
write_file/patchcan no longer write text into a SQLite sidecar (state.db-wal,-shm,-journal), andread_fileno longer returns lossy text for one; overwriting an existing database file is refused as binary.Salvage of #114091 by @garadice (Martin Mogis; cherry-picked, authorship preserved). Opened 2026-09-17 during the P1 sweep; @strzhao reproduced the hole on main in the PR thread.
Root cause:
tools/binary_extensions.py::_has_extension_incompared only the final.suffix, sox.db-walwas an unknown, text-like extension;write_fileinto-walsucceeded and the database then reporteddisk I/O error..dbitself was already refused by the overwrite guard.Changes (net vs the original PR — slimmer, wider)
fix(tools)(@garadice): the contributor's guard, then our follow-ups:binary_extensions.py(one_lower_suffixhelper,is_sqlite_sidecar), so the read guard,file_tools.pyand the five sandbox read sites intools/file_operations.pyagree with the write guard; the write-guard-local regex is gone..db" policy is dropped — text fixtures named*.dbexist; overwrite-only semantics kept for the main file.test_binary_document_write_guard.py: one parametrizedwrite_filetest (existing sidecar / absent sidecar / the.dbitself) + onepatchtest, real WAL-mode database held open.Validation
98f758ae7ewrite_file(state.db-wal)wrote 10 bytes →disk I/O error(before); refused, bytes unchanged,integrity_checkok (after);write_file(new notes.db)still allowed_strip_sidecar_markerdisabled → 3 sidecar cases fail;pdf or has_binary_extension→pdf→ the.dbcase fails; restored → 21/21test_binary_document_write_guard.py21,test_file_write_safety.py,test_file_read_guards.py,test_read_binary_type_disclosure.py: green; 3 pre-existing macOS-only reds intest_file_tools.py/test_read_file_utf8_binary_regression.pyidentical on mainFollow-up (not here): magic-byte (
SQLite format 3\0) sniffing for extensionless databases.Closes #114091 (salvaged). Credit: @garadice.