diff --git a/agent/title_generator.py b/agent/title_generator.py index a76deb4201..40db0e8244 100644 --- a/agent/title_generator.py +++ b/agent/title_generator.py @@ -81,10 +81,12 @@ _EXAMPLE_ECHO_REJECT = frozenset( t.lower() for t in _PROMPT_GOOD_EXAMPLES if t != "Friendly greeting" ) | {_PROMPT_VAGUE_EXAMPLE.lower()} -# A generic greeting is intentionally offered as a title for a conversation -# with no topic yet. Keep it at derived authority so the first substantive -# follow-up can replace it with an LLM title. -_PROVISIONAL_GREETING_TITLES = frozenset({"friendly greeting", "friendly greeting in chat"}) +# "Friendly greeting" is what the prompt asks for when the opener has no topic yet, so +# it is the one model title that must NOT settle the session: it is persisted at +# ``derived`` authority (a placeholder, like the instant title) and the next substantive +# turn upgrades it. Kept as a prompt example on purpose — a predictable placeholder is +# detectable, an improvised one ("Casual check-in chat") would lock the title as ``llm``. +_PROVISIONAL_GREETING_TITLE = "friendly greeting" _TITLE_PROMPT_TEMPLATE = ( @@ -320,6 +322,12 @@ def _notify_title(title_callback: Optional[TitleCallback], title: str, source: s _safe_callback(title_callback, (title, source), "%s callback failed", label) +def _is_provisional_greeting_title(title: str) -> bool: + """The prompt's greeting placeholder (also "Friendly greeting in chat" and quoted/bracketed variants).""" + normalized = re.sub(r"^[\W_]+|[\W_]+$", "", title.strip(), flags=re.UNICODE).lower() + return normalized.startswith(_PROVISIONAL_GREETING_TITLE) + + def _is_prompt_example_echo(title: str) -> bool: """Return True when *title* is one of the prompt's own example titles. @@ -393,7 +401,7 @@ def generate_title( # ignored the task and answered the user's message instead ("I don't have context on X — that's not # something I recognize..."). Truncating would store half an assistant blob as the session title, # which is still an assistant blob — reject instead so the caller retries on the next exchange - # (maybe_auto_title fires for the first two exchanges). Port of can1357/oh-my-pi#7306. + # (maybe_auto_title retries a placeholder title through the third exchange). Port of can1357/oh-my-pi#7306. if title is not None and len(title.split()) > _MAX_TITLE_WORDS: # Answer-shaped output: reject (not truncate) so the caller retries next exchange. logger.debug("Rejecting answer-shaped title output (%d words > %d)", len(title.split()), _MAX_TITLE_WORDS) @@ -429,18 +437,6 @@ def _has_upgraded_title(session_db, session_id: str) -> bool: return True -def _has_provisional_greeting_title(session_db, session_id: str) -> bool: - """Whether the stored title is the generic greeting placeholder.""" - try: - return ( - session_db.get_session_title_source(session_id) == "derived" - and str(session_db.get_session_title(session_id) or "").strip().lower() - in _PROVISIONAL_GREETING_TITLES - ) - except Exception: - return False - - def _persist_session_title(session_db, session_id, title, *, source, dedupe=True): """Persist at *source* authority via ``set_auto_title`` (precedence check + write in one transaction, so a manual ``/title`` is never overwritten); None when a higher authority held the row. @@ -521,7 +517,7 @@ def auto_title_session( title, source = generate_title( user_message, failure_callback=failure_callback, main_runtime=main_runtime, runtime_validator=runtime_validator, ), "llm" - if title and title.strip().lower() in _PROVISIONAL_GREETING_TITLES: + if title and _is_provisional_greeting_title(title): source = "derived" if not title: # the inline attempt declined collisions; off the critical path the lineage scan is affordable title, source = derive_title(user_message), "derived" @@ -596,11 +592,16 @@ def maybe_auto_title( """Instant inline title, then a daemon-thread upgrade. Call at the START of a turn, before the model.""" if not session_db or not session_id or not user_message: return - # History may be pre- or post-message. Skip only when BOTH past the opening turn AND named: count alone - # left a machinery-opened session nameless; title alone never titles on an old store. + # History may be pre- or post-message. Past the opening turn, skip once the session holds an + # ``llm``/``user`` name: count alone left a machinery-opened session nameless, and a ``derived`` + # name is still a placeholder (instant slice, or the model's greeting title for a bare "hi") that + # the first substantive turn should replace. Untitled sessions always get another shot; a + # placeholder gets turns 2-3, so a failing title model costs at most three calls, not one per turn. user_msg_count = sum(1 for m in (conversation_history or []) if _is_real_user_turn(m)) - if (user_msg_count > 1 and not _session_is_untitled(session_db, session_id) - and not _has_provisional_greeting_title(session_db, session_id)): + if user_msg_count > 1 and ( + _has_upgraded_title(session_db, session_id) + or (user_msg_count > 3 and not _session_is_untitled(session_db, session_id)) + ): return kanban_title = _kanban_task_title() if kanban_title: diff --git a/tests/agent/test_title_generator.py b/tests/agent/test_title_generator.py index eb2c03aee7..33105564e6 100644 --- a/tests/agent/test_title_generator.py +++ b/tests/agent/test_title_generator.py @@ -8,6 +8,7 @@ from agent.title_generator import ( generate_title, auto_title_session, maybe_auto_title, + wait_for_title_upgrades, _title_language, ) from hermes_state import SessionDB @@ -609,37 +610,43 @@ class TestMaybeAutoTitle: mock_auto.assert_not_called() def test_upgrades_a_provisional_greeting_on_a_substantive_second_turn(self, tmp_path): - """A canned greeting title must not prevent the next real request from naming the session.""" + """A bare "hi" opener leaves only placeholders (instant slice / the model's greeting title); + the next real request must still be allowed to name the session.""" db = SessionDB(tmp_path / "state.db") db.create_session(session_id="sess-1", source="cli") - greeting = MagicMock() - greeting.choices[0].message.content = "Friendly greeting" - with patch("agent.title_generator.call_llm", return_value=greeting): - auto_title_session(db, "sess-1", "hi how are you") + answers = iter(["Friendly greeting", "Debug scheduler failures"]) - assert db.get_session_title("sess-1") == "Friendly greeting" - assert db.get_session_title_source("sess-1") == "derived" + def stub_call_llm(**kwargs): + resp = MagicMock() + resp.choices[0].message.content = next(answers) + resp.choices[0].message.reasoning = None + return resp - history = [ - {"role": "user", "content": "hi how are you"}, - {"role": "assistant", "content": "I'm well, thanks."}, - ] - with patch("agent.title_generator.auto_title_session") as mock_auto: - import threading - called = threading.Event() - mock_auto.side_effect = lambda *a, **k: called.set() + history = [{"role": "user", "content": "hi how are you"}] + with patch("agent.title_generator.call_llm", side_effect=stub_call_llm), \ + patch("agent.title_generator._auto_title_enabled", return_value=True), \ + patch("agent.title_generator._model_title_upgrade_enabled", return_value=True): + maybe_auto_title(db, "sess-1", "hi how are you", history) + wait_for_title_upgrades(10) + assert db.get_session_title_source("sess-1") == "derived" + history += [{"role": "assistant", "content": "Well, thanks."}, + {"role": "user", "content": "help me debug the scheduler"}] maybe_auto_title(db, "sess-1", "help me debug the scheduler", history) - assert called.wait(timeout=10), "auto-title upgrade thread never ran" - mock_auto.assert_called_once() - - substantive = MagicMock() - substantive.choices[0].message.content = "Debug scheduler failures" - with patch("agent.title_generator.call_llm", return_value=substantive): - auto_title_session(db, "sess-1", "help me debug the scheduler") + wait_for_title_upgrades(10) assert db.get_session_title("sess-1") == "Debug scheduler failures" assert db.get_session_title_source("sess-1") == "llm" + def test_a_placeholder_title_stops_retrying_after_the_third_turn(self, tmp_path): + """A derived name gets turns 2-3 to upgrade, not a model call on every later turn.""" + db = SessionDB(tmp_path / "state.db") + db.create_session(session_id="sess-1", source="cli") + db.set_auto_title("sess-1", "hi", source="derived") + history = [{"role": "user", "content": f"turn {n}"} for n in range(4)] + with patch("agent.title_generator.auto_title_session") as mock_auto: + maybe_auto_title(db, "sess-1", "and now the real question", history) + mock_auto.assert_not_called() + def test_instant_title_declines_a_name_collision(self, tmp_path): """A colliding derived title is skipped, not scanned into 'hi #2'.