mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-26 17:38:36 +00:00
fix(sessions): keep the skill body out of session previews
`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.
This commit is contained in:
parent
dcf5fc0eea
commit
c3d199c248
2 changed files with 273 additions and 31 deletions
142
hermes_state.py
142
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
|
||||
|
||||
|
|
|
|||
162
tests/test_session_skill_previews.py
Normal file
162
tests/test_session_skill_previews.py
Normal file
|
|
@ -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") == ""
|
||||
Loading…
Add table
Add a link
Reference in a new issue