Skip to content

fix(tools): treat SQLite journals and sidecars as binary - #114159

Closed
strzhao wants to merge 1 commit into
NousResearch:mainfrom
strzhao:forge/sqlite-sidecar-read-guard
Closed

strzhao wants to merge 1 commit into
NousResearch:mainfrom
strzhao:forge/sqlite-sidecar-read-guard

Conversation

@strzhao

@strzhao strzhao commented Sep 17, 2026

Copy link
Copy Markdown

A SQLite database's -wal / -shm / -journal siblings extend the database name instead of adding an extension (state.db → state.db-wal), so _has_extension_in — which compares the last dot-suffix, .db-wal — never matched them against BINARY_EXTENSIONS. The main file was refused while its live sidecars passed every binary check, so read_file handed the model lossy mojibake that a following write_file could put back over a live WAL generation.

has_binary_extension now also matches a .db / .sqlite / .sqlite3 name carrying one of those suffixes, so the shared helper sees the sidecars and the read guard refuses them — the same round trip #114091 closes at the write end.

Reproduction, driven through the real tool entry points rather than read off the diff (Python 3.11.16 / SQLite 3.53.1, clean origin/main 98f758ae7e checkout):

  • base: read_file("<dir>/state.db-wal") returns lossy text; write_file("<dir>/state.db-wal", "PLAIN TEXT OVERWRITE") lands 20 bytes; the next sqlite3.connect("<dir>/state.db") raises file is not a database.
  • this head: the read is refused, the sidecar bytes are unchanged, the database still opens.

Tests: tests/tools/test_sqlite_sidecar_read_guard.py — 3 passed on this head, 2 of those failing on base 98f758ae7e (the third case is the .sql plain-text control, green on both revisions). tests/tools/test_binary_document_write_guard.py stays green; 20 passed together.

Found while reviewing #114091. It is sent standalone so that PR stays a single concern; cherry-picking this commit into it is fine if you would rather keep the read and write halves in one place.

read_file refused a database but not its -wal/-shm/-journal siblings, so the model received lossy text and could write it back over a live WAL generation. The shared helper now strips a journal suffix and re-checks.
@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) P2 Medium — degraded but workaround exists labels Sep 17, 2026
@kshitijk4poor

Copy link
Copy Markdown

Implemented on main via #114220 (merge 6e7dad7608), which centralised sidecar recognition in tools/binary_extensions.py so the read guard, file_tools.py and the five sandbox read sites in tools/file_operations.py all agree with the write guard — the same mechanism this PR proposes for the read side.

Verified before closing: this PR's three tests (test_sqlite_sidecar_read_guard.py) applied onto current main (64ea66b03d) pass 3/3 unchanged.

You opened this before #114220 and my pre-flight sweep missed it, so the PR body did not credit you — that is on me. Recorded here: @strzhao is the first submitter of the read-side fix, and also the one who reproduced the write-side hole on main in #114091. If you want the test_read_file_refuses_sqlite_sidecars loop landed under your authorship as an extra guard, say so and I will cherry-pick it in a follow-up.

@strzhao

strzhao commented Sep 17, 2026

Copy link
Copy Markdown
Author

Thanks for the close-out and for recording the first-submitter credit.

tools/binary_extensions.py on main now carries the sidecar recognition (_SQLITE_SIDECAR_MARKERS + is_sqlite_sidecar) across the read guard, file_tools and the sandbox read sites — the read-side gap this PR targeted is closed.

Taking you up on the offer: please cherry-pick test_read_file_refuses_sqlite_sidecars under strzhao's authorship in a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants