fix(cli): open observational session stores read-only
## 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
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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)]
|
||||
|
||||
158
tests/hermes_cli/test_observational_sessiondb_modes.py
Normal file
158
tests/hermes_cli/test_observational_sessiondb_modes.py
Normal file
@@ -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()
|
||||
@@ -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",
|
||||
|
||||
@@ -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]
|
||||
)
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user