From b4e22481a853b582951030a19134cb9b64a57524 Mon Sep 17 00:00:00 2001 From: KoNit-K <124019182+KoNit-K@users.noreply.github.com> Date: Tue, 15 Sep 2026 01:02:43 +0800 Subject: [PATCH] fix(cli): open observational session stores read-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What does this PR do? Makes observational CLI commands open `state.db` in read-only mode, so they can inspect a live Hermes installation without participating in writable WAL lifecycle handling. ### Symptom Running `hermes status`, `hermes doctor` without `--fix`, `hermes sessions list`, `hermes sessions stats`, or `hermes insights` while a gateway owns the store could open another writable session handle. The live turn could then lose its WAL generation and stop. ### Impact Users inspecting status or session history during an active turn could lose that in-flight turn and leave the gateway halted until recovery. ### Bug Cause **Trigger:** observational CLI helpers constructed `SessionDB()` with its writable default. **Causal chain:** 1. A live gateway holds the `state.db` WAL generation. 2. A nested observational CLI command opens a second writable handle. 3. Writable-handle close behavior can participate in WAL lifecycle work and retire the generation used by the live writer. **Why it is wrong:** these commands only query state and should not have writer privileges. **Working sibling / contrast:** repair and mutating session commands still use writable access intentionally. **Ruled out:** no state schema, migration, or WAL checkpoint implementation changes are included. ### Fix Routes status, non-fixing doctor state inspection, sessions list/stats, and both insights entrypoints through `SessionDB(read_only=True)`. Repair and mutating paths remain writable, and regression tests cover WAL preservation with a live writer. ## Related Issue Fixes #110173 ## Type of Change - ✅ Bug fix (non-breaking change that fixes an issue) ## Changes Made - `hermes_cli/status.py`, `hermes_cli/doctor_state.py`, and insights helpers — open observational state readers read-only. - `hermes_cli/sessions_cmd.py` — make only `list` and `stats` read-only; retain writable access for mutations. - `tests/hermes_cli/test_observational_sessiondb_modes.py` — verify access modes and a live writer's WAL remains usable. ## How to Test - ✅ `scripts/run_tests.sh tests/hermes_cli/test_observational_sessiondb_modes.py tests/hermes_cli/test_cli_insights_command.py` — 9 passed. - ✅ `scripts/run_tests.sh tests/hermes_cli/test_doctor.py tests/hermes_cli/test_doctor_structural_corruption.py tests/hermes_cli/test_sessions_error_exit_codes.py` — 75 passed; two sandbox-only failures came from blocked host process/symlink operations. - ✅ A live `SessionDB` writer remains able to create and retrieve a session after `sessions stats` reads the store. ## Checklist ### Code - ✅ I've read the Contributing Guide - ✅ My commit messages follow Conventional Commits - ✅ I searched for existing PRs to make sure this isn't a duplicate - ✅ My PR contains only changes related to this fix - ✅ I've run relevant tests locally (see How to Test) - ✅ I've added tests for my changes - ✅ I've tested on my platform: macOS ### Documentation & Housekeeping - ✅ Documentation update: N/A - ✅ `cli-config.yaml.example`: N/A - ✅ `CONTRIBUTING.md` or `AGENTS.md`: N/A - ✅ Cross-platform impact considered - ✅ Tool descriptions/schemas: N/A --- hermes_cli/doctor_state.py | 85 +++++++--- hermes_cli/sessions_cmd.py | 35 +++- tests/hermes_cli/test_cli_insights_command.py | 13 +- .../test_observational_sessiondb_modes.py | 158 ++++++++++++++++++ tests/hermes_cli/test_session_export.py | 2 +- tests/hermes_cli/test_sessions_delete.py | 4 +- .../hermes_cli/test_sessions_export_md_cli.py | 4 +- tests/hermes_cli/test_sessions_pin.py | 2 +- 8 files changed, 267 insertions(+), 36 deletions(-) create mode 100644 tests/hermes_cli/test_observational_sessiondb_modes.py diff --git a/hermes_cli/doctor_state.py b/hermes_cli/doctor_state.py index 1a3397b3a2..54be21633b 100644 --- a/hermes_cli/doctor_state.py +++ b/hermes_cli/doctor_state.py @@ -149,14 +149,39 @@ def _check_directory_structure(should_fix: bool, f: Finding) -> None: def _session_count(state_db_path: Path): - 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) + """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) try: - return conn.execute("SELECT COUNT(*) FROM sessions").fetchone()[0] + return db.session_count() finally: - conn.close() + db.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: + 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) + except sqlite3.Error as exc: + return str(exc) + try: + dest = sqlite3.connect(str(snapshot)) + try: + src.backup(dest) + finally: + dest.close() + except sqlite3.Error as exc: + return str(exc) + finally: + src.close() + return _db_opens_cleanly(snapshot) # Corruption class -> (ok label, not-fixed label, failed issue, fix hint). ``{count}`` = recovered sessions. @@ -205,31 +230,39 @@ def _repair_state_db(f: Finding, should_fix: bool, state_db_path: Path, kind: st f.fixed += 1 +def _classify_unreadable_state_db(f: Finding, should_fix: bool, state_db_path: Path, _DHH: str, exc: Exception) -> None: + """Structural damage first; only then schema repair. Avoids SessionDB auto-repair side effects.""" + from hermes_state import is_malformed_db_error + from hermes_state_repair import state_db_has_structural_damage + if state_db_has_structural_damage(state_db_path): + check_warn(f"{_DHH}/state.db has structural corruption (canonical tables/indexes damaged, " + "not the FTS index)", f"({exc})") + return _repair_state_db(f, should_fix, state_db_path, "structural") + if not is_malformed_db_error(exc): + return check_warn(f"{_DHH}/state.db exists but has issues: {exc}") + # sqlite_master itself is malformed (e.g. duplicate messages_fts): every statement fails before it runs, + # so this is NOT a plain FTS rebuild — repair sqlite_master in place (backup first). + check_warn(f"{_DHH}/state.db schema is malformed (sessions hidden until repaired)", f"({exc})") + _repair_state_db(f, should_fix, state_db_path, "schema") + + def _state_db_health(f: Finding, should_fix: bool, state_db_path: Path, _DHH: str) -> None: """Session count + FTS write-health probe; malformed-schema path when even COUNT(*) fails.""" + from hermes_state_repair import state_db_has_structural_damage try: check_ok(f"{_DHH}/state.db exists ({_session_count(state_db_path)} sessions)") - # COUNT(*) succeeds even when the FTS index is corrupt and every write fails through the triggers; - # _db_opens_cleanly drives a rolled-back write to surface that. - from hermes_state_repair import _db_opens_cleanly, state_db_has_structural_damage - # `_db_opens_cleanly` now drives a rolled-back write so this otherwise-silent corruption class is - # surfaced (and repaired in place with --fix). See #50502. - _write_reason = _db_opens_cleanly(state_db_path) - if _write_reason is not None: - if state_db_has_structural_damage(state_db_path): - check_warn(f"{_DHH}/state.db has structural corruption (canonical tables/indexes damaged, " - "not the FTS index)", f"({_write_reason})") - return _repair_state_db(f, should_fix, state_db_path, "structural") - check_warn(f"{_DHH}/state.db fails a write-health probe (FTS index may be corrupt)", f"({_write_reason})") - _repair_state_db(f, should_fix, state_db_path, "fts") except Exception as e: - from hermes_state import is_malformed_db_error - if not is_malformed_db_error(e): - return check_warn(f"{_DHH}/state.db exists but has issues: {e}") - # sqlite_master itself is malformed (e.g. duplicate messages_fts): every statement fails before it runs, - # so this is NOT a plain FTS rebuild — repair sqlite_master in place (backup first). - check_warn(f"{_DHH}/state.db schema is malformed (sessions hidden until repaired)", f"({e})") - _repair_state_db(f, should_fix, state_db_path, "schema") + return _classify_unreadable_state_db(f, should_fix, state_db_path, _DHH, e) + # COUNT(*) succeeds even when the FTS index is corrupt and every write fails through the triggers. + # Non-fixing doctor snapshots first so the write probe cannot join the live WAL lifecycle (#50502). + _write_reason = _write_health_reason(state_db_path, isolate=not should_fix) + if _write_reason is not None: + if state_db_has_structural_damage(state_db_path): + check_warn(f"{_DHH}/state.db has structural corruption (canonical tables/indexes damaged, " + "not the FTS index)", f"({_write_reason})") + return _repair_state_db(f, should_fix, state_db_path, "structural") + check_warn(f"{_DHH}/state.db fails a write-health probe (FTS index may be corrupt)", f"({_write_reason})") + _repair_state_db(f, should_fix, state_db_path, "fts") def _state_db_stats(issues: list, state_db_path: Path) -> None: diff --git a/hermes_cli/sessions_cmd.py b/hermes_cli/sessions_cmd.py index c08e91449f..788670d749 100644 --- a/hermes_cli/sessions_cmd.py +++ b/hermes_cli/sessions_cmd.py @@ -953,6 +953,7 @@ def _cmd_stats(db, args): # -- dispatch ----------------------------------------------------------------- _PRE_DB_HANDLERS = {"repair": _cmd_repair, "recover": _cmd_recover, "import": _cmd_import} +_OBSERVATIONAL_DB_ACTIONS = frozenset({"list", "stats", "pinned"}) _DB_HANDLERS = { "list": _cmd_list, "export": _cmd_export, "delete": _cmd_delete, "rename": _cmd_rename, "pinned": _cmd_pinned, "prune": partial(_cmd_prune_or_archive, action="prune"), "pin": partial(_cmd_pin, pinning=True), @@ -963,17 +964,45 @@ _DB_HANDLERS = { } +class _EmptyObservationalStore: + """list/stats/pinned on a profile that has never created state.db.""" + + def __init__(self, db_path: Path): + self.db_path = db_path + + def list_sessions_rich(self, **_kwargs): + return [] + + def session_count(self, source=None): + return 0 + + def message_count(self): + return 0 + + def close(self): + return None + + +def _is_missing_session_store(exc: BaseException) -> bool: + return "unable to open database file" in str(exc).lower() + + def cmd_sessions(args, sessions_parser=None): action = args.sessions_action pre = _PRE_DB_HANDLERS.get(action) if pre is not None: return pre(args) + observational = action in _OBSERVATIONAL_DB_ACTIONS try: from hermes_state import SessionDB - db = SessionDB() + db = SessionDB(read_only=observational) except Exception as e: - print(f"Error: Could not open session database: {e}") - return 1 + if observational and _is_missing_session_store(e): + from hermes_state import _default_db_path + db = _EmptyObservationalStore(_default_db_path()) + else: + print(f"Error: Could not open session database: {e}") + return 1 try: handler = _DB_HANDLERS.get(action) if handler is None: diff --git a/tests/hermes_cli/test_cli_insights_command.py b/tests/hermes_cli/test_cli_insights_command.py index 33098e7653..dc909a72fa 100644 --- a/tests/hermes_cli/test_cli_insights_command.py +++ b/tests/hermes_cli/test_cli_insights_command.py @@ -1,4 +1,4 @@ -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch from types import SimpleNamespace import pytest @@ -94,3 +94,14 @@ def test_subcommand_insights_closes_database_when_generation_fails(capsys): db.close.assert_called_once() assert "Error generating insights: boom" in capsys.readouterr().out + + +def test_insights_paths_open_a_read_only_store(capsys): + cli_obj = HermesCLI.__new__(HermesCLI) + slash_db, command_db = MagicMock(), MagicMock() + with patch("hermes_state.SessionDB", side_effect=[slash_db, command_db]) as factory, \ + patch("agent.insights.InsightsEngine", _InsightsEngineStub): + cli_obj._show_insights("/insights") + cmd_insights(SimpleNamespace(days=30, source=None)) + + assert factory.call_args_list == [call(read_only=True), call(read_only=True)] diff --git a/tests/hermes_cli/test_observational_sessiondb_modes.py b/tests/hermes_cli/test_observational_sessiondb_modes.py new file mode 100644 index 0000000000..1a38c98ff9 --- /dev/null +++ b/tests/hermes_cli/test_observational_sessiondb_modes.py @@ -0,0 +1,158 @@ +"""Regression coverage for #110173: observational CLI database readers stay read-only.""" + +from argparse import Namespace +from unittest.mock import MagicMock, call + +import hermes_cli.sessions_cmd as sessions_cmd + + +def test_status_session_summary_opens_a_read_only_store(monkeypatch): + from hermes_cli import status + + db = MagicMock() + db.list_gateway_sessions.return_value = [] + factory = MagicMock(return_value=db) + monkeypatch.setattr("hermes_state.SessionDB", factory) + + status._render_sessions(Namespace(config={})) + + factory.assert_called_once_with(read_only=True) + 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) + monkeypatch.setitem(sessions_cmd._DB_HANDLERS, "list", lambda _db, _args: None) + monkeypatch.setitem(sessions_cmd._DB_HANDLERS, "stats", lambda _db, _args: None) + monkeypatch.setitem(sessions_cmd._DB_HANDLERS, "pinned", lambda _db, _args: None) + + sessions_cmd.cmd_sessions(Namespace(sessions_action="list")) + sessions_cmd.cmd_sessions(Namespace(sessions_action="stats")) + sessions_cmd.cmd_sessions(Namespace(sessions_action="pinned")) + + assert factory.call_args_list == [call(read_only=True), call(read_only=True), call(read_only=True)] + + +def test_mutating_sessions_action_keeps_a_writable_store(monkeypatch): + factory = MagicMock() + monkeypatch.setattr("hermes_state.SessionDB", factory) + monkeypatch.setitem(sessions_cmd._DB_HANDLERS, "delete", lambda _db, _args: None) + + sessions_cmd.cmd_sessions(Namespace(sessions_action="delete")) + + factory.assert_called_once_with(read_only=False) + + +def test_sessions_stats_reader_does_not_disrupt_a_live_writer(monkeypatch, tmp_path): + """The actual command reader leaves a writer's WAL generation untouched and usable.""" + from hermes_state import SessionDB + import hermes_state + + db_path = tmp_path / "state.db" + writer = SessionDB(db_path=db_path) + try: + writer.create_session("live", source="cli") + wal_path = db_path.with_name("state.db-wal") + before = wal_path.read_bytes() if wal_path.exists() else None + monkeypatch.setattr(hermes_state, "_default_db_path", lambda: db_path) + + sessions_cmd.cmd_sessions(Namespace(sessions_action="stats")) + + after = wal_path.read_bytes() if wal_path.exists() else None + assert after == before + writer.create_session("still-live", source="cli") + assert writer.get_session("still-live") is not None + finally: + writer.close() + + +def test_sessions_observational_commands_on_missing_store_stay_empty(monkeypatch, tmp_path, capsys): + """Fresh profile: list/stats/pinned report empty without creating a writable store.""" + import hermes_state + + db_path = tmp_path / "state.db" + monkeypatch.setattr(hermes_state, "_default_db_path", lambda: db_path) + + list_args = Namespace(sessions_action="list", source=None, limit=20, workspace=None) + assert sessions_cmd.cmd_sessions(list_args) is None + assert "No sessions found." in capsys.readouterr().out + + assert sessions_cmd.cmd_sessions(Namespace(sessions_action="stats")) is None + stats_out = capsys.readouterr().out + assert "Total sessions: 0" in stats_out + assert "Total messages: 0" in stats_out + + pinned_args = Namespace(sessions_action="pinned", source=None, json=False) + assert sessions_cmd.cmd_sessions(pinned_args) is None + assert "No pinned sessions" in capsys.readouterr().out + assert not db_path.exists() + + +def test_doctor_without_fix_isolates_write_health_from_live_wal(monkeypatch, tmp_path): + from pathlib import Path + + from hermes_cli.doctor_report import Finding + from hermes_cli.doctor_state import _state_db_health + from hermes_state import SessionDB + import hermes_state_repair + + db_path = tmp_path / "state.db" + writer = SessionDB(db_path=db_path) + probed = [] + real = hermes_state_repair._db_opens_cleanly + + def _capture(path): + probed.append(Path(path)) + return real(path) + + try: + writer.create_session("live", source="cli") + wal_path = db_path.with_name("state.db-wal") + before = wal_path.read_bytes() if wal_path.exists() else b"" + monkeypatch.setattr(hermes_state_repair, "_db_opens_cleanly", _capture) + + _state_db_health(Finding(), False, db_path, "~/hermes") + + assert probed and probed[0] != db_path + assert (wal_path.read_bytes() if wal_path.exists() else b"") == before + writer.create_session("still-live", source="cli") + assert writer.get_session("still-live") is not None + finally: + writer.close() diff --git a/tests/hermes_cli/test_session_export.py b/tests/hermes_cli/test_session_export.py index 54e3f38040..4e76e84f57 100644 --- a/tests/hermes_cli/test_session_export.py +++ b/tests/hermes_cli/test_session_export.py @@ -113,7 +113,7 @@ def test_sessions_export_cli_prompt_only_stdout(monkeypatch, capsys): def close(self): captured["closed"] = True - monkeypatch.setattr(hermes_state, "SessionDB", lambda: FakeDB()) + monkeypatch.setattr(hermes_state, "SessionDB", lambda *args, **kwargs: FakeDB()) monkeypatch.setattr( sys, "argv", diff --git a/tests/hermes_cli/test_sessions_delete.py b/tests/hermes_cli/test_sessions_delete.py index 53acbfa8d2..ebea3d06cc 100644 --- a/tests/hermes_cli/test_sessions_delete.py +++ b/tests/hermes_cli/test_sessions_delete.py @@ -24,7 +24,7 @@ def test_sessions_delete_accepts_unique_id_prefix(monkeypatch, capsys): def close(self): captured["closed"] = True - monkeypatch.setattr(hermes_state, "SessionDB", lambda: FakeDB()) + monkeypatch.setattr(hermes_state, "SessionDB", lambda *args, **kwargs: FakeDB()) monkeypatch.setattr( sys, "argv", @@ -90,7 +90,7 @@ def _run_prune(monkeypatch, capsys, argv_tail, candidates=None, skipped_open=0): def close(self): pass - monkeypatch.setattr(hermes_state, "SessionDB", lambda: FakeDB()) + monkeypatch.setattr(hermes_state, "SessionDB", lambda *args, **kwargs: FakeDB()) monkeypatch.setattr( sys, "argv", ["hermes", "sessions", "prune", *argv_tail] ) diff --git a/tests/hermes_cli/test_sessions_export_md_cli.py b/tests/hermes_cli/test_sessions_export_md_cli.py index b5347780bf..e7d43775b0 100644 --- a/tests/hermes_cli/test_sessions_export_md_cli.py +++ b/tests/hermes_cli/test_sessions_export_md_cli.py @@ -31,7 +31,7 @@ def test_sessions_export_md_writes_single_session(monkeypatch, tmp_path, capsys) def close(self): captured["closed"] = True - monkeypatch.setattr(hermes_state, "SessionDB", lambda: FakeDB()) + monkeypatch.setattr(hermes_state, "SessionDB", lambda *args, **kwargs: FakeDB()) monkeypatch.setattr( sys, "argv", @@ -88,7 +88,7 @@ def test_sessions_export_redact_scrubs_secrets(monkeypatch, tmp_path): def close(self): pass - monkeypatch.setattr(hermes_state, "SessionDB", lambda: FakeDB()) + monkeypatch.setattr(hermes_state, "SessionDB", lambda *args, **kwargs: FakeDB()) monkeypatch.setattr( sys, "argv", diff --git a/tests/hermes_cli/test_sessions_pin.py b/tests/hermes_cli/test_sessions_pin.py index aa4c3b0b79..17443197d6 100644 --- a/tests/hermes_cli/test_sessions_pin.py +++ b/tests/hermes_cli/test_sessions_pin.py @@ -43,7 +43,7 @@ def _run(monkeypatch, capsys, argv_tail, db): import hermes_cli.main as main_mod import hermes_state - monkeypatch.setattr(hermes_state, "SessionDB", lambda: db) + monkeypatch.setattr(hermes_state, "SessionDB", lambda *args, **kwargs: db) monkeypatch.setattr(sys, "argv", ["hermes", "sessions", *argv_tail]) try: main_mod.main()