Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 52 additions & 3 deletions agent/title_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,53 @@ def generate_title(
return None


def _persist_session_title(session_db, session_id, title):
"""Persist a generated title, recovering from duplicate-title collisions.

The write goes through ``set_auto_title_if_empty`` (predicate + write in
one transaction) so a manual ``/title`` set while LLM generation was in
flight is never overwritten — a plain ``set_session_title`` fallback keeps
older stores working. ``set_session_title`` raises ValueError when the
title would collide with another session (the unique-title index). Rather
than swallow it and leave the session untitled (#50537), append a #N
suffix via get_next_title_in_lineage() when the store supports lineage
dedup; otherwise re-raise so the caller can decide.

Returns the title actually persisted, or None when a concurrent manual
title won the race (nothing was written).
"""
atomic_fn = getattr(session_db, "set_auto_title_if_empty", None)

def _set(t):
if atomic_fn is not None:
if not atomic_fn(session_id, t):
# Predicate failed: a title appeared while generation was in
# flight (manual /title wins), or the session vanished.
logger.debug(
"Skipping auto-generated session title because a title "
"was set while generation was in flight"
)
return None
return t
ok = session_db.set_session_title(session_id, t)
if ok is False:
raise RuntimeError(
f"session {session_id} not found when storing title"
)
return t

try:
return _set(title)
except ValueError:
next_title_fn = getattr(session_db, "get_next_title_in_lineage", None)
if next_title_fn is None:
raise
deduped = next_title_fn(title)
if not deduped or deduped == title:
raise
return _set(deduped)


def auto_title_session(
session_db,
session_id: str,
Expand Down Expand Up @@ -237,11 +284,13 @@ def _auto_title_session(
return

try:
session_db.set_session_title(session_id, title)
logger.debug("Auto-generated session title: %s", title)
persisted = _persist_session_title(session_db, session_id, title)
if persisted is None:
return
logger.debug("Auto-generated session title: %s", persisted)
if title_callback is not None:
try:
title_callback(title)
title_callback(persisted)
except Exception:
logger.debug("Auto-title callback failed", exc_info=True)
except Exception as e:
Expand Down
76 changes: 68 additions & 8 deletions cron/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,45 @@
logger = logging.getLogger(__name__)


def _set_cron_session_title(session_db, session_id, base_title):
"""Robustly title a finished cron session before it is closed.

Centralizes the title write so the cron finally block can guarantee a
non-blank, unique title is persisted before end_session()/close() tear
the connection down (issues #50535, #50536, #50537):

- #50535: never leaves the session blank. base_title already carries a
cron-id fallback for nameless jobs; this also guards a failed write.
- #50537: a duplicate title makes set_session_title raise ValueError (the
unique-title index). Recover by appending a #N suffix via
get_next_title_in_lineage() when supported, instead of swallowing the
error and ending up untitled. If lineage dedup is unavailable, raise.
- #50536: this runs synchronously in the cron finally block ahead of the
session close, so no in-flight title write can race the close.

Returns the title actually persisted, or None if nothing could be set.
"""
if not session_db or not session_id:
return None
title = (base_title or "").strip()
if not title:
return None
try:
session_db.set_session_title(session_id, title)
return title
except ValueError:
# Title collision against the unique-title index. Fall back to the
# next title in the lineage (base #2, base #3, ...) when supported.
next_title_fn = getattr(session_db, "get_next_title_in_lineage", None)
if next_title_fn is None:
raise
deduped = next_title_fn(title)
if not deduped or deduped == title:
raise
session_db.set_session_title(session_id, deduped)
return deduped


def _summarize_cron_failure_for_delivery(job: dict, error: str | None) -> str:
"""Return a compact one-line failure message for chat delivery.

Expand Down Expand Up @@ -3500,18 +3539,39 @@ def _heartbeat_run_claim_if_due():
for _var_name in _cron_delivery_vars:
_VAR_MAP[_var_name].set("")
if _session_db:
# Title the cron session from the job (name → short prompt → id) so
# sidebars/history show a meaningful label instead of the injected
# "[IMPORTANT: …]" hint that is the session's first message. Set here
# (not at create time) so the agent's own INSERT keeps model /
# system_prompt; this only UPDATEs the title column. The run-time
# suffix keeps it unique against the sessions.title index across runs.
# Title the cron session from the job (name -> id) and PERSIST it
# BEFORE end_session()/close() tear the connection down, so the
# close can never run over an in-flight title write (#50536). The
# run-time suffix keeps it unique against the sessions.title index
# across runs; _set_cron_session_title dedupes (#50537) and the
# except-fallback below guarantees a non-blank title (#50535).
try:
_title_base = " ".join(job_name.split())[:60].strip() or f"cron {job_id}"
_cron_title = f"{_title_base} · {_hermes_now().strftime('%b %d %H:%M')}"
_session_db.set_session_title(_cron_session_id, _cron_title)
if not _set_cron_session_title(_session_db, _cron_session_id, _cron_title):
# Helper returned None (blank base) -> use the id fallback.
_set_cron_session_title(
_session_db, _cron_session_id, f"cron {job_id}"
)
except (Exception, KeyboardInterrupt) as e:
logger.debug("Job '%s': failed to set cron session title: %s", job_id, e)
logger.debug(
"Job '%s': failed to set cron session title: %s", job_id, e
)
# Last-resort: never leave the session blank (#50535). Try the
# next free title in the lineage, then a bare id-stamped title.
for _fallback in (
getattr(_session_db, "get_next_title_in_lineage", lambda b: b)(
f"cron {job_id}"
),
f"cron {job_id} {_cron_session_id[-6:]}",
):
try:
if _set_cron_session_title(
_session_db, _cron_session_id, _fallback
):
break
except (Exception, KeyboardInterrupt):
continue
try:
_session_db.end_session(_cron_session_id, "cron_complete")
except (Exception, KeyboardInterrupt) as e:
Expand Down
47 changes: 38 additions & 9 deletions hermes_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -3222,16 +3222,24 @@ def _is_compression_ancestor(
).fetchone()
return row is not None

def set_session_title(self, session_id: str, title: str) -> bool:
"""Set or update a session's title.

Returns True if session was found and title was set.
Raises ValueError if title is already in use by another session,
or if the title fails validation (too long, invalid characters).
Empty/whitespace-only strings are normalized to None (clearing the title).
"""
def _set_session_title(
self,
session_id: str,
title: str,
*,
only_if_empty: bool,
) -> bool:
title = self.sanitize_title(title)

def _do(conn):
if only_if_empty:
current = conn.execute(
"SELECT title FROM sessions WHERE id = ?",
(session_id,),
).fetchone()
if current is None or current["title"] is not None:
return 0

if title:
# Check uniqueness (allow the same session to keep its own title)
cursor = conn.execute(
Expand Down Expand Up @@ -3263,14 +3271,35 @@ def _do(conn):
raise ValueError(
f"Title '{title}' is already in use by session {conflict_id}"
)
predicate = " AND title IS NULL" if only_if_empty else ""
cursor = conn.execute(
"UPDATE sessions SET title = ? WHERE id = ?",
f"UPDATE sessions SET title = ? WHERE id = ?{predicate}",
(title, session_id),
)
return cursor.rowcount

rowcount = self._execute_write(_do)
return rowcount > 0

def set_session_title(self, session_id: str, title: str) -> bool:
"""Set or update a session's title.

Returns True if session was found and title was set.
Raises ValueError if title is already in use by another session,
or if the title fails validation (too long, invalid characters).
Empty/whitespace-only strings are normalized to None (clearing the title).
"""
return self._set_session_title(session_id, title, only_if_empty=False)

def set_auto_title_if_empty(self, session_id: str, title: str) -> bool:
"""Set an auto-generated title only when the current title is NULL.

The predicate and write run in one transaction so a concurrent manual
rename cannot be overwritten. Validation and uniqueness behavior match
:meth:`set_session_title`.
"""
return self._set_session_title(session_id, title, only_if_empty=True)

def get_session_title(self, session_id: str) -> Optional[str]:
"""Get the title for a session, or None."""
with self._lock:
Expand Down
1 change: 1 addition & 0 deletions scripts/release.py
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@
"157689911+itsflownium@users.noreply.github.com": "itsflownium",
"dirtyren@users.noreply.github.com": "dirtyren",
"s96919@gmail.com": "s96919",
"rasitakyol@hotmail.com": "rasitakyol",
"yakimenkoleksander228@gmail.com": "doxe0x",
"a54983334@163.com": "Code-suphub",
"78542984+Code-suphub@users.noreply.github.com": "Code-suphub",
Expand Down
111 changes: 108 additions & 3 deletions tests/agent/test_title_generator.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests for agent.title_generator — auto-generated session titles."""

import pytest
from unittest.mock import MagicMock, patch


Expand All @@ -9,6 +10,7 @@
maybe_auto_title,
_title_language,
)
from hermes_state import SessionDB


class TestGenerateTitle:
Expand Down Expand Up @@ -240,14 +242,42 @@ def test_skips_if_title_exists(self):
def test_generates_and_sets_title(self):
db = MagicMock()
db.get_session_title.return_value = None
db.set_auto_title_if_empty.return_value = True

with patch("agent.title_generator.generate_title", return_value="New Title"):
auto_title_session(db, "sess-1", "hi", "hello")
db.set_session_title.assert_called_once_with("sess-1", "New Title")
db.set_auto_title_if_empty.assert_called_once_with("sess-1", "New Title")

def test_does_not_overwrite_title_set_immediately_before_conditional_write(
self, tmp_path
):
db = SessionDB(tmp_path / "state.db")
db.create_session(session_id="sess-1", source="cli")
seen = []

def generate_after_manual_title(*_args, **_kwargs):
db.set_session_title("sess-1", "Manual Title")
return "Auto Title"

with patch(
"agent.title_generator.generate_title",
side_effect=generate_after_manual_title,
):
auto_title_session(
db,
"sess-1",
"hi",
"hello",
title_callback=seen.append,
)

assert db.get_session_title("sess-1") == "Manual Title"
assert seen == []

def test_invokes_title_callback_after_setting_title(self):
db = MagicMock()
db.get_session_title.return_value = None
db.set_auto_title_if_empty.return_value = True
seen = []
with patch("agent.title_generator.generate_title", return_value="Readable Session"):
auto_title_session(
Expand All @@ -257,7 +287,7 @@ def test_invokes_title_callback_after_setting_title(self):
"hi there",
title_callback=seen.append,
)
db.set_session_title.assert_called_once_with("sess-1", "Readable Session")
db.set_auto_title_if_empty.assert_called_once_with("sess-1", "Readable Session")
assert seen == ["Readable Session"]

def test_skips_if_generation_fails(self):
Expand All @@ -266,7 +296,7 @@ def test_skips_if_generation_fails(self):

with patch("agent.title_generator.generate_title", return_value=None):
auto_title_session(db, "sess-1", "hi", "hello")
db.set_session_title.assert_not_called()
db.set_auto_title_if_empty.assert_not_called()

def test_never_raises_when_body_throws(self):
"""Daemon-thread target must swallow ALL exceptions (e.g. the
Expand Down Expand Up @@ -408,3 +438,78 @@ def test_skips_if_no_response(self):

def test_skips_if_no_session_db(self):
maybe_auto_title(None, "sess-1", "hello", "response", []) # no db


class TestAutoTitleDuplicateHandling:
"""Duplicate auto-title handling and not-found hardening (#50537)."""

def test_dedupes_duplicate_title_via_lineage(self):
db = MagicMock()
db.get_session_title.return_value = None
# Atomic write path: collision raises ValueError, retry persists.
db.set_auto_title_if_empty.side_effect = [ValueError("in use"), True]
db.get_next_title_in_lineage.return_value = "Debugging Import Error #2"
with patch(
"agent.title_generator.generate_title",
return_value="Debugging Import Error",
):
seen = []
auto_title_session(db, "sess-1", "hi", "hello", title_callback=seen.append)
db.get_next_title_in_lineage.assert_called_once_with("Debugging Import Error")
assert db.set_auto_title_if_empty.call_args_list[-1][0] == (
"sess-1",
"Debugging Import Error #2",
)
# callback fires with the actually-persisted (deduped) title
assert seen == ["Debugging Import Error #2"]

def test_dedupes_duplicate_title_via_lineage_legacy_store(self):
# Store without set_auto_title_if_empty: same dedup via the plain
# set_session_title fallback.
db = MagicMock(
spec=["get_session_title", "set_session_title", "get_next_title_in_lineage"]
)
db.get_session_title.return_value = None
db.set_session_title.side_effect = [ValueError("in use"), True]
db.get_next_title_in_lineage.return_value = "Debugging Import Error #2"
with patch(
"agent.title_generator.generate_title",
return_value="Debugging Import Error",
):
seen = []
auto_title_session(db, "sess-1", "hi", "hello", title_callback=seen.append)
assert db.set_session_title.call_args_list[-1][0] == (
"sess-1",
"Debugging Import Error #2",
)
assert seen == ["Debugging Import Error #2"]

def test_swallows_value_error_without_lineage_support(self):
# No get_next_title_in_lineage -> ValueError propagates out of the
# persist helper but auto_title_session still swallows it (no crash).
db = MagicMock(spec=["get_session_title", "set_session_title"])
db.get_session_title.return_value = None
db.set_session_title.side_effect = ValueError("in use")
with patch(
"agent.title_generator.generate_title", return_value="Dup Title"
):
auto_title_session(db, "sess-1", "hi", "hello") # must not raise

def test_manual_title_race_skips_without_callback(self):
# Atomic predicate fails (manual /title landed while generation was in
# flight) -> nothing persisted, no callback fired.
from agent.title_generator import _persist_session_title
db = MagicMock()
db.set_auto_title_if_empty.return_value = False
assert _persist_session_title(db, "sess-1", "Some Title") is None
db.set_session_title.assert_not_called()

def test_not_found_raises_runtime_error_internally(self):
# Legacy store (no atomic write): set_session_title returning False
# (session vanished) -> RuntimeError in the persist helper, swallowed
# by auto_title_session, no callback.
from agent.title_generator import _persist_session_title
db = MagicMock(spec=["get_session_title", "set_session_title"])
db.set_session_title.return_value = False
with pytest.raises(RuntimeError):
_persist_session_title(db, "missing", "Some Title")
Loading
Loading