From 929878ff2e89f08e8e5939a035884ff11e1c1554 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:33:08 -0700 Subject: [PATCH 1/5] fix(cli): route doctor's journal-mode probe through the pre-open reader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _read_journal_mode opened each Hermes database with a bare open(db_path, "rb") to read header byte 18. The read itself is harmless; the close() is not. Per sqlite.org/howtocorrupt.html, close() on *any* descriptor for a file cancels every POSIX advisory lock this process holds on it — so the close at the end of that with-block drops the locks a live connection is holding, including the EXCLUSIVE lock a VACUUM holds while it rewrites the whole file. Another process is then free to write into a file its writer still believes it owns, which is the documented route to "database disk image is malformed". This is reachable. run_doctor is not only a standalone CLI process: the dashboard console registers "doctor" (console_engine.py:570) and calls run_doctor directly, in-process (console_engine.py:1297), on the web server's console thread pool — in a process that holds live SessionDB connections (web_server.py:11673, :11689). Typing "doctor" there raw-opened and closed state.db, projects.db, response_store.db, cron/executions.db and every board's kanban.db while those connections were live. The HTTP route at /api/ops/doctor deliberately spawns a subprocess instead; the console path did not. hermes_cli.sqlite_safe_read exists to prevent exactly this, and its read_header_bytes_preopen is documented as "the ONLY sanctioned byte-level read of a database file". It performs the registry check and the open/read/close together under the connection-lifecycle lock, so it refuses once any connection to the path is live. The audit that converted the other byte-probes (hermes_state.py:2750, backup.py:436, kanban_db.py:1861) landed in 95fb4778561 on 2026-07-25; _read_journal_mode was added in 65832970868 on 2026-08-06 and reintroduced the pattern, so this is a regression against an invariant the tree already states, not a refactor preference. The helper is a plain byte read, so the docstring's stated property is preserved: no SQLite engine open, and no -wal/-shm sidecars are created. Only the acquisition of `header` changes; the empty / not-a-database / unrecognized-format-version branches are untouched. --- hermes_cli/doctor.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index e16de6060c9e..6153aa56e31b 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -124,12 +124,25 @@ def _read_journal_mode(db_path: Path) -> tuple[str | None, str | None]: Header byte 18 is 2 for WAL and 1 for a rollback journal. Opening the database through the SQLite engine — even read-only — creates -wal/-shm sidecar files, which a diagnostic must not do. + + The byte read is routed through ``read_header_bytes_preopen`` rather than + a bare ``open()``: closing *any* descriptor for a database file cancels + this process's POSIX advisory locks on it, so a raw read would drop the + locks a live connection is holding (see ``hermes_cli.sqlite_safe_read``). + ``run_doctor`` is also called in-process by the dashboard console, which + holds live ``SessionDB`` connections. The helper refuses in that case and + the mode is reported as unreadable instead. """ - try: - with open(db_path, "rb") as fh: - header = fh.read(20) - except OSError as exc: - return None, str(exc) + from hermes_cli.sqlite_safe_read import ( + has_live_connection, + read_header_bytes_preopen, + ) + + header = read_header_bytes_preopen(db_path, length=20) + if header is None: + if has_live_connection(db_path): + return None, "database is open in this process" + return None, "file could not be read" if len(header) == 0: return None, "file is empty" if len(header) < 20 or not header.startswith(_SQLITE_HEADER_MAGIC): From e970abd16833514b4a7b26e172575aa07a11eb32 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:33:47 -0700 Subject: [PATCH 2/5] fix(cli): keep doctor's journal-mode read errors specific MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_header_bytes_preopen returns None for every failure, so routing the probe through it flattened "[Errno 2] No such file or directory: …" and "[Errno 13] Permission denied: …" into one opaque "file could not be read". doctor exists to name the problem, so that detail is worth keeping: _report_database_journal_modes prints the string verbatim, and on a vulnerable SQLite it is the only clue the user gets about why WAL exposure could not be ruled out. _unreadable_reason recovers it from metadata only. stat() reports the missing file, the dangling symlink and the unsearchable parent directory; os.access(..., R_OK) reports the unreadable file that stat() can still see. Neither call takes a file descriptor, so neither can cancel the POSIX advisory locks the previous commit was about — the invariant holds. --- hermes_cli/doctor.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 6153aa56e31b..dd00f42c871f 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -118,6 +118,23 @@ def _hermes_database_paths(hermes_home: Path) -> list[tuple[str, Path]]: _SQLITE_HEADER_MAGIC = b"SQLite format 3\x00" +def _unreadable_reason(db_path: Path) -> str: + """Explain why a database file could not be read, without opening it. + + ``read_header_bytes_preopen`` collapses every ``OSError`` into ``None``, + but doctor's job is to say *which* problem it hit. ``stat()`` and + ``access()`` answer that from directory metadata alone — neither takes a + file descriptor, so neither can cancel the file's POSIX advisory locks. + """ + try: + db_path.stat() + except OSError as exc: + return str(exc) + if not os.access(db_path, os.R_OK): + return f"permission denied: {db_path}" + return "file could not be read" + + def _read_journal_mode(db_path: Path) -> tuple[str | None, str | None]: """Return (journal mode, error) from the file header without opening the database. @@ -142,7 +159,7 @@ def _read_journal_mode(db_path: Path) -> tuple[str | None, str | None]: if header is None: if has_live_connection(db_path): return None, "database is open in this process" - return None, "file could not be read" + return None, _unreadable_reason(db_path) if len(header) == 0: return None, "file is empty" if len(header) < 20 or not header.startswith(_SQLITE_HEADER_MAGIC): From e86ce0536f9c8989a9d29f9c4463175a68cce93c Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:36:38 -0700 Subject: [PATCH 3/5] test(cli): cover doctor's journal-mode probe against live connections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Locks the invariant the probe now honours: while this process holds a registered connection to a database, _read_journal_mode reports it as unreadable instead of taking a descriptor whose close() would cancel that connection's POSIX advisory locks. Against the previous implementation the four regression cases fail with `assert 'wal' is None` — it read the header straight out of a live database — and pass once the read is routed through read_header_bytes_preopen. Coverage is both the registry API (track_connection) and connect_tracked, the path SessionDB actually takes, plus the _report_database_journal_modes output so the degraded row is asserted end to end. Two cases deliberately hold in both directions and are guards rather than probes: - an untracked sqlite3.connect holding BEGIN EXCLUSIVE must NOT block the read. Only connections this process registered can be cancelled by a close() we make; another process's locks are irrelevant. Without this, a later "just refuse whenever the file looks busy" change would silently turn every doctor row into "could not be read". - the refusal creates no new -wal/-shm sidecars, which is the property the function's docstring promises and the reason it byte-probes rather than opening a connection in the first place. --- tests/hermes_cli/test_doctor_journal_modes.py | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/tests/hermes_cli/test_doctor_journal_modes.py b/tests/hermes_cli/test_doctor_journal_modes.py index acc56ff0ea2a..94fccc1ea7a1 100644 --- a/tests/hermes_cli/test_doctor_journal_modes.py +++ b/tests/hermes_cli/test_doctor_journal_modes.py @@ -14,6 +14,12 @@ import pytest import hermes_cli.doctor as doctor +from hermes_cli.sqlite_safe_read import ( + connect_tracked, + has_live_connection, + track_connection, + untrack_connection, +) VULNERABLE = (3, 50, 4) FIXED_VERSIONS = [(3, 51, 3), (3, 52, 0), (3, 50, 7), (3, 44, 6)] @@ -38,6 +44,16 @@ def _sidecars(directory): ) +@pytest.fixture +def clean_registry(): + """Keep the module-level connection registry from leaking across tests.""" + yield + import hermes_cli.sqlite_safe_read as mod + + with mod._live_lock: + mod._live_connections.clear() + + class TestReadJournalMode: def test_reads_wal(self, tmp_path): db = tmp_path / "state.db" @@ -141,6 +157,113 @@ def test_does_not_mutate_database_files(self, tmp_path): assert _sidecars(tmp_path) == [] +class TestLiveConnectionSafety: + """The probe must not raw-open a database this process has connections to. + + close() on any descriptor cancels every POSIX advisory lock the process + holds on that file, so a byte-probe run while a connection is live drops + that connection's locks — including the EXCLUSIVE lock a VACUUM holds + mid-rewrite. run_doctor is reachable in-process (the dashboard console + imports and calls it directly while holding live SessionDB connections), + so the probe must defer to the registry rather than open the file. + """ + + def test_probe_is_refused_while_a_tracked_connection_is_live( + self, tmp_path, clean_registry + ): + db = tmp_path / "state.db" + _make_db(db, journal_mode="WAL") + + track_connection(db) + try: + assert has_live_connection(db) + + mode, error = doctor._read_journal_mode(db) + + assert mode is None + assert error == "database is open in this process" + finally: + untrack_connection(db) + + def test_probe_is_refused_for_a_real_tracked_connection( + self, tmp_path, clean_registry + ): + """The same, through connect_tracked — the path SessionDB actually takes.""" + db = tmp_path / "state.db" + _make_db(db, journal_mode="WAL") + + conn = connect_tracked(db) + try: + assert has_live_connection(db) + + mode, error = doctor._read_journal_mode(db) + + assert mode is None + assert error == "database is open in this process" + finally: + conn.close() + + def test_probe_resumes_once_the_connection_closes(self, tmp_path, clean_registry): + db = tmp_path / "state.db" + _make_db(db, journal_mode="WAL") + + conn = connect_tracked(db) + assert doctor._read_journal_mode(db)[0] is None + conn.close() + + assert not has_live_connection(db) + assert doctor._read_journal_mode(db) == ("wal", None) + + def test_refusal_creates_no_new_sidecars(self, tmp_path, clean_registry): + db = tmp_path / "state.db" + _make_db(db, journal_mode="WAL") + + conn = connect_tracked(db) + try: + before = _sidecars(tmp_path) + + doctor._read_journal_mode(db) + + assert _sidecars(tmp_path) == before + finally: + conn.close() + + def test_report_degrades_instead_of_probing_a_live_database( + self, tmp_path, capsys, clean_registry + ): + db = tmp_path / "state.db" + _make_db(db, journal_mode="WAL") + + conn = connect_tracked(db) + try: + doctor._report_database_journal_modes(tmp_path, VULNERABLE) + finally: + conn.close() + + out = capsys.readouterr().out + assert "state.db: journal mode could not be read" in out + assert "database is open in this process" in out + assert "cannot rule out WAL exposure" in out + + def test_an_untracked_lock_holder_does_not_block_the_probe(self, tmp_path): + """Only this process's *registered* connections gate the read. + + A plain sqlite3.connect elsewhere is not in the registry, and a lock + held by another process is irrelevant — neither can be cancelled by a + close() we never perform. Guards against over-correcting into refusing + every read. + """ + db = tmp_path / "state.db" + _make_db(db) + holder = sqlite3.connect(db, isolation_level=None) + try: + holder.execute("BEGIN EXCLUSIVE") + + assert doctor._read_journal_mode(db) == ("rollback", None) + finally: + holder.close() + + class TestReportDatabaseJournalModes: def test_vulnerable_runtime_wal_db_is_exposed(self, tmp_path, capsys): _make_db(tmp_path / "state.db", journal_mode="WAL") From c2ada0dba80763a94f87933c39816867fe3f2955 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:36:50 -0700 Subject: [PATCH 4/5] test(cli): pin the diagnostic detail in doctor's unreadable-database rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_header_bytes_preopen answers None for a live connection, a missing file and an unreadable file alike, so the error string doctor prints is now chosen rather than inherited from the OSError. These cases pin that choice: the missing file keeps its errno text, and the chmod-000 file is still reported as a permission problem rather than collapsing into the generic message — the behaviour the raw open() gave before. test_reason_does_not_open_the_file is the load-bearing one. It patches builtins.open to raise and asserts _unreadable_reason still answers, which fixes the constraint that makes the helper safe to call on a database path at all: stat() and access() read metadata and take no file descriptor, so no close() of ours can cancel the file's advisory locks. A future edit that reached for open() here to get a better message would reintroduce the original bug on the error path, and this test fails loudly if it does. The root check is written as hasattr(os, "geteuid") and os.geteuid() == 0 rather than the bare call the surrounding tests use. skipif conditions are evaluated at collection time and os.geteuid is POSIX-only, so the bare form raises AttributeError and takes the whole module down on Windows. The pre-existing occurrences are left alone — #81926 and #84073 are already open against exactly those lines, and this only avoids adding a third instance of the same defect. --- tests/hermes_cli/test_doctor_journal_modes.py | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/hermes_cli/test_doctor_journal_modes.py b/tests/hermes_cli/test_doctor_journal_modes.py index 94fccc1ea7a1..0e36cb55c575 100644 --- a/tests/hermes_cli/test_doctor_journal_modes.py +++ b/tests/hermes_cli/test_doctor_journal_modes.py @@ -264,6 +264,49 @@ def test_an_untracked_lock_holder_does_not_block_the_probe(self, tmp_path): holder.close() +class TestUnreadableReason: + def test_missing_file_keeps_the_os_error_text(self, tmp_path): + reason = doctor._unreadable_reason(tmp_path / "gone.db") + + assert "No such file or directory" in reason + + @pytest.mark.skipif(os.name == "nt", reason="chmod is a no-op on Windows") + @pytest.mark.skipif( + # os.geteuid is POSIX-only, and a skipif condition is evaluated at + # collection time — calling it unguarded would raise AttributeError + # and take the whole module down on Windows. + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="root ignores file permissions", + ) + def test_unreadable_file_is_reported_as_permission_denied(self, tmp_path): + db = tmp_path / "state.db" + _make_db(db) + os.chmod(db, 0o000) + try: + mode, error = doctor._read_journal_mode(db) + finally: + os.chmod(db, 0o644) + + assert mode is None + assert "permission denied" in error.lower() + + def test_reason_does_not_open_the_file(self, tmp_path, monkeypatch): + """_unreadable_reason must answer from metadata only. + + It runs on database paths, so taking a descriptor would reintroduce + the very close() this module's guard exists to prevent. + """ + db = tmp_path / "state.db" + _make_db(db) + + def _fail(*args, **kwargs): + raise AssertionError("_unreadable_reason must not open the file") + + monkeypatch.setattr("builtins.open", _fail) + + assert doctor._unreadable_reason(db) == "file could not be read" + + class TestReportDatabaseJournalModes: def test_vulnerable_runtime_wal_db_is_exposed(self, tmp_path, capsys): _make_db(tmp_path / "state.db", journal_mode="WAL") From 833dc9f6c8147c4b0bb8f1573bde6eec611271d2 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:54:48 -0700 Subject: [PATCH 5/5] test(cli): make the doctor journal-mode registry fixture order-independent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clean_registry cleared the connection registry only on teardown, so it protected the tests that ran after it but not the test holding it. A leak from earlier in the session — a failed test that never reached its close(), or any test that does not take this fixture — would leave a stale entry behind, and read_header_bytes_preopen would then refuse for that stale reason instead of the one under test. The refusal assertions would still pass, but for the wrong reason, which is the failure mode a regression test can least afford. Clearing on entry as well makes the fixture independent of what ran before it, and the teardown clear now runs under try/finally so a failing test cannot skip it. --- tests/hermes_cli/test_doctor_journal_modes.py | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/hermes_cli/test_doctor_journal_modes.py b/tests/hermes_cli/test_doctor_journal_modes.py index 0e36cb55c575..55828de63134 100644 --- a/tests/hermes_cli/test_doctor_journal_modes.py +++ b/tests/hermes_cli/test_doctor_journal_modes.py @@ -46,12 +46,24 @@ def _sidecars(directory): @pytest.fixture def clean_registry(): - """Keep the module-level connection registry from leaking across tests.""" - yield + """Isolate a test from the module-level connection registry. + + Clears on both sides, not just teardown: a test that leaks a tracked + connection (an earlier failure, or a test that does not take this + fixture) would otherwise leave the registry dirty and make the *next* + test's refusal assertion pass for the wrong reason. + """ import hermes_cli.sqlite_safe_read as mod - with mod._live_lock: - mod._live_connections.clear() + def _clear(): + with mod._live_lock: + mod._live_connections.clear() + + _clear() + try: + yield + finally: + _clear() class TestReadJournalMode: