From 0d3ee7de9f9f865688531ea77b0a03ff1ef97e04 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 9 Jul 2026 09:04:59 -0700 Subject: [PATCH] Release Batch A: worktree cli-shadow + atomic settings.json + state.db read-only (#5832, #5829, #5833) (#5837) Phase 1 low-risk backend reliability batch. Three independent contributor fixes on distinct files, each byte-identical to its PR head, Codex SAFE, 691+16 tests pass. - #5832 (silent-reader-cn): importlib-load agent cli.py by absolute path so a shadowing cli/ namespace package can't brick worktree creation - #5829 (ai-ag2026): atomic settings.json writes (temp+fsync+os.replace, mode-preserving) - #5833 (ai-ag2026): open the remaining 8 state.db read projections read-only Co-authored-by: t Co-authored-by: silent-reader-cn Co-authored-by: ai-ag2026 --- CHANGELOG.md | 6 + api/config.py | 66 +++++- api/models.py | 17 +- api/worktrees.py | 23 ++- tests/test_atomic_settings_writes.py | 109 ++++++++++ tests/test_state_db_readonly_reads_models.py | 202 +++++++++++++++++++ 6 files changed, 409 insertions(+), 14 deletions(-) create mode 100644 tests/test_atomic_settings_writes.py create mode 100644 tests/test_state_db_readonly_reads_models.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 67cf3c05..32f9b345 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ ### Fixed +- **Creating a worktree-backed session no longer fails when another `cli` package shadows the agent's `cli.py`.** The worktree helper imported the agent's `cli` module with a bare `from cli import ...`, which an unrelated `cli/` namespace package earlier on `sys.path` (e.g. a dependency like `stringzilla`) could preempt — bricking worktree creation. It now loads the agent's `cli.py` by absolute path via `importlib`, bypassing `sys.path` resolution, and preserves the existing failure contract. Thanks @silent-reader-cn. (#5832) + +- **`settings.json` is now written atomically so a crash mid-write can't wipe your settings.** `save_settings()` and the startup default-workspace rewrite used a truncating in-place write, so a crash, full disk, or power loss between truncate and full write could leave `settings.json` empty — losing theme, workspace, tab order, and the login `password_hash` (silently dropping to no-auth). Both writes now go through a same-directory temp file → `fsync` → `os.replace`, preserving the file's existing permission mode (a hardened `0600` stays `0600`) and writing through symlinks. Thanks @ai-ag2026. (#5829) + +- **The remaining `state.db` read projections now open the database read-only.** Eight pure-read session/model projections still opened `state.db` with a writable connection; they now use the read-only (`mode=ro`) URI helper, matching the rest of the read-path hardening. Behavior is unchanged (all sites are pure `SELECT`/`PRAGMA` guarded by existence checks); the two genuinely-writable paths are correctly left alone. Thanks @ai-ag2026. (#5833) + - **A stuck or half-open gateway connection no longer pins a turn for 10 minutes ignoring Stop.** Gateway SSE reads now carry a bounded byte-silence budget (default 600s, env-tunable via `HERMES_WEBUI_GATEWAY_READ_TIMEOUT`) and treat a read timeout as terminal — so pressing Stop during a silent stall ends the turn cleanly instead of waiting out the full timeout. The 600s default preserves the prior budget so legitimately long, silent tool calls are not cut off. Thanks @ai-ag2026. (#5789, #2476) - **Session loading is faster — addresses the "WebUI feels laggy" reports.** The session-load path (`/api/sessions` + session-by-id) had redundant full-metadata reads on every request; it now uses a profile-scoped, content-verified, lock-guarded cache (capped at 32 entries) with a fast path that only serves cached data when disk state is provably fresh, falling back to full comparison otherwise. Response shape and profile-scoping are unchanged. Thanks @Kopamed. (#5803, #5455) diff --git a/api/config.py b/api/config.py index 8d4fd4ee..d5030c69 100644 --- a/api/config.py +++ b/api/config.py @@ -8668,6 +8668,64 @@ _SETTINGS_WRITE_VERSION = 0 _SETTINGS_WRITE_LOCK = __import__("threading").Lock() +def _atomic_write_settings_text(path: Path, text: str) -> None: + """Write *text* to *path* atomically (temp file + fsync + os.replace). + + ``settings.json`` was rewritten with a plain ``Path.write_text``, which + truncates the file in place: a crash or full disk mid-write leaves it + truncated/empty, so the next start loses every persisted setting (theme, + workspace, tab order, and the login ``password_hash``). Writing to a + sibling temp file, fsyncing, then ``os.replace`` keeps the old contents + intact until the rename commits the new ones in one step. Mirrors the + tempfile+fsync+os.replace pattern already used by + ``webui_session_db.WebUIJsonSessionDB._atomic_write``. + + The existing file's mode is carried onto the replacement: ``os.replace`` + swaps in the temp file's inode, and a plain ``open`` respects the umask + (typically 0644), so without this an operator-hardened ``settings.json`` + (chmod 0600 because it holds the password hash) would be silently loosened + on the next save. New files fall back to the umask-adjusted default. + + A symlinked target is written through to its referent (same follow-through + as the ``Path.write_text`` this replaces), rather than replacing the link + itself with a regular file. + """ + path = Path(path) + write_path = path.resolve(strict=False) if path.is_symlink() else path + tmp = write_path.with_name( + f".{write_path.name}.{os.getpid()}.{threading.get_ident()}.tmp" + ) + try: + mode = os.stat(write_path).st_mode & 0o777 + except FileNotFoundError: + mode = 0o666 & ~_current_umask() + try: + with open(tmp, "w", encoding="utf-8") as handle: + handle.write(text) + handle.flush() + os.fsync(handle.fileno()) + os.chmod(tmp, mode) + os.replace(tmp, write_path) + finally: + try: + os.unlink(tmp) + except OSError: + pass + + +def _current_umask() -> int: + """Read the process umask without leaving it changed. + + ``os.umask`` has no read-only form (it sets and returns the prior value), + so we set-then-restore. Called only on the new-``settings.json`` path, which + is rare; the tiny set-to-0 window is acceptable here (unlike a per-write + hot path). + """ + umask = os.umask(0) + os.umask(umask) + return umask + + def _coerce_provider_cost_budget(value: Any) -> float | None: """Normalize a monthly budget to the persisted two-decimal representation.""" try: @@ -8827,9 +8885,9 @@ def save_settings(settings: dict) -> dict: effective_persisted_speech_keys.update(applied_speech_keys) persisted = _settings_payload_for_write(current, effective_persisted_speech_keys) SETTINGS_FILE.parent.mkdir(parents=True, exist_ok=True) - SETTINGS_FILE.write_text( + _atomic_write_settings_text( + SETTINGS_FILE, json.dumps(persisted, ensure_ascii=False, indent=2), - encoding="utf-8", ) global _SETTINGS_WRITE_VERSION with _SETTINGS_WRITE_LOCK: @@ -8870,7 +8928,8 @@ if _settings_file_exists: startup_persisted_speech_keys = _extract_persisted_speech_keys( _read_raw_settings_file() ) - SETTINGS_FILE.write_text( + _atomic_write_settings_text( + SETTINGS_FILE, json.dumps( _settings_payload_for_write( _startup_settings, startup_persisted_speech_keys @@ -8878,7 +8937,6 @@ if _settings_file_exists: ensure_ascii=False, indent=2, ), - encoding="utf-8", ) except Exception: pass diff --git a/api/models.py b/api/models.py index 38c3dfc0..cdddcbc9 100644 --- a/api/models.py +++ b/api/models.py @@ -27,6 +27,7 @@ from api.agent_sessions import ( _is_continuation_session, is_cli_session_row, normalize_agent_session_source, + open_state_db_readonly, read_importable_agent_session_rows, read_session_lineage_metadata, ) @@ -4140,7 +4141,7 @@ def state_db_has_session(sid: str) -> bool: if not db_path.exists(): return False try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: cur = conn.cursor() cur.execute("SELECT 1 FROM sessions WHERE id = ? LIMIT 1", (str(sid),)) return cur.fetchone() is not None @@ -4246,7 +4247,7 @@ def agent_session_rows_existing( if db_path is None: return frozenset(wanted) try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: cur = conn.cursor() cur.execute("PRAGMA table_info(sessions)") cols = {str(row[1]) for row in cur.fetchall()} @@ -4302,7 +4303,7 @@ def agent_session_zero_message_sids( if db_path is None: return frozenset() try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: cur = conn.cursor() cur.execute("PRAGMA table_info(sessions)") sessions_cols = {str(row[1]) for row in cur.fetchall()} @@ -4399,7 +4400,7 @@ def _read_state_db_sidebar_overrides( except ImportError: return {} try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute("PRAGMA table_info(sessions)") @@ -6483,7 +6484,7 @@ def get_state_db_session_messages( return [] try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute("PRAGMA table_info(messages)") @@ -6629,7 +6630,7 @@ def get_state_db_session_message_keys_before_timestamp( return [] try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute("PRAGMA table_info(messages)") @@ -6679,7 +6680,7 @@ def get_state_db_session_summary(sid, *, profile=None) -> dict: return {"message_count": 0, "last_message_at": 0.0} try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute("PRAGMA table_info(messages)") @@ -7710,7 +7711,7 @@ def count_conversation_rounds(sid: str, since: float | None = None) -> int: return 0 try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute( diff --git a/api/worktrees.py b/api/worktrees.py index 42424ee5..d5aeb1e7 100644 --- a/api/worktrees.py +++ b/api/worktrees.py @@ -327,8 +327,27 @@ def find_git_repo_root(workspace: str | Path) -> Path: def _setup_agent_worktree(repo_root: str) -> dict: try: - import api.config # noqa: F401 # ensure Hermes Agent dir is on sys.path - from cli import _setup_worktree + import importlib.util + from pathlib import Path + + from api.config import _AGENT_DIR # Hermes Agent source root + + # Use importlib to load cli.py from its absolute path instead of a bare + # ``from cli import _setup_worktree``. The bare import is vulnerable to + # namespace-package shadowing — any third-party package (e.g. the ``cli`` + # namespace shipped by stringzilla) that creates a cli/ directory in + # site-packages will preempt the real hermes-agent/cli.py module. + cli_path = str(Path(_AGENT_DIR) / "cli.py") + spec = importlib.util.spec_from_file_location( + "hermes_cli_worktree", cli_path, + ) + if spec is None or spec.loader is None: + raise RuntimeError( + f"Could not locate hermes-agent worktree helper at {cli_path}" + ) + cli_mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(cli_mod) + _setup_worktree = cli_mod._setup_worktree except Exception as exc: raise RuntimeError("Hermes Agent worktree helper is unavailable") from exc output = StringIO() diff --git a/tests/test_atomic_settings_writes.py b/tests/test_atomic_settings_writes.py new file mode 100644 index 00000000..12eb24cf --- /dev/null +++ b/tests/test_atomic_settings_writes.py @@ -0,0 +1,109 @@ +"""Regression tests — settings.json is written atomically. + +`save_settings` (and the startup default-workspace rewrite) persisted +`settings.json` via a plain `Path.write_text`, which truncates the file in +place. A crash / full disk / power loss mid-write leaves it truncated or +empty, so the next start loses every persisted setting — theme, workspace, +tab order, and the login `password_hash` (losing the hash also silently +disables auth, but the data-loss regression is the point here). + +The write now goes through `_atomic_write_settings_text`: temp file in the +same dir, fsync, `os.replace`. A failure before the rename leaves the +ORIGINAL file byte-for-byte intact. These pin the helper directly (success, +mode preservation, crash-safety, symlink write-through) so a refactor can't +reintroduce the truncating plain write. +""" +import os +from pathlib import Path + +import pytest + +from api.config import _atomic_write_settings_text + + +def test_replaces_contents(tmp_path: Path) -> None: + target = tmp_path / "settings.json" + target.write_text('{"theme": "old"}', encoding="utf-8") + + _atomic_write_settings_text(target, '{"theme": "new"}') + + assert target.read_text(encoding="utf-8") == '{"theme": "new"}' + # No temp debris after a clean write. + assert [p.name for p in tmp_path.iterdir()] == ["settings.json"] + + +def test_creates_new_file(tmp_path: Path) -> None: + target = tmp_path / "settings.json" + _atomic_write_settings_text(target, '{"created": true}') + assert target.read_text(encoding="utf-8") == '{"created": true}' + + +def test_preserves_hardened_mode(tmp_path: Path) -> None: + """A 0600 settings.json (operator-hardened because it holds the password + hash) must not be loosened to 0644 by the atomic replace.""" + target = tmp_path / "settings.json" + target.write_text('{"password_hash": "x"}', encoding="utf-8") + os.chmod(target, 0o600) + + _atomic_write_settings_text(target, '{"password_hash": "y"}') + + assert (os.stat(target).st_mode & 0o777) == 0o600 + + +def test_failed_replace_leaves_original_intact(tmp_path: Path, monkeypatch) -> None: + target = tmp_path / "settings.json" + original = '{"theme": "keep-me"}' + target.write_text(original, encoding="utf-8") + + def _boom(src, dst): + raise RuntimeError("simulated crash before rename commits") + + monkeypatch.setattr(os, "replace", _boom) + + with pytest.raises(RuntimeError, match="simulated crash"): + _atomic_write_settings_text(target, '{"theme": "half-written"}') + + assert target.read_text(encoding="utf-8") == original + # Temp file cleaned up, not left as debris. + assert [p.name for p in tmp_path.iterdir()] == ["settings.json"] + + +def test_writes_through_symlink(tmp_path: Path) -> None: + real_dir = tmp_path / "real" + link_dir = tmp_path / "link" + real_dir.mkdir() + link_dir.mkdir() + target = real_dir / "settings.json" + link = link_dir / "settings.json" + target.write_text('{"theme": "old"}', encoding="utf-8") + link.symlink_to(target) + + _atomic_write_settings_text(link, '{"theme": "new"}') + + # The link stays a link; the referent got the new contents. + assert link.is_symlink() + assert target.read_text(encoding="utf-8") == '{"theme": "new"}' + assert [p.name for p in link_dir.iterdir()] == ["settings.json"] + + +def test_save_settings_uses_atomic_writer(tmp_path: Path, monkeypatch) -> None: + """End-to-end: save_settings must route through the atomic writer, not a + bare write_text (which a future edit could reintroduce).""" + import api.config as config + + settings_file = tmp_path / "settings.json" + monkeypatch.setattr(config, "SETTINGS_FILE", settings_file) + + calls = [] + real = config._atomic_write_settings_text + + def _spy(path, text): + calls.append(Path(path)) + return real(path, text) + + monkeypatch.setattr(config, "_atomic_write_settings_text", _spy) + + config.save_settings({"theme": "dark"}) + + assert settings_file in calls, "save_settings must use the atomic writer" + assert settings_file.exists() diff --git a/tests/test_state_db_readonly_reads_models.py b/tests/test_state_db_readonly_reads_models.py new file mode 100644 index 00000000..78e0b6cb --- /dev/null +++ b/tests/test_state_db_readonly_reads_models.py @@ -0,0 +1,202 @@ +"""Regression tests — the remaining pure-read state.db projections in +api/models.py open the live agent ``state.db`` read-only. + +#5455 moved the session-listing / lineage / gateway-watcher reads onto +``open_state_db_readonly`` (``file:...?mode=ro``) so a write-capable handle +doesn't add checkpoint/lock surface while the agent streams into the same WAL +DB. Eight more pure-read projections in api/models.py were still opening a +read-WRITE connection with ``sqlite3.connect(str(db_path))``. This shares them +onto the same helper. The lone write path (``delete_cli_session``, a DELETE) +deliberately keeps its writable connection. + +These pin that each converted read (a) routes through the read-only helper and +(b) still returns correct data, so a refactor can't silently reintroduce a +write-capable handle on the agent DB. +""" +import sqlite3 +from contextlib import closing + +import api.agent_sessions as agent_sessions +import api.models as models +from api.agent_sessions import open_state_db_readonly + + +def _make_state_db(path): + conn = sqlite3.connect(str(path)) + conn.execute( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, parent_session_id TEXT, end_reason TEXT, + source TEXT, session_source TEXT, title TEXT, + message_count INTEGER, started_at REAL, ended_at REAL + ) + """ + ) + conn.execute( + """ + CREATE TABLE messages ( + id INTEGER PRIMARY KEY, session_id TEXT, role TEXT, content TEXT, + timestamp REAL, tool_calls TEXT + ) + """ + ) + conn.execute( + "INSERT INTO sessions (id, source, session_source, title, message_count, " + "started_at, ended_at) VALUES (?,?,?,?,?,?,?)", + ("sess-1", "cli", "cli", "Root", 2, 1000.0, 1100.0), + ) + conn.executemany( + "INSERT INTO messages (id, session_id, role, content, timestamp, tool_calls) " + "VALUES (?,?,?,?,?,?)", + [ + (1, "sess-1", "user", "hi", 1000.0, None), + (2, "sess-1", "assistant", "hello", 1001.0, None), + ], + ) + conn.commit() + conn.close() + + +def _record_connects(monkeypatch): + """Spy on the connect that ``open_state_db_readonly`` uses (agent_sessions + namespace), recording target + uri per call.""" + real_connect = sqlite3.connect + calls = [] + + def spy(target, *args, **kwargs): + calls.append({"target": str(target), "uri": bool(kwargs.get("uri"))}) + return real_connect(target, *args, **kwargs) + + monkeypatch.setattr(agent_sessions.sqlite3, "connect", spy) + return calls + + +def _point_models_at(monkeypatch, db): + monkeypatch.setattr(models, "_active_state_db_path", lambda: db) + monkeypatch.setattr(models, "_agent_state_db_path", lambda *, profile=None: db) + + +def _assert_read_only(calls): + assert calls, "no state.db connection was opened" + assert calls[0]["uri"] is True, "connection was not opened via a URI (mode=ro)" + assert "mode=ro" in calls[0]["target"], "connection was not opened read-only" + + +def test_state_db_has_session_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + _point_models_at(monkeypatch, db) + calls = _record_connects(monkeypatch) + + assert models.state_db_has_session("sess-1") is True + assert models.state_db_has_session("nope") is False + _assert_read_only(calls) + + +def test_agent_session_rows_existing_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + _point_models_at(monkeypatch, db) + calls = _record_connects(monkeypatch) + + result = models.agent_session_rows_existing(["sess-1", "ghost"]) + assert result == frozenset({"sess-1"}) + _assert_read_only(calls) + + +def test_agent_session_zero_message_sids_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + _point_models_at(monkeypatch, db) + calls = _record_connects(monkeypatch) + + # sess-1 has 2 messages → not zero-message. + result = models.agent_session_zero_message_sids(["sess-1"]) + assert "sess-1" not in result + _assert_read_only(calls) + + +def test_sidebar_overrides_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + calls = _record_connects(monkeypatch) + + overrides = models._read_state_db_sidebar_overrides(db, {"sess-1"}) + assert isinstance(overrides, dict) + _assert_read_only(calls) + + +def test_get_state_db_session_messages_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + _point_models_at(monkeypatch, db) + calls = _record_connects(monkeypatch) + + msgs = models.get_state_db_session_messages("sess-1") + assert [m.get("role") for m in msgs] == ["user", "assistant"] + _assert_read_only(calls) + + +def test_get_state_db_message_keys_before_timestamp_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + _point_models_at(monkeypatch, db) + calls = _record_connects(monkeypatch) + + keys = models.get_state_db_session_message_keys_before_timestamp("sess-1", 1000.5) + assert keys is not None + _assert_read_only(calls) + + +def test_get_state_db_session_summary_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + _point_models_at(monkeypatch, db) + calls = _record_connects(monkeypatch) + + summary = models.get_state_db_session_summary("sess-1") + assert summary["message_count"] == 2 + _assert_read_only(calls) + + +def test_count_conversation_rounds_opens_read_only(tmp_path, monkeypatch): + db = tmp_path / "state.db" + _make_state_db(db) + # count_conversation_rounds resolves its DB via get_active_hermes_home() + # (falling back to $HERMES_HOME only on error), so patch that. + import api.profiles as profiles + + monkeypatch.setattr(profiles, "get_active_hermes_home", lambda: str(tmp_path)) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + calls = _record_connects(monkeypatch) + + rounds = models.count_conversation_rounds("sess-1") + assert rounds == 1 # one user + one assistant = one round + _assert_read_only(calls) + + +def test_read_only_handle_rejects_writes(tmp_path): + """Why the delete path must stay writable: the read-only handle these reads + now use rejects any write.""" + db = tmp_path / "state.db" + _make_state_db(db) + with closing(open_state_db_readonly(db)) as conn: + try: + conn.execute("DELETE FROM sessions WHERE id = 'sess-1'") + raise AssertionError("read-only handle unexpectedly allowed a write") + except sqlite3.OperationalError: + pass + + +def test_delete_cli_session_still_opens_writable_connection(): + """The lone write path (a DELETE) must keep a writable connection — a + read-only downgrade would make the delete silently no-op.""" + import inspect + + src = inspect.getsource(models.delete_cli_session) + assert "sqlite3.connect(str(db_path))" in src, ( + "delete_cli_session must keep its writable connection" + ) + assert "open_state_db_readonly" not in src, ( + "delete_cli_session must not be downgraded to the read-only helper" + )