From c3d199c248cb39ff54c23213f6843db1dcc9f0ed Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sun, 26 Jul 2026 02:58:20 -0500 Subject: [PATCH] fix(sessions): keep the skill body out of session previews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `preview` is the head of the first user message and the title fallback on every surface — sidebar rows, pickers, exports, the desktop's sessionTitle(). An untitled /skill session therefore read `[IMPORTANT: The user has invoked the "work" skill, indicatin...` wherever it appeared. A scaffolded row now selects a wide enough excerpt to reach the typed instruction (head + tail spliced for a long body) and shapes it through describe_skill_invocation(). Because previews are computed on read, existing sessions are corrected without a migration. The six copies of the preview subquery and four copies of its shaping collapse into one expression and one helper along the way, and the /rewind picker gets the same treatment. --- hermes_state.py | 142 ++++++++++++++++++----- tests/test_session_skill_previews.py | 162 +++++++++++++++++++++++++++ 2 files changed, 273 insertions(+), 31 deletions(-) create mode 100644 tests/test_session_skill_previews.py diff --git a/hermes_state.py b/hermes_state.py index 0fc1364c2ad8..32fcffac585f 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -29,6 +29,11 @@ from pathlib import Path from agent.memory_manager import sanitize_context from agent.message_sanitization import _sanitize_surrogates +from agent.skill_commands import ( + SKILL_EXCERPT_JOINT, + SKILL_SCAFFOLD_SQL_LIKE, + describe_skill_invocation, +) from hermes_constants import get_hermes_home from hermes_cli.sqlite_runtime import ( is_sqlite_wal_reset_vulnerable as _is_sqlite_wal_reset_vulnerable, @@ -130,6 +135,51 @@ def _cwd_prefix_clause(cwd_prefix: str) -> Tuple[str, List[str]]: return "(s.cwd = ? OR s.cwd LIKE ? OR s.cwd LIKE ?)", [prefix, f"{prefix}/%", f"{prefix}\\%"] +# Session preview = the head of the first user message, shown wherever a +# session has no title (sidebar rows, pickers, exports, the desktop's +# `sessionTitle` fallback). +# +# A /skill invocation expands into a message that embeds the whole skill body, +# so the plain head of it previews the SKILL's opening prose as if the user had +# written it. Scaffolded rows therefore carry a wider excerpt so +# ``_shape_preview`` can hand it to ``describe_skill_invocation`` and recover +# ``/work — fix the title leak``: the whole message while it stays under the +# budget, and head + tail (where the typed instruction lands) once it doesn't. +_PREVIEW_HEAD_CHARS = 63 +_PREVIEW_SCAFFOLD_WINDOW = 400 +_PREVIEW_MAX_CHARS = 60 + +_PREVIEW_CONTENT_SQL = "REPLACE(REPLACE(m.content, X'0A', ' '), X'0D', ' ')" +_PREVIEW_SCAFFOLDED_SQL = f"m.content LIKE '{SKILL_SCAFFOLD_SQL_LIKE}'" + +# The shared ``_preview_raw`` SELECT expression, interpolated by every listing +# query. A scaffolded row gets a wider excerpt: the whole message while it fits +# the budget, else head + tail (where the typed instruction lands) spliced +# around SKILL_EXCERPT_JOINT. +_PREVIEW_RAW_SELECT = ( + f"CASE WHEN {_PREVIEW_SCAFFOLDED_SQL}" + f" AND LENGTH(m.content) > {_PREVIEW_SCAFFOLD_WINDOW * 2}" + f" THEN SUBSTR({_PREVIEW_CONTENT_SQL}, 1, {_PREVIEW_SCAFFOLD_WINDOW})" + f" || '{SKILL_EXCERPT_JOINT}'" + f" || SUBSTR({_PREVIEW_CONTENT_SQL}, -{_PREVIEW_SCAFFOLD_WINDOW})" + f" WHEN {_PREVIEW_SCAFFOLDED_SQL}" + f" THEN SUBSTR({_PREVIEW_CONTENT_SQL}, 1, {_PREVIEW_SCAFFOLD_WINDOW * 2})" + f" ELSE SUBSTR({_PREVIEW_CONTENT_SQL}, 1, {_PREVIEW_HEAD_CHARS}) END" +) + + +def _shape_preview(raw: Any) -> str: + """Turn a ``_preview_raw`` column into the short preview callers show.""" + text = str(raw or "").strip() + if not text: + return "" + described = describe_skill_invocation(text) + text = described if described is not None else text.split(SKILL_EXCERPT_JOINT)[0] + if len(text) > _PREVIEW_MAX_CHARS: + return text[:_PREVIEW_MAX_CHARS] + "..." + return text + + # A child session counts as a /branch (kept visible, never cascade-deleted) if # it carries the stable marker OR the legacy end_reason heuristic holds. _BRANCH_CHILD_SQL = ( @@ -6005,7 +6055,7 @@ class SessionDB: ) SELECT {_sel}, COALESCE( - (SELECT SUBSTR(REPLACE(REPLACE(m.content, X'0A', ' '), X'0D', ' '), 1, 63) + (SELECT {_PREVIEW_RAW_SELECT} FROM messages m WHERE m.session_id = s.id AND m.role = 'user' AND m.content IS NOT NULL ORDER BY m.timestamp, m.id LIMIT 1), @@ -6030,7 +6080,7 @@ class SessionDB: query = f""" SELECT {_sel}, COALESCE( - (SELECT SUBSTR(REPLACE(REPLACE(m.content, X'0A', ' '), X'0D', ' '), 1, 63) + (SELECT {_PREVIEW_RAW_SELECT} FROM messages m WHERE m.session_id = s.id AND m.role = 'user' AND m.content IS NOT NULL ORDER BY m.timestamp, m.id LIMIT 1), @@ -6052,13 +6102,7 @@ class SessionDB: sessions = [] for row in rows: s = dict(row) - # Build the preview from the raw substring - raw = s.pop("_preview_raw", "").strip() - if raw: - text = raw[:60] - s["preview"] = text + ("..." if len(raw) > 60 else "") - else: - s["preview"] = "" + s["preview"] = _shape_preview(s.pop("_preview_raw", "")) # Drop the internal ordering column so callers see a clean dict. s.pop("_effective_last_active", None) sessions.append(s) @@ -6131,10 +6175,10 @@ class SessionDB: # compute it generically rather than hardcoding the successor char. prefix_hi = prefix[:-1] + chr(ord(prefix[-1]) + 1) - query = """ + query = f""" SELECT s.*, COALESCE( - (SELECT SUBSTR(REPLACE(REPLACE(m.content, X'0A', ' '), X'0D', ' '), 1, 63) + (SELECT {_PREVIEW_RAW_SELECT} FROM messages m WHERE m.session_id = s.id AND m.role = 'user' AND m.content IS NOT NULL ORDER BY m.timestamp, m.id LIMIT 1), @@ -6156,12 +6200,7 @@ class SessionDB: runs: List[Dict[str, Any]] = [] for row in rows: s = dict(row) - raw = s.pop("_preview_raw", "").strip() - if raw: - text = raw[:60] - s["preview"] = text + ("..." if len(raw) > 60 else "") - else: - s["preview"] = "" + s["preview"] = _shape_preview(s.pop("_preview_raw", "")) runs.append(s) return runs @@ -6177,7 +6216,7 @@ class SessionDB: query = f""" SELECT {_sel}, COALESCE( - (SELECT SUBSTR(REPLACE(REPLACE(m.content, X'0A', ' '), X'0D', ' '), 1, 63) + (SELECT {_PREVIEW_RAW_SELECT} FROM messages m WHERE m.session_id = s.id AND m.role = 'user' AND m.content IS NOT NULL ORDER BY m.timestamp, m.id LIMIT 1), @@ -6196,14 +6235,54 @@ class SessionDB: if not row: return None s = dict(row) - raw = s.pop("_preview_raw", "").strip() - if raw: - text = raw[:60] - s["preview"] = text + ("..." if len(raw) > 60 else "") - else: - s["preview"] = "" + s["preview"] = _shape_preview(s.pop("_preview_raw", "")) return s + def list_skill_scaffolded_sessions(self, limit: int = 200) -> List[Dict[str, Any]]: + """Titled sessions whose first user turn was a ``/skill`` invocation. + + Those titles were generated from the expanded message, which embeds the + whole skill body — so they describe the skill rather than the request. + Returns ``id``, ``title``, and the full first-turn ``content`` so a + caller can re-derive what the user typed. Newest first. + """ + with self._lock: + rows = self._conn.execute( + """ + SELECT s.id, s.title, m.content + FROM sessions s + JOIN messages m ON m.id = ( + SELECT m2.id FROM messages m2 + WHERE m2.session_id = s.id AND m2.role = 'user' + AND m2.content IS NOT NULL + ORDER BY m2.timestamp, m2.id LIMIT 1 + ) + WHERE s.title IS NOT NULL AND m.content LIKE ? + ORDER BY s.started_at DESC + LIMIT ? + """, + (SKILL_SCAFFOLD_SQL_LIKE, int(limit)), + ).fetchall() + return [dict(row) for row in rows] + + def get_first_assistant_text(self, session_id: str) -> str: + """The session's first assistant reply as plain text ('' when none). + + Pairs with :meth:`list_skill_scaffolded_sessions` so a re-title can feed + the titler the same (request, reply) shape the live path uses. + """ + with self._lock: + row = self._conn.execute( + "SELECT content FROM messages " + "WHERE session_id = ? AND role = 'assistant' AND content IS NOT NULL " + "ORDER BY timestamp, id LIMIT 1", + (session_id,), + ).fetchone() + if not row: + return "" + decoded = self._decode_content(row["content"]) + return decoded if isinstance(decoded, str) else "" + # ========================================================================= # Message storage # ========================================================================= @@ -7582,7 +7661,9 @@ class SessionDB: if not preview: preview = "[multimodal content]" elif isinstance(decoded, str): - preview = decoded + # A /skill turn embeds the whole skill body; show what the user + # typed instead of the skill's opening prose. + preview = describe_skill_invocation(decoded) or decoded else: preview = "" preview = " ".join(preview.split()) # collapse whitespace @@ -10311,10 +10392,10 @@ class SessionDB: with self._lock: try: rows = self._conn.execute( - """ + f""" SELECT s.*, COALESCE( - (SELECT SUBSTR(REPLACE(REPLACE(m.content, X'0A', ' '), X'0D', ' '), 1, 63) + (SELECT {_PREVIEW_RAW_SELECT} FROM messages m WHERE m.session_id = s.id AND m.role = 'user' AND m.content IS NOT NULL ORDER BY m.timestamp, m.id LIMIT 1), @@ -10340,10 +10421,10 @@ class SessionDB: # telegram_dm_topic_bindings doesn't exist yet — no bindings # means every telegram session for this user is "unlinked". rows = self._conn.execute( - """ + f""" SELECT s.*, COALESCE( - (SELECT SUBSTR(REPLACE(REPLACE(m.content, X'0A', ' '), X'0D', ' '), 1, 63) + (SELECT {_PREVIEW_RAW_SELECT} FROM messages m WHERE m.session_id = s.id AND m.role = 'user' AND m.content IS NOT NULL ORDER BY m.timestamp, m.id LIMIT 1), @@ -10365,8 +10446,7 @@ class SessionDB: sessions: List[Dict[str, Any]] = [] for row in rows: session = dict(row) - raw = str(session.pop("_preview_raw", "") or "").strip() - session["preview"] = raw[:60] + ("..." if len(raw) > 60 else "") if raw else "" + session["preview"] = _shape_preview(session.pop("_preview_raw", "")) sessions.append(session) return sessions diff --git a/tests/test_session_skill_previews.py b/tests/test_session_skill_previews.py new file mode 100644 index 000000000000..04f393295628 --- /dev/null +++ b/tests/test_session_skill_previews.py @@ -0,0 +1,162 @@ +"""Session previews must never surface a /skill's own body. + +`preview` is the head of the first user message, and it is the TITLE FALLBACK on +every surface (sidebar rows, pickers, exports, desktop `sessionTitle`). A /skill +invocation expands into a message that embeds the whole skill body, so an +untitled skill session used to read `[IMPORTANT: The user has invoked the "work" +skill, indicatin...` in the sidebar. + +These drive the real SQL/shaping path through SessionDB rather than calling the +shaper directly, so the CASE expression and the Python side are covered together. +""" + +import pytest + +import agent.skill_commands as skill_commands +import tools.skills_tool as skills_tool +from hermes_state import SessionDB + +SKILL_BODY = ( + "Kick off a task in a fresh isolated git worktree instead of the current checkout. " + "Look at what the repo already does, and copy it. Create the worktree and branch. " +) + + +@pytest.fixture() +def db(tmp_path): + session_db = SessionDB(db_path=tmp_path / "state.db") + yield session_db + session_db.close() + + +def _install_skill(tmp_path, monkeypatch, name="work", body=SKILL_BODY): + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Description for {name}\n---\n\n# {name}\n\n{body}\n" + ) + monkeypatch.setattr(skills_tool, "SKILLS_DIR", skills_dir) + monkeypatch.setattr(skill_commands, "_skill_commands", {}) + monkeypatch.setattr(skill_commands, "_skill_commands_platform", None) + skill_commands.scan_skill_commands() + return skills_dir + + +def _seed(db, session_id, content, *, title=None, reply="On it."): + db.create_session(session_id=session_id, source="cli", model="m") + db.append_message(session_id, role="user", content=content) + db.append_message(session_id, role="assistant", content=reply) + if title: + db.set_session_title(session_id, title) + + +class TestSkillPreview: + def test_plain_message_preview_is_unchanged(self, db): + _seed(db, "s1", "fix the title leak") + (row,) = db.list_sessions_rich(limit=10) + assert row["preview"] == "fix the title leak" + + def test_long_plain_message_still_truncates(self, db): + _seed(db, "s1", "x" * 200) + (row,) = db.list_sessions_rich(limit=10) + assert row["preview"] == "x" * 60 + "..." + + def test_skill_preview_shows_the_typed_instruction(self, db, tmp_path, monkeypatch): + _install_skill(tmp_path, monkeypatch) + message = skill_commands.build_skill_invocation_message( + "/work", user_instruction="fix the title leak" + ) + _seed(db, "s1", message) + (row,) = db.list_sessions_rich(limit=10) + assert row["preview"] == "/work — fix the title leak" + + def test_skill_preview_never_leaks_the_body(self, db, tmp_path, monkeypatch): + _install_skill(tmp_path, monkeypatch) + message = skill_commands.build_skill_invocation_message( + "/work", user_instruction="fix the title leak" + ) + _seed(db, "s1", message) + (row,) = db.list_sessions_rich(limit=10) + assert "IMPORTANT" not in row["preview"] + assert "worktree" not in row["preview"] + + def test_bare_skill_preview_is_the_command(self, db, tmp_path, monkeypatch): + _install_skill(tmp_path, monkeypatch) + message = skill_commands.build_skill_invocation_message("/work") + _seed(db, "s1", message) + (row,) = db.list_sessions_rich(limit=10) + assert row["preview"] == "/work" + + def test_huge_skill_body_still_recovers_the_instruction( + self, db, tmp_path, monkeypatch + ): + # Long enough that the head window lands mid-body and the SQL has to + # splice the tail to reach the instruction. + _install_skill(tmp_path, monkeypatch, body="filler line.\n" * 300) + message = skill_commands.build_skill_invocation_message( + "/work", user_instruction="fix the title leak" + ) + _seed(db, "s1", message) + (row,) = db.list_sessions_rich(limit=10) + assert row["preview"] == "/work — fix the title leak" + assert "filler line" not in row["preview"] + + def test_single_row_lookup_agrees_with_the_list(self, db, tmp_path, monkeypatch): + """_get_session_rich_row shares the shaper — a compression tip must + surface the same preview the list does.""" + _install_skill(tmp_path, monkeypatch) + message = skill_commands.build_skill_invocation_message( + "/work", user_instruction="fix the title leak" + ) + _seed(db, "s1", message) + (listed,) = db.list_sessions_rich(limit=10) + single = db._get_session_rich_row("s1") + assert single["preview"] == listed["preview"] == "/work — fix the title leak" + + def test_rewind_picker_shows_the_typed_instruction( + self, db, tmp_path, monkeypatch + ): + _install_skill(tmp_path, monkeypatch) + message = skill_commands.build_skill_invocation_message( + "/work", user_instruction="fix the title leak" + ) + _seed(db, "s1", message) + (entry,) = db.list_recent_user_messages("s1") + assert entry["preview"] == "/work — fix the title leak" + + +class TestSkillScaffoldedSessionLookup: + """Backing queries for `hermes sessions retitle-skills`.""" + + def test_finds_only_titled_skill_sessions(self, db, tmp_path, monkeypatch): + _install_skill(tmp_path, monkeypatch) + message = skill_commands.build_skill_invocation_message( + "/work", user_instruction="fix the title leak" + ) + _seed(db, "titled", message, title="Isolated Git Worktree Setup") + _seed(db, "untitled", message) + _seed(db, "plain", "fix the title leak", title="Fixing The Title Leak") + + rows = db.list_skill_scaffolded_sessions() + assert [row["id"] for row in rows] == ["titled"] + assert rows[0]["title"] == "Isolated Git Worktree Setup" + # The full first turn comes back so the caller can re-derive the ask. + assert "fix the title leak" in rows[0]["content"] + + def test_limit_is_honored(self, db, tmp_path, monkeypatch): + _install_skill(tmp_path, monkeypatch) + message = skill_commands.build_skill_invocation_message("/work") + for i in range(3): + _seed(db, f"s{i}", message, title=f"Title {i}") + assert len(db.list_skill_scaffolded_sessions(limit=2)) == 2 + + def test_first_assistant_text(self, db): + _seed(db, "s1", "hello", reply="first reply") + db.append_message("s1", role="assistant", content="second reply") + assert db.get_first_assistant_text("s1") == "first reply" + + def test_first_assistant_text_missing_is_empty(self, db): + db.create_session(session_id="s1", source="cli", model="m") + db.append_message("s1", role="user", content="hello") + assert db.get_first_assistant_text("s1") == ""