feat(session_search): add sort param for fast-mode temporal direction
Fast mode currently orders results by FTS5 BM25 rank only. That's correct
when the user's question is exploratory ('what do we know about X') —
relevance leads, time is neutral — but it actively hurts two other common
question shapes:
1. Recency-shaped: 'where did we leave X', 'latest status of Y'. Same-rank
matches from years ago and yesterday are tied; FTS5 picks arbitrarily.
A reactivated old session can outrank a fresh one with no signal.
2. Origin-shaped: 'how did X start', 'first time we discussed Y'. The
originating session is usually short and gets out-scored by later
sessions that revisit the topic with more context — the origin hides
under its own descendants.
Adding a temporal tie-breaker by default would silently bias every query
toward 'latest', breaking the origin-shaped case. So sort is opt-in and
bidirectional, matching the existing 'agent picks the mode that fits the
question shape' pattern.
What this adds:
- session_search() gains a sort parameter accepting 'newest', 'oldest',
or None (default = current FTS5 rank-only behaviour preserved).
- db.search_messages() honours sort across all three SQL paths: main
FTS5 (timestamp DESC/ASC primary, rank tiebreaker), trigram CJK
(same), LIKE fallback (timestamp direction flip; no rank to combine).
- Tool layer normalises sort case-insensitively, falls back to None on
garbage values rather than failing the search, and silently strips
sort outside fast mode (with a debug log). Summary's session
selection deliberately stays time-neutral — agents wanting temporal
narrative drive fast with sort, then drill anchors with guided.
- Schema description gains a TEMPORAL DIRECTION section with concrete
question-shape examples, and a sort property on the parameters
block enumerating the valid values.
Tests:
- 6 new tool-layer tests covering default behaviour, both directions,
case-insensitivity, garbage fallback, and silent-ignore in summary.
- 4 new SQL-layer tests against the real DB exercising 'newest' /
'oldest' / unset (BM25 rank preserved) / invalid (rank fallback).
- 95→102 passing on tools/test_session_search.py before this commit;
108 passing after.
This commit is contained in:
+46
-3
@@ -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])
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user