diff --git a/src/scribe/services/dedup.py b/src/scribe/services/dedup.py index bef0c42..eef78be 100644 --- a/src/scribe/services/dedup.py +++ b/src/scribe/services/dedup.py @@ -281,6 +281,12 @@ async def find_duplicate_note( # would refuse their write and point them at something they may not # be able to edit. scope="own", + # NOT demoted by supersession (#278). A superseded record is still a + # duplicate of what you are about to write — the claim is that it is + # no longer CURRENT, not that it is gone. Demoting it here would let + # the same note be recorded a second time, and the second copy would + # be the one nothing warns about. + demote_superseded=False, ) for score, note in hits: # semantic_search_notes doesn't filter note_type — enforce it here so diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 7161a73..de20b09 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -86,6 +86,69 @@ def _cosine_similarity(a: list[float], b: list[float]) -> float: return dot / (mag_a * mag_b) +# How much a superseded record is pushed down the ranking (#278). +# +# Chosen against a measurement, not by feel. On 2026-08-07 dev-log #2420 sat at +# 0.6120 on a query made of its own title phrase, 8th, behind #1759 at 0.6506 — +# a deficit of 0.039 to the top and ~0.014 to its nearest neighbours. A penalty +# of 0.05 clears that whole band, so demoting a cluster's stale members actually +# reorders it rather than shuffling within a tie. +# +# It is deliberately NOT large. Supersession is a claim about SOME of a record's +# content, so a superseded note that strongly answers a question nothing else +# answers should still surface — just behind anything comparable that is +# current. A penalty big enough to bury it outright would be hiding by another +# name, which is the thing the operator ruled out. +_SUPERSESSION_PENALTY = 0.05 + +# Candidates fetched per requested result when a re-rank follows. Three ranks of +# headroom is far more than a 0.05 penalty can move anything through in a corpus +# whose neighbours sit ~0.01-0.02 apart. +_SUPERSESSION_OVERFETCH = 3 + + +async def _apply_supersession_penalty( + scored: list[tuple[float, "Note"]], limit: int +) -> list[tuple[float, "Note"]]: + """Push superseded records below their equals, then take the top `limit`. + + The penalty is applied to the RANKING score and the returned score, so + downstream gates see the adjusted value — the auto-inject margin band in + particular, which exists to stop near-ties dragging in neighbours and would + otherwise re-tie exactly what this just separated. + + It is NOT applied to the relevance threshold: the floor decides whether a + record is relevant at all, the penalty decides which relevant record comes + first. Applying it to the floor would drop a superseded record out of the + results entirely — hiding, which is the one thing this must not do. + + Stable within a tie: Python's sort preserves the distance order the database + already established, so equal-scoring records keep their original sequence + rather than reshuffling per call. + """ + if not scored: + return [] + from scribe.services.supersession import superseded_ids + + try: + stale = await superseded_ids([int(note.id) for _score, note in scored]) + except Exception: + # Fail OPEN, and the direction matters: ranking without the penalty is + # the behaviour that shipped for months. Returning nothing, or raising, + # would turn a supersession-lookup hiccup into a broken search. + logger.warning("Supersession lookup failed — ranking unpenalised", exc_info=True) + return scored[:limit] + + if not stale: + return scored[:limit] + adjusted = [ + (score - _SUPERSESSION_PENALTY if int(note.id) in stale else score, note) + for score, note in scored + ] + adjusted.sort(key=lambda pair: pair[0], reverse=True) + return adjusted[:limit] + + def embedding_text(title: str | None, body: str | None) -> str: """The document a record is embedded AS. @@ -145,6 +208,7 @@ async def semantic_search_notes( task_kind: str | Sequence[str] | None = None, orphan_only: bool = False, scope: str = "own", + demote_superseded: bool = True, ) -> list[tuple[float, Note]]: """Return up to *limit* (score, note) pairs most relevant to *query*. @@ -176,6 +240,13 @@ async def semantic_search_notes( so a similarity floor of *threshold* is a distance ceiling of ``1 - threshold`` and similarity is recovered as ``1 - distance``. + `demote_superseded` applies the supersession penalty (#278): a record a + later note claims to have overtaken ranks below its equals. Callers asking + "what is the current answer" want it; the near-duplicate gate does NOT, and + passes False — a superseded record is still a duplicate of what you are + about to write, and demoting it there would let the same note be recorded + twice, the second time invisibly. + Returns an empty list if the embedder is unavailable or on any error. """ if not query or not query.strip(): @@ -232,14 +303,38 @@ async def semantic_search_notes( ) if exclude_ids: stmt = stmt.where(NoteEmbedding.note_id.notin_(exclude_ids)) - stmt = stmt.where(distance <= max_distance).order_by(distance.asc()).limit(limit) + # OVER-FETCH when a re-rank follows, so the demotion can actually + # move something. Demoting after a LIMIT k would be theatre: the cut + # already happened, so a superseded record pushed down still sits in + # the results and the live record that should have replaced it was + # never fetched. + # + # Ordering stays on RAW distance so pgvector's HNSW index still + # serves it (migration 0067). Ordering by `distance + penalty` + # instead would be exact, and would turn an indexed top-k into a + # scan-and-sort of every embedded note. + # + # The cost of that trade, stated plainly: a live record outside the + # over-fetch window cannot be promoted into the results. With a + # penalty far smaller than the window's score spread, that case + # needs the true answer to be more than _SUPERSESSION_OVERFETCH + # ranks down, which no observed query comes close to. + fetch = limit * _SUPERSESSION_OVERFETCH if demote_superseded else limit + stmt = ( + stmt.where(distance <= max_distance) + .order_by(distance.asc()) + .limit(fetch) + ) rows = list((await session.execute(stmt)).all()) except Exception: logger.warning("Failed to query note embeddings", exc_info=True) return [] # Recover similarity (1 - distance) and preserve the highest-first contract. - return [(1.0 - float(dist), note) for note, dist in rows] + scored = [(1.0 - float(dist), note) for note, dist in rows] + if not demote_superseded: + return scored[:limit] + return await _apply_supersession_penalty(scored, limit) async def backfill_note_embeddings() -> None: diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 192c3b8..39a8537 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -31,6 +31,7 @@ from scribe.services import snippets as snippets_svc from scribe.services.access import label_shared_items, owner_names_for from scribe.services.embeddings import semantic_search_notes from scribe.services.note_usage import record_surfaced +from scribe.services.supersession import superseded_ids from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.settings import get_setting @@ -425,11 +426,19 @@ async def build_autoinject_hint( "`get_note(id)`, or `get_snippet` / `get_process` for those kinds " "(titles only; injected once per session):", ] + # A superseded record is DEMOTED, not removed (#278) — so one can still reach + # this menu, and when it does the reader has to be told. An agent handed + # stale material with nothing marking it acts on it with full confidence, + # which is worse than never having surfaced it. One query for the whole menu. + stale = await superseded_ids([int(n.id) for _s, n in kept]) + note_ids: list[int] = [] for score, note in kept: note_ids.append(int(note.id)) title = (note.title or "(untitled)").replace("\n", " ").strip() line = f"> - #{note.id} [{_record_kind(note)}] \"{title}\" ({score:.2f})" + if int(note.id) in stale: + line += " — SUPERSEDED, a later record covers this; check that first" if note.user_id != user_id: who = owners.get(int(note.user_id)) or "another user" line += f" — shared by {who}, treat as a suggestion" diff --git a/tests/test_supersession_ranking.py b/tests/test_supersession_ranking.py new file mode 100644 index 0000000..23cd778 --- /dev/null +++ b/tests/test_supersession_ranking.py @@ -0,0 +1,154 @@ +"""Demotion: superseded records rank behind their equals, and are never hidden. + +Step 3 of #278. The distinction these tests exist to protect is DEMOTE vs +FILTER. The operator was explicit: + + "the failure is pollution, not existence" + +Hiding a superseded record would turn every one of them into something you must +already know exists in order to find, and would destroy "what did we think +then" — half the reason a log is kept. So every test here that could be +satisfied by dropping a record instead checks that it is still present. +""" +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services.embeddings import ( + _SUPERSESSION_PENALTY, + _apply_supersession_penalty, +) + + +def _note(note_id: int): + n = MagicMock() + n.id = note_id + return n + + +def _stale(*ids): + return patch( + "scribe.services.supersession.superseded_ids", + AsyncMock(return_value=set(ids)), + ) + + +@pytest.mark.asyncio +async def test_a_superseded_record_falls_behind_an_equal_live_one(): + scored = [(0.70, _note(1)), (0.69, _note(2))] # 1 leads on raw score + with _stale(1): + out = await _apply_supersession_penalty(scored, limit=5) + assert [int(n.id) for _s, n in out] == [2, 1] + + +@pytest.mark.asyncio +async def test_a_strong_superseded_record_still_beats_a_weak_live_one(): + """Demote, not filter — and this is why the penalty is small. + + Supersession is a claim about SOME of a record's content. One that strongly + answers a question nothing else answers should still surface, just behind + anything comparable that is current. + """ + scored = [(0.90, _note(1)), (0.50, _note(2))] + with _stale(1): + out = await _apply_supersession_penalty(scored, limit=5) + assert [int(n.id) for _s, n in out] == [1, 2] + assert out[0][0] == pytest.approx(0.90 - _SUPERSESSION_PENALTY) + + +@pytest.mark.asyncio +async def test_the_superseded_record_is_still_returned(): + """The whole point. A test that only checked ordering would pass just as + happily against an implementation that dropped it.""" + scored = [(0.70, _note(1))] + with _stale(1): + out = await _apply_supersession_penalty(scored, limit=5) + assert [int(n.id) for _s, n in out] == [1] + + +@pytest.mark.asyncio +async def test_the_returned_score_is_the_adjusted_one(): + """Downstream gates must see the adjusted value — the auto-inject margin + band in particular, which exists to stop near-ties dragging in neighbours + and would otherwise re-tie exactly what this just separated.""" + scored = [(0.70, _note(1)), (0.68, _note(2))] + with _stale(1): + out = await _apply_supersession_penalty(scored, limit=5) + by_id = {int(n.id): s for s, n in out} + assert by_id[1] == pytest.approx(0.65) + assert by_id[2] == pytest.approx(0.68) + + +@pytest.mark.asyncio +async def test_nothing_superseded_leaves_the_order_untouched(): + scored = [(0.70, _note(1)), (0.69, _note(2)), (0.60, _note(3))] + with _stale(): + out = await _apply_supersession_penalty(scored, limit=5) + assert [int(n.id) for _s, n in out] == [1, 2, 3] + + +@pytest.mark.asyncio +async def test_ties_keep_their_database_order(): + """Stable sort. Equal scores must not reshuffle per call — a menu that + reorders between identical queries reads as nondeterminism and sends + someone hunting for a bug that isn't there.""" + scored = [(0.70, _note(1)), (0.70, _note(2)), (0.70, _note(3))] + with _stale(): + out = await _apply_supersession_penalty(scored, limit=5) + assert [int(n.id) for _s, n in out] == [1, 2, 3] + + +@pytest.mark.asyncio +async def test_the_limit_is_applied_after_reordering(): + """Over-fetching is pointless if the cut happens first. Three candidates, + limit 2, and the demoted leader must be the one that falls out.""" + scored = [(0.70, _note(1)), (0.69, _note(2)), (0.68, _note(3))] + with _stale(1): + out = await _apply_supersession_penalty(scored, limit=2) + assert [int(n.id) for _s, n in out] == [2, 3] + + +@pytest.mark.asyncio +async def test_a_failed_lookup_returns_unpenalised_results_not_none(): + """Fail OPEN, and the direction matters. Ranking without the penalty is the + behaviour that shipped for months; returning nothing would turn a + supersession hiccup into a broken search.""" + scored = [(0.70, _note(1)), (0.69, _note(2))] + with patch("scribe.services.supersession.superseded_ids", + AsyncMock(side_effect=RuntimeError("db gone"))): + out = await _apply_supersession_penalty(scored, limit=5) + assert [int(n.id) for _s, n in out] == [1, 2] + assert out[0][0] == pytest.approx(0.70) + + +@pytest.mark.asyncio +async def test_an_empty_candidate_set_short_circuits(): + """No candidates means no lookup — this runs on every ranked query, and a + round trip to learn nothing is a round trip too many.""" + called = AsyncMock(return_value=set()) + with patch("scribe.services.supersession.superseded_ids", called): + assert await _apply_supersession_penalty([], limit=5) == [] + called.assert_not_awaited() + + +def test_the_penalty_is_sized_to_reorder_a_cluster_not_shuffle_within_it(): + """Sized against a measurement, not by feel. + + Measured 2026-08-07 (#2486): the notes competing on a dev-log's own title + phrase sat within ~0.014 of each other, spanning 0.6506 down to 0.6120. A + penalty smaller than that spread would move a record within a tie without + changing which one wins — the failure #2486 already proved cannot be tuned + away, because the neighbours are not barely passing, they are tied. + + The upper bound is the operator's constraint, not an optimisation: a penalty + large enough to bury a superseded record outright is hiding by another name. + """ + assert _SUPERSESSION_PENALTY > 0.014, ( + "must exceed the measured neighbour spread, or it reorders nothing" + ) + assert _SUPERSESSION_PENALTY < 0.15, ( + "must not bury a superseded record outright — that is hiding, which " + "the operator ruled out" + )