diff --git a/hermes_state.py b/hermes_state.py index 61c8d6a43e..e0b13d367f 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -2067,6 +2067,7 @@ class SessionDB: role_filter: List[str] = None, limit: int = 20, offset: int = 0, + sort: str = None, ) -> List[Dict[str, Any]]: """ Full-text search across session messages using FTS5. @@ -2079,6 +2080,19 @@ class SessionDB: Returns matching messages with session metadata, content snippet, and surrounding context (1 message before and after the match). + + ``sort`` controls temporal ordering of results: + - ``None`` (default): FTS5 BM25 relevance only. Time-neutral, but + ties between equally-relevant messages are broken arbitrarily. + - ``"newest"``: order by message timestamp DESC, then by rank. + Recent matches surface first; rank breaks same-timestamp ties. + - ``"oldest"``: order by message timestamp ASC, then by rank. + For "how did this start" / "what was the original X" questions. + + The LIKE fallback path (short CJK queries) ignores ``sort`` because + it has no rank to combine with — it already orders by timestamp DESC + unconditionally. The trigram CJK path honours ``sort`` like the main + FTS5 path. """ if not query or not query.strip(): return [] @@ -2087,6 +2101,26 @@ class SessionDB: if not query: return [] + # Normalise sort. Anything not in the allowed set falls back to None + # (FTS5 rank-only) — be forgiving to callers who pass empty string or + # an unexpected value rather than failing the search. + if isinstance(sort, str): + sort_norm = sort.strip().lower() + if sort_norm not in ("newest", "oldest"): + sort_norm = None + else: + sort_norm = None + + # Build the ORDER BY clause shared by both FTS5 paths (main + trigram). + # When sort is set, timestamp is primary and rank is the tiebreaker; + # otherwise rank alone (current behaviour). + if sort_norm == "newest": + order_by_sql = "ORDER BY m.timestamp DESC, rank" + elif sort_norm == "oldest": + order_by_sql = "ORDER BY m.timestamp ASC, rank" + else: + order_by_sql = "ORDER BY rank" + # Build WHERE clauses dynamically where_clauses = ["messages_fts MATCH ?"] params: list = [query] @@ -2125,7 +2159,7 @@ class SessionDB: JOIN messages m ON m.id = messages_fts.rowid JOIN sessions s ON s.id = m.session_id WHERE {where_sql} - ORDER BY rank + {order_by_sql} LIMIT ? OFFSET ? """ @@ -2194,7 +2228,7 @@ class SessionDB: JOIN messages m ON m.id = messages_fts_trigram.rowid JOIN sessions s ON s.id = m.session_id WHERE {' AND '.join(tri_where)} - ORDER BY rank + {order_by_sql} LIMIT ? OFFSET ? """ tri_params.extend([limit, offset]) @@ -2233,6 +2267,15 @@ class SessionDB: if role_filter: like_where.append(f"m.role IN ({','.join('?' for _ in role_filter)})") like_params.extend(role_filter) + # LIKE fallback has no FTS5 rank to combine with, so the + # ordering options collapse to just timestamp direction. + # Default and "newest" both produce DESC (preserving prior + # behaviour); "oldest" flips to ASC. + like_order_sql = ( + "ORDER BY m.timestamp ASC" + if sort_norm == "oldest" + else "ORDER BY m.timestamp DESC" + ) like_sql = f""" SELECT m.id, m.session_id, m.role, substr(m.content, @@ -2243,7 +2286,7 @@ class SessionDB: FROM messages m JOIN sessions s ON s.id = m.session_id WHERE {' AND '.join(like_where)} - ORDER BY m.timestamp DESC + {like_order_sql} LIMIT ? OFFSET ? """ like_params.extend([limit, offset]) diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index 3bae763b94..eb69c7853e 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -2494,6 +2494,103 @@ class TestExcludeSources: sources = [r["source"] for r in results] assert sources == ["cli"] + def test_search_messages_sort_newest_orders_by_timestamp_desc(self, db): + """``sort='newest'`` makes timestamp the primary sort key (DESC) with + FTS5 rank as the tiebreaker. With three matching messages at distinct + timestamps, results come out newest-first regardless of BM25 score.""" + db.create_session("old_sid", "cli") + db.create_session("mid_sid", "cli") + db.create_session("new_sid", "cli") + # Same content → identical BM25 score; only timestamps differ. + mid_old = db.append_message("old_sid", "user", "matchword discussion") + mid_mid = db.append_message("mid_sid", "user", "matchword discussion") + mid_new = db.append_message("new_sid", "user", "matchword discussion") + # Stamp explicit, well-separated timestamps after the fact. + with db._lock: + db._conn.execute("UPDATE messages SET timestamp=1000 WHERE id=?", (mid_old,)) + db._conn.execute("UPDATE messages SET timestamp=2000 WHERE id=?", (mid_mid,)) + db._conn.execute("UPDATE messages SET timestamp=3000 WHERE id=?", (mid_new,)) + db._conn.commit() + + results = db.search_messages("matchword", sort="newest") + session_order = [r["session_id"] for r in results] + assert session_order == ["new_sid", "mid_sid", "old_sid"], ( + f"sort=newest must return newest first; got {session_order}" + ) + + def test_search_messages_sort_oldest_orders_by_timestamp_asc(self, db): + """``sort='oldest'`` is symmetric — earliest matches first. Critical + for 'how did X start' questions where rank-only ordering would hide + the origin under more recent revisitations.""" + db.create_session("a", "cli") + db.create_session("b", "cli") + db.create_session("c", "cli") + m_a = db.append_message("a", "user", "matchword") + m_b = db.append_message("b", "user", "matchword") + m_c = db.append_message("c", "user", "matchword") + with db._lock: + db._conn.execute("UPDATE messages SET timestamp=3000 WHERE id=?", (m_a,)) + db._conn.execute("UPDATE messages SET timestamp=1000 WHERE id=?", (m_b,)) + db._conn.execute("UPDATE messages SET timestamp=2000 WHERE id=?", (m_c,)) + db._conn.commit() + + results = db.search_messages("matchword", sort="oldest") + session_order = [r["session_id"] for r in results] + assert session_order == ["b", "c", "a"], ( + f"sort=oldest must return earliest first; got {session_order}" + ) + + def test_search_messages_sort_unset_preserves_rank_ordering(self, db): + """No sort param → ``ORDER BY rank`` (FTS5 BM25). With identical + single-keyword matches on different-length messages, BM25 prefers + the shorter / denser ones — that's the existing default and it must + not regress when the new param is omitted.""" + db.create_session("short_sid", "cli") + db.create_session("long_sid", "cli") + # Single keyword in a short message scores higher than the same + # keyword buried in a much longer one (BM25 length normalisation). + m_short = db.append_message("short_sid", "user", "matchword.") + m_long = db.append_message( + "long_sid", "user", "matchword " + ("padding " * 200) + ) + # Older = short_sid so we can confirm rank wins, not recency. + with db._lock: + db._conn.execute("UPDATE messages SET timestamp=1000 WHERE id=?", (m_short,)) + db._conn.execute("UPDATE messages SET timestamp=2000 WHERE id=?", (m_long,)) + db._conn.commit() + + results = db.search_messages("matchword") # sort omitted + assert len(results) == 2 + # BM25 should rank the short message first despite being older. + assert results[0]["session_id"] == "short_sid", ( + "Default (no sort) must use FTS5 rank — short_sid should outrank " + f"the longer message. Got order: {[r['session_id'] for r in results]}" + ) + + def test_search_messages_sort_invalid_value_falls_back_to_rank(self, db): + """Passing a value outside the allowed set (e.g. 'sideways') silently + falls back to FTS5 rank-only ordering rather than raising. Same + forgiveness as the tool-layer normalisation, in case callers reach + SessionDB directly.""" + db.create_session("short_sid", "cli") + db.create_session("long_sid", "cli") + m_short = db.append_message("short_sid", "user", "matchword.") + m_long = db.append_message( + "long_sid", "user", "matchword " + ("padding " * 200) + ) + with db._lock: + db._conn.execute("UPDATE messages SET timestamp=1000 WHERE id=?", (m_short,)) + db._conn.execute("UPDATE messages SET timestamp=2000 WHERE id=?", (m_long,)) + db._conn.commit() + + # Garbage sort should behave the same as no sort. + results_default = db.search_messages("matchword") + results_garbage = db.search_messages("matchword", sort="sideways") + assert ( + [r["session_id"] for r in results_default] + == [r["session_id"] for r in results_garbage] + ) + class TestResolveSessionByNameOrId: """Tests for the main.py helper that resolves names or IDs.""" diff --git a/tests/tools/test_session_search.py b/tests/tools/test_session_search.py index aebd9d83ab..3e6c3b26d6 100644 --- a/tests/tools/test_session_search.py +++ b/tests/tools/test_session_search.py @@ -701,6 +701,119 @@ class TestSessionSearch: assert "aux_usage_total" not in result assert "aux_usage" not in result["results"][0] + def test_fast_mode_default_sort_is_relevance_only(self): + """Without ``sort``, fast mode passes ``sort=None`` to the DB layer so + the existing FTS5 ``ORDER BY rank`` behaviour is preserved. This locks + the default to time-neutral relevance — agents that don't think about + temporal direction get the same retrieval shape as before.""" + from unittest.mock import MagicMock + from tools.session_search_tool import session_search + + mock_db = MagicMock() + mock_db.search_messages.return_value = [] + mock_db.get_session.return_value = {"parent_session_id": None} + + session_search(query="foo", db=mock_db, mode="fast") + + call_kwargs = mock_db.search_messages.call_args.kwargs + assert call_kwargs.get("sort") is None, ( + "Default sort must be None so DB layer keeps FTS5 ORDER BY rank. " + f"Got sort={call_kwargs.get('sort')!r}" + ) + + def test_fast_mode_passes_newest_sort_to_db(self): + """``sort='newest'`` flows through to ``db.search_messages`` so the + DB layer can rewrite ORDER BY to put recent matches first.""" + from unittest.mock import MagicMock + from tools.session_search_tool import session_search + + mock_db = MagicMock() + mock_db.search_messages.return_value = [] + mock_db.get_session.return_value = {"parent_session_id": None} + + session_search(query="foo", db=mock_db, mode="fast", sort="newest") + + assert mock_db.search_messages.call_args.kwargs["sort"] == "newest" + + def test_fast_mode_passes_oldest_sort_to_db(self): + """``sort='oldest'`` flows through for origin-shaped questions + ('how did X start') — symmetric with newest.""" + from unittest.mock import MagicMock + from tools.session_search_tool import session_search + + mock_db = MagicMock() + mock_db.search_messages.return_value = [] + mock_db.get_session.return_value = {"parent_session_id": None} + + session_search(query="foo", db=mock_db, mode="fast", sort="oldest") + + assert mock_db.search_messages.call_args.kwargs["sort"] == "oldest" + + def test_fast_mode_sort_garbage_value_falls_back_to_default(self): + """Anything outside {'newest', 'oldest'} (case-insensitive) collapses + to None at the tool layer rather than failing the search. Forgiving + coercion — bad sort param doesn't mean no results.""" + from unittest.mock import MagicMock + from tools.session_search_tool import session_search + + mock_db = MagicMock() + mock_db.search_messages.return_value = [] + mock_db.get_session.return_value = {"parent_session_id": None} + + for bad in ("garbage", "", "RANDOM", 42, None): + mock_db.reset_mock() + session_search(query="foo", db=mock_db, mode="fast", sort=bad) + assert mock_db.search_messages.call_args.kwargs["sort"] is None, ( + f"Bad sort value {bad!r} should collapse to None" + ) + + def test_fast_mode_sort_is_case_insensitive(self): + """Mixed-case 'Newest' / 'OLDEST' normalise to canonical lowercase + values. Don't punish callers for case wobble.""" + from unittest.mock import MagicMock + from tools.session_search_tool import session_search + + mock_db = MagicMock() + mock_db.search_messages.return_value = [] + mock_db.get_session.return_value = {"parent_session_id": None} + + session_search(query="foo", db=mock_db, mode="fast", sort="NEWEST") + assert mock_db.search_messages.call_args.kwargs["sort"] == "newest" + + mock_db.reset_mock() + session_search(query="foo", db=mock_db, mode="fast", sort=" Oldest ") + assert mock_db.search_messages.call_args.kwargs["sort"] == "oldest" + + def test_summary_mode_silently_ignores_sort_parameter(self, monkeypatch): + """``sort`` is fast-mode-only by design. Passing it with mode='summary' + is a no-op (logged at debug level) — search proceeds with sort=None. + Prevents temporal bias from leaking into summary's session selection + without breaking callers that pass sort defensively.""" + from unittest.mock import MagicMock + from tools.session_search_tool import session_search + + async def fake_summarize(_text, _query, _meta): + return "summary", None + + monkeypatch.setattr("tools.session_search_tool._summarize_session", fake_summarize) + monkeypatch.setattr("model_tools._run_async", lambda coro: asyncio.run(coro)) + + mock_db = MagicMock() + mock_db.search_messages.return_value = [ + {"session_id": "sid", "source": "cli", "session_started": 1709500000, "model": "test"}, + ] + mock_db.get_session.return_value = {"parent_session_id": None, "source": "cli"} + mock_db.get_messages_as_conversation.return_value = [ + {"role": "user", "content": "transcript"}, + ] + + session_search(query="foo", db=mock_db, mode="summary", sort="newest") + + # Summary calls search_messages internally; sort must be stripped. + assert mock_db.search_messages.call_args.kwargs["sort"] is None, ( + "sort must be ignored outside fast mode" + ) + def test_positional_db_argument_remains_backwards_compatible(self): """Keep the historical positional order: query, role_filter, limit, db, current_session_id.""" from unittest.mock import MagicMock diff --git a/tools/session_search_tool.py b/tools/session_search_tool.py index 308757f246..e97f59b804 100644 --- a/tools/session_search_tool.py +++ b/tools/session_search_tool.py @@ -785,6 +785,11 @@ def session_search( around_message_id: int = None, window: int = 5, anchors: list = None, + # Fast-mode-only temporal bias for ranking. ``None`` keeps FTS5's BM25 + # ordering (time-neutral); ``"newest"`` / ``"oldest"`` make timestamp + # the primary key with rank as the tiebreaker. Silently ignored in + # other modes — see schema description. + sort: str = None, ) -> str: """ Search past sessions, or drill into a specific one. @@ -824,6 +829,22 @@ def session_search( if mode not in ("fast", "summary", "guided"): mode = "summary" + # Normalise sort. Only "newest" / "oldest" are accepted; anything else + # (including None) collapses to None = FTS5 rank-only ordering, which is + # the historical default. sort only affects fast mode — log and ignore + # in summary / guided / recent so misuse is visible but non-fatal. + sort_norm: Optional[str] = None + if isinstance(sort, str): + candidate = sort.strip().lower() + if candidate in ("newest", "oldest"): + sort_norm = candidate + if sort_norm and mode != "fast": + logging.debug( + "session_search: sort=%r is fast-mode only; ignored for mode=%s", + sort_norm, mode, + ) + sort_norm = None + # Guided mode is a different shape: it doesn't search, it drills. Branch # before FTS5 so we don't pay for anything we don't use, and so missing-arg # validation happens up front. @@ -866,13 +887,15 @@ def session_search( else: role_list = ["user", "assistant"] - # FTS5 search -- get matches ranked by relevance + # FTS5 search -- get matches ranked by relevance (with optional + # temporal bias when sort is set; see param docs). raw_results = db.search_messages( query=query, role_filter=role_list, exclude_sources=list(_HIDDEN_SESSION_SOURCES), limit=50, # Get more matches to find unique sessions offset=0, + sort=sort_norm, ) if not raw_results: @@ -1194,6 +1217,16 @@ SESSION_SEARCH_SCHEMA = { "user kickoff, the real opener is in the parent — fast-search again scoped to the " "parent if you need it. Most sessions are not compacted; this only matters on long " "multi-day arcs.\n\n" + "TEMPORAL DIRECTION (fast mode only): by default fast ranks by FTS5 relevance with no " + "time signal — fine for keyword discovery but ties are arbitrary, and same-keyword hits " + "from years ago can outrank fresh ones. Pass ``sort='newest'`` when the question is " + "shaped by recency (\"where did we leave X\", \"latest thinking on Y\", \"current status of " + "Z\") and ``sort='oldest'`` when it's shaped by origin (\"how did X start\", \"what was the " + "original take on Y\", \"first time we discussed Z\"). When the question is exploratory " + "(\"what do we know about X\") leave sort unset and let relevance lead. sort is silently " + "ignored in summary / guided / recent modes — for temporal narrative across multiple " + "sessions, drive the temporal selection from a fast call with sort set, then drill the " + "right anchors with guided.\n\n" "Browsing recent sessions: call with NO arguments to see what was worked on recently. " "Returns titles, previews, timestamps. Zero LLM cost, instant. Start here when the user " "asks 'what were we working on' or 'what did we do recently'.\n\n" @@ -1269,6 +1302,11 @@ SESSION_SEARCH_SCHEMA = { "description": "Mode='guided' only. Number of messages to return on each side of each anchor (the anchor itself is always included). Shared across all anchors in a multi-anchor call. Clamped to [1, 20]. Default 5.", "default": 5, }, + "sort": { + "type": "string", + "enum": ["newest", "oldest"], + "description": "Mode='fast' only. Temporal bias on top of FTS5 ranking. Omit to keep relevance-only ordering (the default, suitable for exploratory recall — 'what do we know about X'). Set 'newest' for recency-shaped questions ('where did we leave X', 'latest status of Y') so recent matches surface first with rank as the tiebreaker. Set 'oldest' for origin-shaped questions ('how did X start', 'first time we discussed Y') so the earliest matches surface first. Silently ignored in summary / guided / recent modes — for temporal narrative across sessions, drive fast with sort, then drill the right anchors with guided.", + }, }, "required": [], }, @@ -1291,6 +1329,7 @@ registry.register( around_message_id=args.get("around_message_id"), window=args.get("window", 5), anchors=args.get("anchors"), + sort=args.get("sort"), db=kw.get("db"), current_session_id=kw.get("current_session_id")), check_fn=check_session_search_requirements,