From 4a268c9e4af93dfa171535ce6a1c0b966cc1315d Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:00:57 +0530 Subject: [PATCH] fix(cli): keep doctor's read-only opens URI-safe and holder-gated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _session_count: back to main's raw sqlite mode=ro COUNT(*) via as_uri() — routing it through SessionDB(read_only=True) both re-introduced the raw f-string URI ('?'/'#' in the home path truncate it) and queries columns (s.archived) an unmigrated store lacks, so doctor would report a healthy DB as broken. - _write_health_reason: snapshot source URI built with as_uri() for the same reason; the --fix live probe (_db_opens_cleanly runs BEGIN IMMEDIATE) now falls back to the snapshot unless live_writer_holds_db proves the store quiet, matching _state_db_wal — hermes doctor --fix never becomes a second writer against a gateway's state.db (#103339). - SessionDB._connect_read_only: same as_uri() form so every read-only opener is safe in a home containing '?' or '#'. - test_sessions_export_output_dir: fixture accepts the read_only kwarg the PR introduced. - Drop the two doctor tests that pinned the SessionDB factory kwargs; main's URI-reserved-chars test covers _session_count. Co-authored-by: Ahmett101 --- hermes_cli/doctor_state.py | 21 ++++++----- hermes_state.py | 3 +- .../test_observational_sessiondb_modes.py | 36 ------------------- .../test_sessions_export_output_dir.py | 2 +- 4 files changed, 16 insertions(+), 46 deletions(-) diff --git a/hermes_cli/doctor_state.py b/hermes_cli/doctor_state.py index 54be21633b..239fe0fa32 100644 --- a/hermes_cli/doctor_state.py +++ b/hermes_cli/doctor_state.py @@ -149,26 +149,31 @@ def _check_directory_structure(should_fix: bool, f: Finding) -> None: def _session_count(state_db_path: Path): - """COUNT(*) through a read-only SessionDB — never a writer, so schema auto-repair cannot run.""" - from hermes_state import SessionDB - db = SessionDB(db_path=state_db_path, read_only=True) + import sqlite3 + # mode=ro: doctor is a reader; a writable open of a gateway-held WAL DB is the second-writer class (#103339). + # as_uri() percent-encodes '?' / '#' in the home path; a raw f-string URI truncates there. + conn = sqlite3.connect(Path(state_db_path).resolve().as_uri() + "?mode=ro", uri=True) try: - return db.session_count() + return conn.execute("SELECT COUNT(*) FROM sessions").fetchone()[0] finally: - db.close() + conn.close() def _write_health_reason(state_db_path: Path, *, isolate: bool): """FTS/write-health probe. Isolated copies never join the live store WAL lifecycle.""" - from hermes_state_repair import _db_opens_cleanly - if not isolate: + from hermes_state_repair import _connect_repair_durable, _db_opens_cleanly + from hermes_state_holders import live_writer_holds_db + # Even under --fix the probe's BEGIN IMMEDIATE is a second writer against a gateway-held DB (#103339): + # only probe the live file when the holder scan proves it quiet, else fall back to a snapshot. + if not isolate and not live_writer_holds_db(state_db_path, connect_repair_durable=_connect_repair_durable): return _db_opens_cleanly(state_db_path) import sqlite3 import tempfile with tempfile.TemporaryDirectory() as tmp: snapshot = Path(tmp) / "state.db" try: - src = sqlite3.connect(f"file:{state_db_path}?mode=ro", uri=True, timeout=1.0) + # as_uri() percent-encodes '?' / '#' in the home path; a raw f-string URI truncates there. + src = sqlite3.connect(Path(state_db_path).resolve().as_uri() + "?mode=ro", uri=True, timeout=1.0) except sqlite3.Error as exc: return str(exc) try: diff --git a/hermes_state.py b/hermes_state.py index 84aa323d98..e3b97075ad 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -642,8 +642,9 @@ class SessionDB( def _connect_read_only(self, timeout: float) -> sqlite3.Connection: """``mode=ro`` tracked connection with Row factory. check_same_thread=False: pooled connections are borrowed by whichever thread reads next; exclusive ownership is enforced by pool checkout.""" + # as_uri() percent-encodes '?' / '#' in the home path; a raw f-string URI truncates there. conn = _connect_tracked_db( - f"file:{self.db_path}?mode=ro", tracking_path=self.db_path, uri=True, + Path(self.db_path).resolve().as_uri() + "?mode=ro", tracking_path=self.db_path, uri=True, check_same_thread=False, timeout=timeout, isolation_level=None, ) conn.row_factory = sqlite3.Row diff --git a/tests/hermes_cli/test_observational_sessiondb_modes.py b/tests/hermes_cli/test_observational_sessiondb_modes.py index 1a38c98ff9..243e8d7070 100644 --- a/tests/hermes_cli/test_observational_sessiondb_modes.py +++ b/tests/hermes_cli/test_observational_sessiondb_modes.py @@ -20,42 +20,6 @@ def test_status_session_summary_opens_a_read_only_store(monkeypatch): db.close.assert_called_once() -def test_doctor_without_fix_counts_sessions_through_read_only_store(monkeypatch, tmp_path): - from hermes_cli.doctor_report import Finding - from hermes_cli.doctor_state import _state_db_health - - db_path = tmp_path / "state.db" - db_path.touch() - db = MagicMock() - db.session_count.return_value = 3 - factory = MagicMock(return_value=db) - monkeypatch.setattr("hermes_state.SessionDB", factory) - monkeypatch.setattr("hermes_state_repair._db_opens_cleanly", lambda _path: None) - - _state_db_health(Finding(), False, db_path, "~/hermes") - - factory.assert_called_once_with(db_path=db_path, read_only=True) - db.close.assert_called_once() - - -def test_doctor_with_fix_also_counts_through_read_only_store(monkeypatch, tmp_path): - from hermes_cli.doctor_report import Finding - from hermes_cli.doctor_state import _state_db_health - - db_path = tmp_path / "state.db" - db_path.touch() - db = MagicMock() - db.session_count.return_value = 3 - factory = MagicMock(return_value=db) - monkeypatch.setattr("hermes_state.SessionDB", factory) - monkeypatch.setattr("hermes_state_repair._db_opens_cleanly", lambda _path: None) - - _state_db_health(Finding(), True, db_path, "~/hermes") - - factory.assert_called_once_with(db_path=db_path, read_only=True) - db.close.assert_called_once() - - def test_sessions_list_stats_and_pinned_open_a_read_only_store(monkeypatch): factory = MagicMock() monkeypatch.setattr("hermes_state.SessionDB", factory) diff --git a/tests/hermes_cli/test_sessions_export_output_dir.py b/tests/hermes_cli/test_sessions_export_output_dir.py index 0eaf01df85..f2d26ee264 100644 --- a/tests/hermes_cli/test_sessions_export_output_dir.py +++ b/tests/hermes_cli/test_sessions_export_output_dir.py @@ -19,7 +19,7 @@ class _FakeDB: def _export(monkeypatch, *argv): - monkeypatch.setattr(hermes_state, "SessionDB", lambda: _FakeDB()) + monkeypatch.setattr(hermes_state, "SessionDB", lambda *args, **kwargs: _FakeDB()) monkeypatch.setattr(sys, "argv", ["hermes", "sessions", "export", "--session-id", "sess", *argv]) main_mod.main()