diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 8b6886b..320adcf 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -40,6 +40,7 @@ from scribe.services.retrieval_surfaces import ( ) from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.settings import get_setting +from scribe.services.systems import system_names_for from scribe.services.text import elide logger = logging.getLogger(__name__) @@ -47,23 +48,71 @@ logger = logging.getLogger(__name__) # Defensive cap below Claude Code's 10k additionalContext limit. _MAX_CHARS = 9000 -# Max chars of the matched passage shown under an injected menu line. +# WHAT A MENU LINE CARRIES (#4364): the record's NAME, its kind and System, +# and the WHOLE passage that matched. Metadata plus the evidence, rather than a +# title asked to be both. # -# The menu used to be titles alone, on the reasoning that its job is AWARENESS — -# make the agent know the record exists and reach for it, not dump it. That -# holds for a lesson or a snippet, whose title carries its trigger by -# construction ("what — when it applies"). It does not hold for an issue, a -# dev-log or a plain note, where the title is a headline and the reason this -# record matched is a sentence somewhere inside it. The reader was being asked -# "is this worth opening?" and handed the one part of the record guaranteed not -# to answer it. +# The name, not the title. A snippet's or lesson's title is `name — when it +# applies` by construction (`embeddings.trigger_title`), because that join is +# what makes it rank on its situation. That is an EMBEDDING shape, and rendered +# as a menu line it ran to 1,500+ characters — the trigger paragraph spent +# again on every line, and again on every repeat. The trigger still arrives +# when it is what matched: it is in the chunk, and `_menu_passage` hands it +# over when the title was the whole match. # -# 200 rather than more because this is a menu: eight lines at 200 is ~1.6KB, -# which buys the decision without turning an awareness push into a dump. It is -# the PASSAGE THAT MATCHED, not the record's opening — the search already knows -# which one that is and used to throw it away (#4243) — so 200 characters here -# are worth far more than 200 characters of preamble. -_MENU_PASSAGE_CHARS = 200 +# The whole passage, not 200 characters of it. The search already chose the +# chunk that matched; the old cut kept its head and tail, and the head is the +# title every chunk is prefixed with — so the reader got the title twice and +# lost the middle, which is where the match was (lesson #4248). A chunk is at +# most ~1.4 KB (`embeddings._CHUNK_CHAR_BUDGET`), and it is shown once: a +# repeat is a one-line pointer (`_menu_seen_line`), not a second copy. + + +def _menu_name(title: str | None, note_type: str | None, data=None, body: str | None = "") -> str: + """The record's name — its title without the trigger composed into it.""" + title = (title or "(untitled)").replace("\n", " ").strip() + data = data if isinstance(data, dict) else {} + if note_type == "snippet": + from scribe.services.embeddings import TRIGGER_SEP + return (data.get("name") or title.partition(TRIGGER_SEP)[0]).strip() or title + if note_type == LESSON_NOTE_TYPE: + from types import SimpleNamespace + + from scribe.services.embeddings import untrigger_title + from scribe.services.lessons import lesson_trigger + trigger = lesson_trigger(SimpleNamespace(data=data, body=body or "")) + return untrigger_title(title, trigger).strip() or title + return title + + +def _menu_passage(title: str | None, chunk_text: str | None, name: str = "") -> str: + """The matched chunk on one line, without the title it was embedded under. + + Every chunk is `title\nsection` (`embeddings.embedding_text`), so the title + prefix is stripped exactly. A chunk that WAS only the title — a short + record, or the head chunk of one — matched on the title, and for a + trigger-keyed kind the part of it the name line no longer shows is the + trigger: that is returned, because it is precisely what matched. + One line, so the menu's blockquote survives it. + """ + title = (title or "").strip() + text = (chunk_text or "").strip() + if title and text.startswith(title): + text = text[len(title):] + text = " ".join(text.split()) + if not text and name and title.startswith(name) and title != name: + text = " ".join(title[len(name):].lstrip(" —-").split()) + return text + + +def _menu_label(kind: str, systems: list[str] | None) -> str: + """`issue (done) · Plugin & hooks` — the kind, then where it belongs.""" + return " · ".join([kind, *systems]) if systems else kind + + +def _menu_seen_line(note_id: int, kind: str, name: str) -> str: + """A pointer to a record this session was already shown, not a copy of it.""" + return f"> - #{note_id} [{kind} · seen] {name}" # Max chars of a Process body to fold into the auto-surface description. _PROC_PREVIEW_CHARS = 200 @@ -1115,8 +1164,9 @@ async def build_autoinject_hint( lines = [ "> Possibly relevant from your Scribe records — open any in full with " "`get_note(id)`, or `get_snippet` / `get_process` / `get_lesson` for " - "those kinds (titles only; a line marked `seen` was surfaced earlier " - "this session and may no longer be in context):", + "those kinds. Each line is a record's name, its kind and System, and " + "the passage that matched; a line marked `seen` is a pointer to one " + "already shown this session, so it is in your context:", ] # THE REGISTER, SAID ONCE AND ONLY WHEN IT APPLIES (milestone 385 step 5). # @@ -1152,34 +1202,42 @@ async def build_autoinject_hint( # with the query that actually matched it. menu_chunks = _rep_ai.get("best_chunk") or {} + systems = await system_names_for({int(n.id) for _s, n in kept if int(n.id) not in already}) 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})" - # ONE WORD, NOT A SENTENCE, and deliberately not the rule arms' phrasing. - # A rule line says "before deciding it does not apply", which is the - # voice of a record that BINDS; a note binds nothing, and borrowing that - # tone would tell the reader a dev-log has authority it does not have. - # The header carries the meaning, so the line carries only the flag. - if int(note.id) in already: - line += " [seen]" - if int(note.id) in stale: + nid = int(note.id) + note_ids.append(nid) + kind = _record_kind(note) + # The NAME, not the title (#4364): a snippet's or lesson's title is its + # embedding shape, trigger and all, and ran past 1,500 characters here. + name = _menu_name(note.title, note.note_type, note.data, note.body) + if nid in already: + # A POINTER, not a copy (#4364). The record is in this session's + # context already — the ledger is cleared at compaction, so "seen" + # stays true — and re-rendering it spent its whole line again for + # nothing. What the reader needs is the reminder that it matched + # again, and the id to open it if it has scrolled out of mind. + line = _menu_seen_line(nid, kind, name) + if nid in stale: + line += " — SUPERSEDED" + lines.append(line) + continue + line = f"> - #{nid} [{_menu_label(kind, systems.get(nid))}] \"{name}\" ({score:.2f})" + if nid 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" lines.append(line) - # The passage that earned the line, indented under it. Absent when the - # record has no stored chunk — an un-embedded row, or the reserved - # lesson and reuse slots, which are fetched by their own queries and so - # are not in this search's report. No fallback to the body's opening: - # on a menu that would be a line of preamble dressed as a reason, and a - # reader cannot tell the two apart once they are indented identically. - passage = (menu_chunks.get(int(note.id)) or {}).get("text") or "" - if passage.strip(): - short, _cut = elide(" ".join(passage.split()), _MENU_PASSAGE_CHARS) - lines.append(f"> ↳ {short}") + # The passage that earned the line, WHOLE, indented under it (#4364). + # Absent when the record has no stored chunk — an un-embedded row, or + # the reserved lesson and reuse slots, which are fetched by their own + # queries and so are not in this search's report. No fallback to the + # body's opening: on a menu that would be a line of preamble dressed as + # a reason, and a reader cannot tell the two apart once indented alike. + passage = _menu_passage(note.title, (menu_chunks.get(nid) or {}).get("text"), name) + if passage: + lines.append(f"> ↳ {passage}") # Records what SURVIVED the margin gate, not what the ranker returned — the # menu the agent actually saw. retrieval_logs already holds the full @@ -1469,7 +1527,12 @@ def _prior_art_line(item: dict, marker: str, owner: str | None, foreign_lang: st rather than appended after the title, so the reader sees it while still reading the score — the two together are the judgement being offered. """ - title = (item.get("title") or "(untitled)").replace("\n", " ").strip() + # The NAME, not the composed title (#4364) — a snippet's title carries its + # whole trigger and ran to kilobytes on this line, again on every repeat. + title = ( + item.get("name") or (item.get("snippet") or {}).get("name") + or (item.get("title") or "(untitled)") + ).replace("\n", " ").strip() mark = f"{marker} · {foreign_lang}" if foreign_lang else marker line = f"> - #{item['id']} [{mark}] \"{title}\"" if owner: @@ -2078,6 +2141,7 @@ async def build_write_path_hint( # dropped — decided by which arm happened to find them — is worse than # either rule applied consistently: the marker would read as a complete # account of what the session has met before, and it would not be one. + item["seen"] = nid in excluded placed.append(("nearby · seen" if nid in excluded else "nearby", item)) # The stamping feed's "actually pulled it" half (#2791). Read once, before @@ -2255,6 +2319,12 @@ async def build_write_path_hint( marker, { "id": int(note.id), "title": note.title, "user_id": note.user_id, + # The name the line shows, and whether this session has + # it already — carried as data for the reason `kind` is + # (#4364): the line is built from facts, not from + # re-reading its own marker. + "name": _menu_name(note.title, note.note_type, note.data, note.body), + "seen": int(note.id) in excluded, # Carried, not re-read off the rendered marker. The # marker is prose assembled for a human and it already # varies by kind, language and the `seen` flag — a @@ -2399,8 +2469,9 @@ async def build_write_path_hint( "`get_lesson(id)` for a lesson, `get_note(id)` otherwise. Reuse a " "snippet rather than writing a fresh one-off; read an issue before " "repeating what it records " - "(titles only; a line marked `seen` was surfaced earlier this " - "session and may no longer be in context):" + "(each line is a record's name and kind, with the passage that " + "matched; a line marked `seen` is a pointer to one already shown " + "this session, so it is in your context):" ) # The same clause the prompt menu carries, on the same condition and for # the same reason: this menu's three other kinds are all things that WERE @@ -2434,10 +2505,16 @@ async def build_write_path_hint( # is no matching passage and the body's opening would be a fabricated # reason. Absence here is meaningful: a line with no passage under it is # one that earned its place by where it lives, not by what it says. - passage = (wp_chunks.get(int(item["id"])) or {}).get("text") or "" - if passage.strip(): - short, _cut = elide(" ".join(passage.split()), _MENU_PASSAGE_CHARS) - lines.append(f"> ↳ {short}") + # WHOLE, and only on first sight (#4364): a `seen` line is a pointer to + # a record already in context, and its passage is already there too. + if item.get("seen"): + continue + passage = _menu_passage( + item.get("title"), (wp_chunks.get(int(item["id"])) or {}).get("text"), + item.get("name") or "", + ) + if passage: + lines.append(f"> ↳ {passage}") if stamped: lines.append(_stamp_line(path, stamped)) diff --git a/src/scribe/services/systems.py b/src/scribe/services/systems.py index 7f2f0bc..293cd7a 100644 --- a/src/scribe/services/systems.py +++ b/src/scribe/services/systems.py @@ -310,6 +310,39 @@ async def list_record_systems(user_id: int, note_id: int) -> list[System]: return list(result.scalars().all()) +async def system_names_for(note_ids: set[int]) -> dict[int, list[str]]: + """{note_id: [system name, …]} in one query, for records ALREADY read. + + For decorating a result set the caller was allowed to see — an injected + menu line says which part of the project a record is about, so the reader + can place it without opening it (#4364). No access check here for that + reason: the ids come from a search that applied one, and a system name is + metadata of the record, not a record of its own. + + Fails soft, like `access.owner_names_for`: a menu without its system labels + is a cosmetic downgrade, and failing the whole injection over one is not. + """ + if not note_ids: + return {} + try: + async with async_session() as session: + rows = ( + await session.execute( + select(RecordSystem.note_id, System.name) + .join(System, System.id == RecordSystem.system_id) + .where(RecordSystem.note_id.in_(note_ids), System.deleted_at.is_(None)) + .order_by(System.order_index.asc(), System.name.asc()) + ) + ).all() + except Exception: + logger.warning("System-name lookup failed; menu lines go unlabelled", exc_info=True) + return {} + out: dict[int, list[str]] = {} + for note_id, name in rows: + out.setdefault(int(note_id), []).append(name) + return out + + async def list_records_for_system( user_id: int, system_id: int, kind: str | None = None, open_only: bool = False ) -> list[Note]: diff --git a/tests/conftest.py b/tests/conftest.py index 3058aa3..79c6b29 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -102,6 +102,20 @@ def _no_supersession(): yield +@pytest.fixture(autouse=True) +def _no_system_labels(): + """Stub the menu's "which System is each line about?" lookup (#4364). + + Autouse because every test that renders an injected menu reaches it, and + it is a real database call on a path those tests run without one. Stubbed + to "no labels", the state of any untagged record. Tests of the label + itself patch it with a value. + """ + with patch("scribe.services.plugin_context.system_names_for", + AsyncMock(return_value={})): + yield + + @pytest.fixture(autouse=True) def _no_task_log_arm(): """Stub the task-log read arm that get_task / list_tasks / get_milestone diff --git a/tests/test_autoinject_context.py b/tests/test_autoinject_context.py index ba21b30..63dab32 100644 --- a/tests/test_autoinject_context.py +++ b/tests/test_autoinject_context.py @@ -12,13 +12,18 @@ from __future__ import annotations import json import subprocess from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest from scribe.services.plugin_context import ( _AUTOINJECT_CONTEXT_MAX, _AUTOINJECT_CONTEXT_PROMPT_MAX, _autoinject_query, + _menu_name, + _menu_passage, ) -from tests.helpers import need_tools +from tests.helpers import fake_note, need_tools DEFS = Path(__file__).resolve().parents[1] / "plugin" / "hooks" / "scribe_defs.sh" @@ -96,3 +101,90 @@ def test_the_hook_caps_what_it_sends(tmp_path): def test_no_transcript_is_silence_not_failure(tmp_path): assert recent(tmp_path, []) == "" + + +# --- what a menu line carries (#4364) ---------------------------------------- +# +# The name, the kind and System, and the WHOLE matched passage — once. A repeat +# is a pointer. These pin the shape against the three ways it had gone wrong: +# a trigger-composed title rendered as the line (1,500+ chars), the passage cut +# to its head (which was the title again), and a `seen` repeat re-rendered whole. + +TRIGGER = "Adding a record type that is semantically searchable. " * 20 + + +def test_a_snippet_line_shows_its_name_not_its_trigger(): + assert _menu_name(f"embed_x — {TRIGGER}", "snippet", {"name": "embed_x"}) == "embed_x" + # With no mirror, the first separator is the seam (snippets.py's inverse). + assert _menu_name(f"embed_x — {TRIGGER}", "snippet", None) == "embed_x" + + +def test_a_lesson_line_shows_its_subject_not_its_trigger(): + title = f"A guard does not undo a stored value — {TRIGGER.strip()}" + name = _menu_name(title, "lesson", {"when_to_apply": TRIGGER.strip()}) + assert name == "A guard does not undo a stored value" + + +def test_a_plain_note_keeps_its_title_dashes_and_all(): + t = "Dev-log 2026-07-29 — milestone #232 closed" + assert _menu_name(t, "note", None) == t + + +def test_the_passage_is_the_whole_chunk_without_its_title_prefix(): + body = "section " * 150 # ~1.2 KB: nothing of it is cut + out = _menu_passage("Pool sizing", f"Pool sizing\n{body}\nsecond line") + assert not out.startswith("Pool sizing") + assert out.endswith("second line") and "\n" not in out + assert len(out) > 1100 + + +def test_a_title_only_match_hands_over_the_trigger_it_matched_on(): + title = "embed_x — when adding a searchable record" + assert _menu_passage(title, title, "embed_x") == "when adding a searchable record" + + +async def _menu(hits, seen, chunks, systems=None): + from scribe.services import plugin_context as pc + + calls: list[int] = [] + + async def _search(*_a, **kw): + calls.append(1) + if len(calls) > 1: + return [] + if kw.get("report") is not None: + kw["report"]["best_chunk"] = chunks + return hits + + with patch.object(pc, "get_autoinject_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.55, "top_k": 3})), \ + patch.object(pc, "semantic_search_notes", _search), \ + patch.object(pc, "superseded_ids", AsyncMock(return_value=set())), \ + patch.object(pc, "system_names_for", AsyncMock(return_value=systems or {})), \ + patch.object(pc, "record_retrieval", MagicMock()), \ + patch.object(pc, "record_surfaced", MagicMock()): + return (await pc.build_autoinject_hint(1, "q", project_id=2, exclude_ids=seen))["context"] + + +@pytest.mark.asyncio +async def test_a_first_sighting_carries_name_system_and_passage(): + title = f"embed_x — {TRIGGER}" + hits = [(0.8, fake_note(id=11, title=title, note_type="snippet", + data={"name": "embed_x"}, user_id=1))] + out = await _menu(hits, [], {11: {"index": 1, "text": f"{title}\nthe matched section"}}, + systems={11: ["Retrieval & recall"]}) + line = next(ln for ln in out.splitlines() if "#11" in ln) + assert '[snippet · Retrieval & recall] "embed_x"' in line + assert TRIGGER[:40] not in line + assert "> ↳ the matched section" in out + + +@pytest.mark.asyncio +async def test_a_seen_record_is_a_pointer_not_a_copy(): + title = f"embed_x — {TRIGGER}" + hits = [(0.8, fake_note(id=11, title=title, note_type="snippet", + data={"name": "embed_x"}, user_id=1))] + out = await _menu(hits, [11], {11: {"index": 1, "text": f"{title}\nthe matched section"}}) + line = next(ln for ln in out.splitlines() if "#11" in ln) + assert line == "> - #11 [snippet · seen] embed_x" + assert "↳" not in out