diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index b870433..5df0494 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -14,7 +14,9 @@ import logging import math import os -from sqlalchemy import delete, select +from collections.abc import Sequence + +from sqlalchemy import delete, or_, select from scribe.models import async_session from scribe.models.embedding import NoteEmbedding @@ -114,7 +116,8 @@ async def semantic_search_notes( threshold: float = _SIMILARITY_THRESHOLD, project_id: int | None = None, is_task: bool | None = None, - note_type: str | None = None, + note_type: str | Sequence[str] | None = None, + task_kind: str | Sequence[str] | None = None, orphan_only: bool = False, scope: str = "own", ) -> list[tuple[float, Note]]: @@ -123,8 +126,15 @@ async def semantic_search_notes( Scores are cosine similarities in [-1, 1]; only notes at or above *threshold* are returned, sorted highest-first. - `note_type` narrows to a single record kind (e.g. "snippet"), for callers - that want prior art rather than everything embedded. + `note_type` narrows to a record kind, or several (e.g. "snippet", or + ("snippet", "note")), for callers that want prior art rather than everything + embedded. + + `task_kind` restricts TASKS to the given kinds while leaving non-task notes + untouched. That asymmetry is the point: "recorded experience" is issues plus + dev-logs, and those differ on `is_task`, so neither `note_type` nor `is_task` + alone can express it. With `note_type="note", task_kind="issue"` a caller + gets fixed problems and durable notes without the open to-do list. `scope` ("own" | "browse" | "read", see access.notes_visibility_clause) decides how far this may see. It exists because this one function serves @@ -179,11 +189,22 @@ async def semantic_search_notes( stmt = stmt.where(Note.status.isnot(None)) elif is_task is False: stmt = stmt.where(Note.status.is_(None)) - # Narrow to one kind of record. Composes with is_task rather than - # replacing it — 'snippet' is a non-task note_type, so a caller asking - # for prior art gets snippets and not the dev-log that mentions them. + # Narrow to one kind of record, or several. Composes with is_task + # rather than replacing it — 'snippet' is a non-task note_type, so a + # caller asking for prior art gets snippets and not the dev-log that + # mentions them. if note_type: - stmt = stmt.where(Note.note_type == note_type) + kinds = [note_type] if isinstance(note_type, str) else list(note_type) + stmt = stmt.where(Note.note_type.in_(kinds)) + # Restrict TASKS to certain kinds while leaving notes alone. A note + # has no task_kind that means anything, so a plain `.in_()` would + # drop every dev-log — which is exactly the record a caller asking + # for prior experience wants most. + if task_kind: + tkinds = [task_kind] if isinstance(task_kind, str) else list(task_kind) + stmt = stmt.where( + or_(Note.status.is_(None), Note.task_kind.in_(tkinds)) + ) if exclude_ids: stmt = stmt.where(NoteEmbedding.note_id.notin_(exclude_ids)) stmt = stmt.where(distance <= max_distance).order_by(distance.asc()).limit(limit) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index f07d311..32ecbe7 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -290,6 +290,65 @@ def _record_kind(note) -> str: return note.note_type or "note" +_REUSE_KINDS = ("snippet", "process") + + +async def _reserve_slot_for_reuse( + user_id: int, + query: str, + kept: list, + cfg: dict, + *, + project_id: int | None, + exclude_ids: set[int], +) -> list: + """Guarantee the reuse-shaped kinds one slot, if one clears threshold (#2246). + + Ranking by raw cosine is blind to what KIND of record answers what kind of + ask, and the corpus makes that fatal rather than merely imperfect: Scribe's + project records are *about software work*, so a task titled "surface snippets + before the agent writes code" is a near-perfect lexical match for "write a + function…" while being useless as an answer to it. Measured live, a prompt + asking for a helper returned three records about BUILDING the retrieval + system and zero snippets. + + The bias is structural and gets WORSE as the project record grows — which is + the direction Scribe is supposed to grow. Snippets are ~0.5% of the corpus + here; no threshold tuning fixes a 200:1 ratio. + + So the reserved hit is deliberately NOT held to the margin band. The band + measures distance from the top overall score, and that top score is the very + thing snippets lose to. It still has to clear the configured threshold, so a + weak snippet cannot buy the slot — silence stays the default. + """ + if any(_record_kind(n) in _REUSE_KINDS for _s, n in kept): + return kept # reuse already represented; nothing to do + + top_k = cfg["top_k"] + reuse = await semantic_search_notes( + user_id, query, + limit=1, + threshold=cfg["threshold"], + project_id=project_id, + exclude_ids=exclude_ids | {int(n.id) for _s, n in kept}, + note_type=_REUSE_KINDS, + scope="browse", + ) + # Belt and braces on top of exclude_ids: a record already on the menu must + # never appear twice, and one slot is all this is entitled to. + kept_ids = {int(n.id) for _s, n in kept} + fresh = [(s, n) for s, n in reuse if int(n.id) not in kept_ids][:1] + if not fresh: + return kept + + # Take the LAST slot, never the first: the strongest overall hit is still the + # best answer to the prompt, and displacing it would trade one blindness for + # another. + if len(kept) >= top_k: + return kept[:top_k - 1] + fresh + return (kept + fresh)[:top_k] + + async def build_autoinject_hint( user_id: int, query: str, @@ -341,6 +400,10 @@ async def build_autoinject_hint( # Margin gate: keep only hits close to the strongest one. top_score = hits[0][0] kept = [(s, n) for s, n in hits if s >= top_score - _AUTOINJECT_BAND] + kept = await _reserve_slot_for_reuse( + user_id, q, kept, cfg, project_id=(project_id or None), + exclude_ids=set(exclude_ids or []), + ) # A collaborator's note can reach this menu via a shared project, and the # operator never asked for it — so say whose it is. Unattributed, it reads as @@ -685,7 +748,20 @@ async def build_write_path_hint( threshold=cfg["threshold"], project_id=scope_project, exclude_ids=seen, - note_type="snippet", + # Snippets AND recorded experience (#2246). This arm was + # snippets-only, which is auto-inject's mistake inverted: an issue + # saying "we tried this and it deadlocked", or a dev-log recording + # how a problem was solved, is prior art for the code about to be + # written — arguably better prior art than a resembling helper, + # because it says what NOT to do. + # + # `task_kind="issue"` keeps the open to-do list out. A task titled + # "add debouncing to the search box" resembles the code being + # written and answers nothing; an ISSUE is corrective work with a + # root cause in it, and a non-task note is durable knowledge. Both + # earned their place; a todo did not. + note_type=("snippet", "note"), + task_kind="issue", # Same reasoning as auto-inject: nobody asked for this, so it takes # the browse scope and never surfaces a one-to-one direct share. scope="browse", @@ -693,7 +769,10 @@ async def build_write_path_hint( record_retrieval( user_id=user_id, source="write_path", query=query, threshold=cfg["threshold"], limit=remaining, - project_id=scope_project, is_task=False, results=hits, + # is_task is None, not False: this arm now returns issues too, and + # recording it as a notes-only retrieval would misdescribe the + # candidate set the threshold is being tuned against. + project_id=scope_project, is_task=None, results=hits, duration_ms=(time.perf_counter() - t0) * 1000.0, ) if hits: @@ -701,8 +780,15 @@ async def build_write_path_hint( for score, note in hits: if score < top_score - _AUTOINJECT_BAND: continue + # Name the kind unless it's a snippet — the menu's default and + # the header's default reading. An issue or a dev-log offered + # here is a different KIND of claim ("this was already tried") + # and an unlabelled line would be read as "here is code to + # reuse", which is the opposite of what it says. + kind = _record_kind(note) scored.append(( - f"similar {score:.2f}", + f"similar {score:.2f}" if kind == "snippet" + else f"similar {score:.2f} · {kind}", { "id": int(note.id), "title": note.title, "user_id": note.user_id, # Carried so the line can disclose a cross-language hit @@ -733,7 +819,9 @@ async def build_write_path_hint( lines = [ f"> Prior art already recorded in Scribe for `{path}` — open one with " - "`get_snippet(id)` and reuse it rather than writing a fresh one-off " + "`get_snippet(id)` for a snippet, `get_task(id)` for an issue, " + "`get_note(id)` otherwise. Reuse a snippet rather than writing a fresh " + "one-off; read an issue before repeating what it records " "(titles only; shown once per session):", ] # Say what a language tag MEANS, and only when one is actually on the menu. diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index e73ca26..5754686 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -295,3 +295,174 @@ async def test_build_session_context_caps_length(): assert len(out["context"]) <= 9000 + 60 # cap + truncation note assert "truncated" in out["context"] + + +# --- the reuse slot (#2246) -------------------------------------------------- + +_CFG = {"enabled": True, "threshold": 0.55, "top_k": 3} + + +async def _autoinject(main_hits, reuse_hits, cfg=None): + """Run build_autoinject_hint with the two semantic queries stubbed in order: + the unscoped pool first, then the reserved reuse query.""" + calls: list[dict] = [] + + async def fake_search(*_a, **kw): + calls.append(kw) + return reuse_hits if kw.get("note_type") else main_hits + + with patch("scribe.services.plugin_context.get_autoinject_config", + AsyncMock(return_value=dict(cfg or _CFG))), \ + patch("scribe.services.plugin_context.semantic_search_notes", + AsyncMock(side_effect=fake_search)), \ + patch("scribe.services.plugin_context.record_retrieval", MagicMock()), \ + patch("scribe.services.plugin_context.record_surfaced", MagicMock()), \ + patch("scribe.services.plugin_context.owner_names_for", + AsyncMock(return_value={})): + from scribe.services.plugin_context import build_autoinject_hint + out = await build_autoinject_hint(1, "write a debounce helper") + return out, calls + + +@pytest.mark.asyncio +async def test_a_snippet_takes_the_last_slot_when_none_won_on_score(): + """The measured failure: a prompt asking for a helper returned three project + records ABOUT building the retrieval system, and zero snippets. Scribe's own + records are about software work, so they share vocabulary with any coding + prompt while answering none of them.""" + main = [(0.66, _note(1, "Step 3: title-first auto-inject")), + (0.65, _note(2, "Task-reminder dedup query crashes", is_task=True)), + (0.64, _note(3, "Drafter hardening · write-path trigger", is_task=True))] + reuse = [(0.58, _note(9, "debounce — collapse rapid calls", note_type="snippet"))] + + out, calls = await _autoinject(main, reuse) + + assert out["note_ids"] == [1, 2, 9] # last slot displaced, not the first + assert "#9" in out["context"] and "[snippet]" in out["context"] + # The reserved query is scoped to the reuse kinds and asks for exactly one. + assert calls[1]["note_type"] == ("snippet", "process") + assert calls[1]["limit"] == 1 + + +@pytest.mark.asyncio +async def test_the_reserved_query_is_skipped_when_a_snippet_already_won(): + """No second query, and no slot spent twice, when ranking already did the + right thing — the fix must be invisible in the case it isn't needed.""" + main = [(0.81, _note(9, "debounce helper", note_type="snippet")), + (0.80, _note(1, "some task", is_task=True))] + + out, calls = await _autoinject(main, []) + + assert len(calls) == 1 # reserved query never ran + assert out["note_ids"] == [9, 1] + + +@pytest.mark.asyncio +async def test_a_weak_snippet_does_not_buy_the_slot(): + """The reserved hit skips the MARGIN band — that band is what snippets lose + to — but never the threshold. Silence stays the default; a slot spent on an + irrelevant snippet is how a menu teaches people to ignore it.""" + main = [(0.66, _note(1, "a task", is_task=True))] + + out, calls = await _autoinject(main, []) # threshold returned nothing + + assert out["note_ids"] == [1] + assert len(calls) == 2 # it asked, and got nothing + + +@pytest.mark.asyncio +async def test_the_reserved_hit_is_not_held_to_the_margin_band(): + """0.58 is 0.08 below the top hit. Under the band it would survive; the point + is that it must survive even when it wouldn't — a snippet losing to a + same-vocabulary project record by a wide margin is the whole bug.""" + main = [(0.90, _note(1, "a task", is_task=True))] + reuse = [(0.58, _note(9, "debounce", note_type="snippet"))] + + out, _calls = await _autoinject(main, reuse) + + assert 9 in out["note_ids"] + + +@pytest.mark.asyncio +async def test_a_process_counts_as_reuse_too(): + """A stored process answers 'how do we do X here' the same way a snippet + answers 'what do we already have' — both lose to the same project records.""" + main = [(0.70, _note(1, "a task", is_task=True))] + reuse = [(0.60, _note(8, "DRY pass process", note_type="process"))] + + out, _ = await _autoinject(main, reuse) + assert 8 in out["note_ids"] + + # …and one already on the menu suppresses the reserved query. + out2, calls2 = await _autoinject( + [(0.70, _note(8, "DRY pass process", note_type="process"))], []) + assert len(calls2) == 1 + + +# --- write-path widened beyond snippets (#2246, the mirror half) ------------- + +@pytest.mark.asyncio +async def test_write_path_semantic_arm_asks_for_experience_not_just_snippets(): + """The inverse of auto-inject's mistake. This arm was snippets-only, so an + issue saying "we tried this and it deadlocked" could never reach the moment + that code was about to be written — arguably the better prior art, because + it says what NOT to do.""" + from scribe.services import plugin_context as pc + + hits = [(0.72, _note(9, "debounce helper", note_type="snippet")), + (0.70, _note(7, "Debounce dropped the trailing call", is_task=True, + task_kind="issue"))] + search = AsyncMock(return_value=hits) + rec = MagicMock() + with patch.object(pc, "get_writepath_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.6, + "top_k": 3})), \ + patch.object(pc.snippets_svc, "list_snippets", + AsyncMock(return_value=([], 0))), \ + patch.object(pc, "semantic_search_notes", search), \ + patch.object(pc, "record_retrieval", rec), \ + patch.object(pc, "record_surfaced", MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "concept_query", MagicMock(return_value="debounce a callback")): + out = await pc.build_write_path_hint( + 1, "src/utils/debounce.ts", code="x" * 400, + ) + + kw = search.await_args.kwargs + assert kw["note_type"] == ("snippet", "note") + # An open to-do resembling the code answers nothing; an ISSUE carries a root + # cause and a NOTE carries durable knowledge. Only the todo is excluded. + assert kw["task_kind"] == "issue" + # Telemetry must not claim this was a notes-only retrieval any more. + assert rec.call_args.kwargs["is_task"] is None + + +@pytest.mark.asyncio +async def test_write_path_labels_a_non_snippet_hit_with_its_kind(): + """An unlabelled issue on this menu reads as "here is code to reuse", which + is the opposite of what it says. A snippet stays unlabelled — it is the + menu's default and the header's default reading.""" + from scribe.services import plugin_context as pc + + hits = [(0.72, _note(9, "debounce helper", note_type="snippet")), + (0.71, _note(7, "Debounce dropped the trailing call", is_task=True, + task_kind="issue"))] + with patch.object(pc, "get_writepath_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.6, + "top_k": 3})), \ + patch.object(pc.snippets_svc, "list_snippets", + AsyncMock(return_value=([], 0))), \ + patch.object(pc, "semantic_search_notes", AsyncMock(return_value=hits)), \ + patch.object(pc, "record_retrieval", MagicMock()), \ + patch.object(pc, "record_surfaced", MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "concept_query", MagicMock(return_value="debounce a callback")): + out = await pc.build_write_path_hint( + 1, "src/utils/debounce.ts", code="x" * 400, + ) + + ctx = out["context"] + assert "· issue]" in ctx # the issue says what it is + assert '[similar 0.72] "debounce helper"' in ctx # the snippet does not + # The header now names the right opener for each kind. + assert "get_task(id)" in ctx and "get_snippet(id)" in ctx