From 253fb974f34aa46beb172d495d4360d757f9cd4f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 09:44:10 -0400 Subject: [PATCH] feat(retrieval): every semantic search hands on the passage that matched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4243 fixed one door. Scribe has three semantic searches over three chunk tables, and all three collapsed chunk rows to the best one per record — each of them KNEW which passage earned the hit, and each dropped it. Every surface downstream then previewed the head of the document instead: a span the search had already scored lower, with nothing saying so. Mechanism, one place: - embeddings.record_best_chunk publishes {id: {index, text}} into `report`. Carried in `report`, NOT the return value: all three return list[tuple[float, Record]] and ~30 sites unpack that pair (lesson #4207). - semantic_search_rules and semantic_search_milestones now select chunk_index/chunk_text and publish the winner, as notes already did. semantic_search_milestones gains `report`, which it had no way to take. - services/text.matched_excerpt is the one choice of span, and excerpt_fields the one result block. Doors keep their own field names — the web renders `snippet`, MCP returns `excerpt` — because renaming a field a frontend reads is a different change from fixing what goes in it. Surfaces: - knowledge.query_knowledge, whose own comment calls it "the human's MAIN search surface", was `(note.body or "")[:200]` on every row alike. Now the matched passage on a search, the opening on a browse, and `snippet_is` saying which. KnowledgeView renders that snippet, so this was live. - search(content_type='milestone') gains `matched` — the plan body stays out, but the passage that matched comes along, because recognising a plan means recognising the part you asked about and a description written at the start need not mention it. - The auto-inject menu and the write-path prior-art menu put the passage under their line. Both were title-only, which answers "does this apply?" for a lesson or snippet (the trigger is IN the title) and not at all for an issue or dev-log. No fallback to the body's opening: on a menu that is preamble dressed as a reason, and once indented it cannot be told apart. Left alone deliberately: the rule arms. A rule hint already renders the rule's TRIGGER, which is written to answer exactly "does this apply to me" and beats a matched chunk at it; and that line's budget was measured at #3851. Adding a passage there would duplicate the trigger and spend the budget twice. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- frontend/src/views/KnowledgeView.vue | 35 +++++- src/scribe/mcp/tools/search.py | 68 +++++----- src/scribe/services/embeddings.py | 80 ++++++++++-- src/scribe/services/knowledge.py | 32 ++++- src/scribe/services/plugin_context.py | 48 +++++++ src/scribe/services/text.py | 63 ++++++++++ tests/test_matched_passage_surfaces.py | 166 +++++++++++++++++++++++++ tests/test_services_plugin_context.py | 86 ++++++++++++- 8 files changed, 527 insertions(+), 51 deletions(-) create mode 100644 tests/test_matched_passage_surfaces.py diff --git a/frontend/src/views/KnowledgeView.vue b/frontend/src/views/KnowledgeView.vue index 81932ec..4c93381 100644 --- a/frontend/src/views/KnowledgeView.vue +++ b/frontend/src/views/KnowledgeView.vue @@ -29,7 +29,17 @@ interface KnowledgeItem { id: number; note_type: "note" | "task" | "process" | "snippet" | "lesson"; title: string; + /** ONE SPAN of the record, never the whole thing. On a search this is the + * passage that actually matched; on a plain browse it is the opening, + * because nothing was matched and no span is better than another. */ snippet: string; + /** Which span `snippet` holds. A reader who cannot tell the matched passage + * from the document's first paragraph cannot tell whether a card that looks + * unrelated really is. */ + snippet_is?: "matched_passage" | "body_opening"; + /** Characters in the whole record, so a long record shown by a short span is + * visible as one. */ + body_length?: number; tags: string[]; project_id: number | null; created_at: string; @@ -181,6 +191,16 @@ const CONTENT_PAGE = 24; // items loaded per sentinel trigger const REFILL_THRESHOLD = 48; // fetch more IDs when queue drops below this const items = ref([]); + +// True only when a search actually returned matched passages — not merely when +// the box has text in it. A keyword-only result set, or an embedder that is +// down, carries `body_opening` rows, and claiming otherwise would be the same +// species of lie this whole change is about (#4243). +const showsMatchedPassages = computed( + () => + searchQuery.value.trim().length > 0 && + items.value.some((i) => i.snippet_is === "matched_passage"), +); const allTags = ref([]); const idQueue = ref([]); // unloaded IDs ready to be content-fetched const idOffset = ref(0); // next offset for ID batch requests @@ -573,8 +593,16 @@ onUnmounted(() => {

Your story is unwritten. Create your first note to begin.

+ +

+ Excerpts below are the passage that matched your search, not the start of each record. +

+ -
+
{ line-height: 1.45; margin: 0; } +.k-excerpt-note { + font-size: 0.8rem; + color: var(--fs-text-tertiary); + margin: 0 0 var(--fs-space-3); +} .k-card-footer { display: flex; align-items: center; diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 027f397..f22b2eb 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -11,7 +11,7 @@ import time from scribe.mcp._context import current_user_id from scribe.services.access import owner_names_for -from scribe.services.text import elide +from scribe.services.text import MATCHED_PASSAGE, excerpt_fields from scribe.services.embeddings import ( DEFAULT_SIMILARITY_THRESHOLD, semantic_search_milestones, semantic_search_notes, semantic_search_rules, @@ -20,6 +20,14 @@ from scribe.services import rulebooks as rulebooks_svc from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary +# A matched chunk is at most _CHUNK_CHAR_BUDGET (1400) characters, and it is +# the evidence the ranking was built on — so it is worth more room than the 240 +# characters of document opening this used to send. Elision inside a chunk is +# far less lossy than a head cut of a whole record: the region is already the +# right one. +_EXCERPT_CHARS = 1000 + + async def _search_rules(uid: int, q: str, limit: int, project_id: int) -> dict: """Rules by meaning — a separate result shape because a rule IS different. @@ -74,10 +82,19 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d Its own result shape, like rules: a milestone is a plan with progress, not a note with a body. The plan itself is left out — get_milestone reads it — - because a search hit is for recognising a plan, and bodies run long. + because a search hit is for recognising a plan, and bodies run long. What + does come along is `matched`: the one passage of the body the query + actually hit, with `matched_is` saying whether it is that passage or + merely the body's opening. Recognising a plan means recognising the part + of it you were asking about, and a description written at the start need + not mention that part (#4243). Not part of content_type="all", whose results are note-shaped. """ - raw = await semantic_search_milestones(uid, q, project_id=project_id or None, limit=limit) + report: dict = {} + raw = await semantic_search_milestones( + uid, q, project_id=project_id or None, limit=limit, report=report, + ) + chunks = report.get("best_chunk") or {} progress: dict[int, dict] = {} if raw: from scribe.services import milestones as milestones_svc @@ -91,6 +108,14 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d "id": m.id, "title": m.title, "description": m.description or "", + # The plan body stays out — get_milestone reads it — but the + # passage that MATCHED comes along, because a plan is + # recognised by the part of it the query was about and a + # description need not mention that part at all (#4243). + **excerpt_fields( + m.body or "", chunks.get(int(m.id)), _EXCERPT_CHARS, + key="matched", + ), "status": m.status, "project_id": m.project_id, "total": progress.get(m.id, {}).get("total", 0), @@ -103,14 +128,6 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d } -# A matched chunk is at most _CHUNK_CHAR_BUDGET (1400) characters, and it is -# the evidence the ranking was built on — so it is worth more room than the 240 -# characters of document opening this used to send. Elision inside a chunk is -# far less lossy than a head cut of a whole record: the region is already the -# right one. -_EXCERPT_CHARS = 1000 - - def result_excerpt(note, chunk: dict | None) -> dict: """The part of a record a caller judges "should I open this?" on. @@ -125,30 +142,13 @@ def result_excerpt(note, chunk: dict | None) -> dict: first, which the search had already judged less relevant, and the caller would decide from that and never know (#4243). - So: show the matched passage when there is one, the body when it fits, and - in either case SAY which of the two this is. A caller that cannot tell an - excerpt from a whole record cannot tell whether looking deeper is worth it, - which is the only decision this field supports. + The choice of span lives in services/text.py, shared with the web's + knowledge search, so the two doors cannot drift on which text a reader is + shown or on whether they are told what it is. """ - body = note.body or "" - matched = (chunk or {}).get("text") or "" - out: dict = {"body_length": len(body)} - - if matched.strip(): - text, cut = elide(matched.strip(), _EXCERPT_CHARS) - out["excerpt"] = text - out["excerpt_is"] = "matched_passage" - if (chunk or {}).get("index") is not None: - out["chunk_index"] = int(chunk["index"]) - else: - # No stored chunk — an un-embedded record, or a caller that passed no - # report. Fall back to the opening, and name it as the opening rather - # than letting it pass for the relevant part. - text, cut = elide(body, _EXCERPT_CHARS) - out["excerpt"] = text - out["excerpt_is"] = "body_opening" - if cut or (out["excerpt_is"] == "matched_passage" and len(matched) < len(body)): - out["read_full"] = "get_note / get_task by id for the whole record." + out = excerpt_fields(note.body or "", chunk, _EXCERPT_CHARS) + if out.get("excerpt_is") == MATCHED_PASSAGE and (chunk or {}).get("index") is not None: + out["chunk_index"] = int(chunk["index"]) return out diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 6ac135e..d706a38 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -577,6 +577,30 @@ GLOBAL_NOTE_TYPES: tuple[str, ...] = ("lesson",) # write telemetry pass a dict and read `best_available_score` back out of it. +def record_best_chunk(report: dict | None, chunks: dict[int, dict]) -> None: + """Publish the winning chunk per record into `report["best_chunk"]`. + + Every semantic search here collapses several chunk rows to the best one per + record, which means each of them KNOWS which passage earned the hit — and + each of them used to drop it, leaving every caller to preview the head of + the document instead. The head is a different span, one the search has + already scored lower, and nothing in the result said so (#4243). + + It rides in `report` rather than in the return value because all three + searches return `list[tuple[float, Record]]` and roughly thirty sites + unpack that pair; widening it would be an interface change to every one of + them with nothing to catch a miss (lesson #4207). `report` is already the + side-channel these functions use for `searched` and `best_available_score`, + so this adds a key to a channel callers already open. + + Shape: {record_id: {"index": int, "text": str}}. A caller that passed no + report simply doesn't get it, and every consumer falls back to the body. + """ + if report is None: + return + report["best_chunk"] = chunks + + async def semantic_search_notes( user_id: int, query: str, @@ -823,12 +847,11 @@ async def semantic_search_notes( final = await _apply_supersession_penalty(scored, limit) # Only for what actually came back, so a caller can key straight off the # results without carrying chunks for records it never saw. - if report is not None: - report["best_chunk"] = { - int(n.id): best_chunk[int(n.id)] - for _s, n in final - if int(n.id) in best_chunk - } + record_best_chunk(report, { + int(n.id): best_chunk[int(n.id)] + for _s, n in final + if int(n.id) in best_chunk + }) return final @@ -1075,7 +1098,12 @@ async def semantic_search_rules( async with async_session() as session: rows = (await session.execute( - select(Rule, distance.label("distance")) + select( + Rule, + distance.label("distance"), + RuleEmbedding.chunk_index, + RuleEmbedding.chunk_text, + ) .select_from(RuleEmbedding) .join(Rule, RuleEmbedding.rule_id == Rule.id) .outerjoin(RulebookTopic, Rule.topic_id == RulebookTopic.id) @@ -1098,11 +1126,22 @@ async def semantic_search_rules( return [] best: dict[int, tuple[float, object]] = {} - for rule, dist in rows: + # Which chunk won, kept beside the score it won with — a rule's `why` and + # `how_to_apply` are long, and a caller shown only the head cannot see the + # clause that actually matched (#4243). + won: dict[int, dict] = {} + for rule, dist, chunk_index, chunk_text in rows: score = 1.0 - float(dist) if rule.id not in best or score > best[rule.id][0]: best[rule.id] = (score, rule) + won[int(rule.id)] = { + "index": int(chunk_index), "text": chunk_text or "", + } ranked = sorted(best.values(), key=lambda pair: pair[0], reverse=True) + kept = [pair for pair in ranked if pair[0] >= threshold][:limit] + record_best_chunk(report, { + int(r.id): won[int(r.id)] for _s, r in kept if int(r.id) in won + }) if report is not None: # See the sibling search: absent means the search never ran (#3765). report["searched"] = True @@ -1110,7 +1149,7 @@ async def semantic_search_rules( best = ranked[0] if ranked else None report["best_available_score"] = best[0] if best else None report["best_available_id"] = int(best[1].id) if best else None - return [pair for pair in ranked if pair[0] >= threshold][:limit] + return kept async def backfill_rule_embeddings() -> None: @@ -1218,6 +1257,7 @@ async def semantic_search_milestones( status: str | None = None, limit: int = 5, threshold: float = _SIMILARITY_THRESHOLD, + report: dict | None = None, ) -> list[tuple[float, "Milestone"]]: """Return up to *limit* (score, milestone) pairs most like *query*. @@ -1254,7 +1294,12 @@ async def semantic_search_milestones( scope = Milestone.user_id == user_id async with async_session() as session: rows = (await session.execute( - select(Milestone, distance.label("distance")) + select( + Milestone, + distance.label("distance"), + MilestoneEmbedding.chunk_index, + MilestoneEmbedding.chunk_text, + ) .select_from(MilestoneEmbedding) .join(Milestone, MilestoneEmbedding.milestone_id == Milestone.id) .where( @@ -1270,12 +1315,23 @@ async def semantic_search_milestones( return [] best: dict[int, tuple[float, object]] = {} - for milestone, dist in rows: + # A milestone's `body` IS the plan, and search results show its short + # `description` — so a match on the design was previewed by a sentence that + # need not mention it. The winning chunk is what the caller should see. + won: dict[int, dict] = {} + for milestone, dist, chunk_index, chunk_text in rows: score = 1.0 - float(dist) if milestone.id not in best or score > best[milestone.id][0]: best[milestone.id] = (score, milestone) + won[int(milestone.id)] = { + "index": int(chunk_index), "text": chunk_text or "", + } ranked = sorted(best.values(), key=lambda pair: pair[0], reverse=True) - return [pair for pair in ranked if pair[0] >= threshold][:limit] + kept = [pair for pair in ranked if pair[0] >= threshold][:limit] + record_best_chunk(report, { + int(m.id): won[int(m.id)] for _s, m in kept if int(m.id) in won + }) + return kept async def backfill_milestone_embeddings() -> None: diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index dd7959d..8e053c9 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -25,6 +25,7 @@ from scribe.models import async_session from scribe.models.note import Note from scribe.models.base import iso from scribe.services.access import browsable_notes_clause, readable_notes_clause +from scribe.services.text import excerpt_fields logger = logging.getLogger(__name__) @@ -211,12 +212,30 @@ def _verification_clause(value: str): ) -def _note_to_item(note: Note) -> dict: +def _note_to_item(note: Note, chunks: dict[int, dict] | None = None) -> dict: + """One row for a browse or search listing. + + `chunks` is `report["best_chunk"]` from a semantic search, when this row + came from one. With it the card shows the passage that MATCHED; without it + — a plain listing, where nothing was matched and so no span is better than + any other — it shows the record's opening. `snippet_is` says which, on + every row, so the two never have to be told apart by guessing. + + This was `(note.body or "")[:_SNIPPET_LEN]` on every row alike: the head of + the document, on the human's main search surface, with nothing marking the + cut. A record matched on its sixth paragraph was shown its first, which the + search had already scored lower (#4243). + """ item: dict = { "id": note.id, "note_type": note.note_type or "note", "title": note.title, - "snippet": (note.body or "")[:_SNIPPET_LEN], + **excerpt_fields( + note.body or "", + (chunks or {}).get(int(note.id)), + _SNIPPET_LEN, + key="snippet", + ), "tags": note.tags or [], "project_id": note.project_id, # These lists now include records shared with the caller, so the client @@ -507,6 +526,9 @@ async def _semantic_knowledge_search( # record would be findable by wording and invisible by meaning — which is the # case a semantic search exists to serve. semantic_notes: list[Note] = [] + # Filled by the search below; stays empty when the embedder is down or the + # call raises, in which case every row falls back to its opening. + _semantic_report: dict = {} try: from scribe.services.embeddings import ( INTERACTIVE_SEARCH_THRESHOLD, @@ -519,6 +541,7 @@ async def _semantic_knowledge_search( user_id=user_id, scope="read", query=q, + report=_semantic_report, limit=min(200, limit * 4), # The shared interactive floor — this was a bare `0.3` while # routes/search.py had the same number as a commented constant, the @@ -571,7 +594,10 @@ async def _semantic_knowledge_search( total = len(merged) page_items = merged[offset: offset + limit] - return [_note_to_item(n) for n in page_items], total + return [ + _note_to_item(n, _semantic_report.get("best_chunk")) + for n in page_items + ], total async def get_knowledge_tags(user_id: int, note_type: str | None = None) -> list[str]: diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 5f7ed2b..43bc94c 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -40,12 +40,31 @@ from scribe.services.retrieval_surfaces import ( ) from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.settings import get_setting +from scribe.services.text import elide 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. +# +# 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. +# +# 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 + # Max chars of a Process body to fold into the auto-surface description. _PROC_PREVIEW_CHARS = 200 @@ -1095,6 +1114,10 @@ async def build_autoinject_hint( # 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]) + # From THIS arm's own search (`_rep_ai`), so a chunk is only ever paired + # with the query that actually matched it. + menu_chunks = _rep_ai.get("best_chunk") or {} + note_ids: list[int] = [] for score, note in kept: note_ids.append(int(note.id)) @@ -1113,6 +1136,16 @@ async def build_autoinject_hint( 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}") # Records what SURVIVED the margin gate, not what the ranker returned — the # menu the agent actually saw. retrieval_logs already holds the full @@ -2035,6 +2068,11 @@ async def build_write_path_hint( # floor. Applying the floor after this would throw away the best queries. if query: query = concept_query(query) or query + # Declared out here because the search below is conditional — this arm runs + # only when the menu has room AND a query survived the floor. The render + # loop is not conditional, so it needs something to read either way, and an + # empty mapping means every line falls back to its title alone. + wp_chunks: dict[int, dict] = {} if remaining > 0 and query: t0 = time.perf_counter() # Pulled-and-already-listed ids stay in the query (as evidence for @@ -2089,6 +2127,7 @@ async def build_write_path_hint( scope="browse", report=_rep_wp, ) + wp_chunks = _rep_wp.get("best_chunk") or {} resembles = { int(note.id): float(score) for score, note in hits if int(note.id) in pulled @@ -2337,6 +2376,15 @@ async def build_write_path_hint( for item, marker, owner, foreign_lang in rendered: note_ids.append(int(item["id"])) lines.append(_prior_art_line(item, marker, owner, foreign_lang)) + # Only the semantically-matched lines carry a passage. The records-this- + # file lines came from a LOCATION lookup — nothing was matched, so there + # 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}") if stamped: lines.append(_stamp_line(path, stamped)) diff --git a/src/scribe/services/text.py b/src/scribe/services/text.py index f966b89..0d2ce3c 100644 --- a/src/scribe/services/text.py +++ b/src/scribe/services/text.py @@ -36,3 +36,66 @@ def elide(text: str, budget: int) -> tuple[str, bool]: head = text[:head_len].rstrip() tail = text[-tail_len:].lstrip() return f"{head}\n\n[… {omitted} characters omitted …]\n\n{tail}", True + + +# What a search result shows a reader, as one decision made in one place. +# +# Every door onto a semantic search faces the same question — which span of a +# record do I show someone deciding whether to open it? — and each used to +# answer it separately with a bare head cut of a different length: 240 +# characters in the MCP search, 200 in the web's knowledge search. Both showed +# the document's OPENING, which is not the span that matched and not the span +# the ranking was built on. +# +# Doors keep their own field names (the web UI renders `snippet`, the MCP +# surface returns `excerpt`), because renaming a field a frontend consumes is +# a separate change from fixing which text goes in it. What they share is this +# function: the choice of span, and the obligation to say which span it is. + +MATCHED_PASSAGE = "matched_passage" +BODY_OPENING = "body_opening" + + +def matched_excerpt( + body: str, chunk: dict | None, budget: int +) -> tuple[str, str, bool]: + """Pick the span to show, and say which span it is. + + `chunk` is a `report["best_chunk"]` entry from one of the semantic + searches — `{"index": int, "text": str}` — or None when the caller ran no + semantic search, passed no report, or the record has no embedding row. + + Returns `(text, kind, was_cut)` where `kind` is MATCHED_PASSAGE or + BODY_OPENING. The kind is not decoration: a reader who cannot tell the + passage that earned the hit from the first paragraph of the document + cannot tell whether a thin-looking result is genuinely thin, and the whole + point of the excerpt is to support exactly that judgement. + + A short record comes back whole either way — fragmenting a 200-character + note serves nobody, and its opening IS its content. + """ + passage = (chunk or {}).get("text") or "" + if passage.strip(): + text, cut = elide(passage.strip(), budget) + return text, MATCHED_PASSAGE, cut + text, cut = elide(body or "", budget) + return text, BODY_OPENING, cut + + +def excerpt_fields( + body: str, chunk: dict | None, budget: int, *, key: str = "excerpt" +) -> dict: + """`matched_excerpt` as the block a result row carries. + + `key` names the text field so a door can keep the name its consumers + already read. The companion keys are derived from it, so a row never ends + up with an excerpt under one name and its label under another. + """ + text, kind, cut = matched_excerpt(body, chunk, budget) + out = {key: text, f"{key}_is": kind, "body_length": len(body or "")} + if cut or (kind == MATCHED_PASSAGE and len(text) < len(body or "")): + out["read_full"] = ( + "This is one span of a longer record. Open it by id for the whole " + "thing rather than judging from what is here." + ) + return out diff --git a/tests/test_matched_passage_surfaces.py b/tests/test_matched_passage_surfaces.py new file mode 100644 index 0000000..cf628f8 --- /dev/null +++ b/tests/test_matched_passage_surfaces.py @@ -0,0 +1,166 @@ +"""Every semantic search hands on the passage that matched (#4243, #4250). + +Three searches collapse chunk rows to the best one per record, so each of them +KNOWS which passage earned the hit. Each used to drop it, leaving every door to +preview the head of the document instead — a span the search had already scored +lower, with nothing saying so. + +These pin the mechanism (`report["best_chunk"]` from all three searches) and +each surface that reads it, because the failure mode is silent: a door that +quietly reverts to the body's opening still returns a plausible-looking string +and no test that only checks "a preview exists" would notice. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import embeddings as emb +from scribe.services.text import ( + BODY_OPENING, + MATCHED_PASSAGE, + excerpt_fields, + matched_excerpt, +) +from tests.helpers import make_mock_session + + +def _searching(rows): + """A patched session whose one query returns `rows` — the shared + make_mock_session (#2834) rather than a third local copy of the + __aenter__/__aexit__ dance.""" + session = make_mock_session() + result = MagicMock() + result.all.return_value = rows + session.execute = AsyncMock(return_value=result) + return session + + +# --------------------------------------------------------------------------- +# The shared choice of span +# --------------------------------------------------------------------------- + +def test_the_matched_passage_wins_over_the_opening(): + body = "An opening about nothing much. " * 20 + " THE ANSWER." + text, kind, _cut = matched_excerpt(body, {"index": 4, "text": "THE ANSWER."}, 1000) + assert text == "THE ANSWER." + assert kind == MATCHED_PASSAGE + + +def test_without_a_chunk_the_opening_is_named_as_the_opening(): + """The fallback is legitimate — a plain listing matched nothing — but it + must not pass for the passage that matched.""" + text, kind, _cut = matched_excerpt("just a body", None, 1000) + assert kind == BODY_OPENING + + +def test_an_empty_chunk_is_not_mistaken_for_a_passage(): + """A record embedded from its title alone stores an empty body chunk; + rendering that as "the passage that matched" would be a blank line + presented as evidence.""" + _t, kind, _c = matched_excerpt("real body", {"index": 0, "text": " "}, 1000) + assert kind == BODY_OPENING + + +def test_the_field_names_travel_together(): + """A door renames the text field to keep its consumers working; the label + has to follow it, or a row carries an excerpt under one name and its + meaning under another.""" + out = excerpt_fields("b" * 500, {"index": 1, "text": "hit"}, 100, key="snippet") + assert out["snippet"] == "hit" + assert out["snippet_is"] == MATCHED_PASSAGE + assert out["body_length"] == 500 + assert "read_full" in out + + +def test_a_record_shown_whole_advertises_nothing_further(): + out = excerpt_fields("short", None, 1000) + assert out["excerpt"] == "short" + assert "read_full" not in out + + +# --------------------------------------------------------------------------- +# All three searches publish the winning chunk +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_rule_search_reports_the_clause_that_matched(): + """A rule's `why` and `how_to_apply` run long. A caller shown only the head + cannot see the clause the query actually hit.""" + r1, r2 = MagicMock(id=1), MagicMock(id=2) + session = _searching([ + (r1, 0.10, 2, "the clause that matched"), + (r2, 0.20, 0, "r2 best"), + (r1, 0.40, 9, "a worse clause of r1"), + ]) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + out = await emb.semantic_search_rules(1, "q", limit=5, threshold=0.0, + report=report) + assert [r.id for _s, r in out] == [1, 2] + assert report["best_chunk"][1] == {"index": 2, "text": "the clause that matched"} + + +@pytest.mark.asyncio +async def test_milestone_search_reports_the_passage_of_the_plan_that_matched(): + """A milestone's body IS the plan and search shows its short description, + which need not mention the part the query was about.""" + m1 = MagicMock(id=7) + session = _searching([(m1, 0.15, 5, "step 6 — the acceptance case")]) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + out = await emb.semantic_search_milestones( + 1, "acceptance", limit=5, threshold=0.0, report=report, + ) + assert [m.id for _s, m in out] == [7] + assert report["best_chunk"][7]["text"] == "step 6 — the acceptance case" + + +@pytest.mark.asyncio +async def test_a_caller_that_passes_no_report_still_works(): + """Every one of these searches fails open by design — a recall aid must + never break the call it serves — and that includes the chunk channel.""" + session = _searching([(MagicMock(id=1), 0.1, 0, "text")]) + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + out = await emb.semantic_search_milestones(1, "q", limit=5, threshold=0.0) + assert len(out) == 1 + + +def test_record_best_chunk_on_no_report_is_a_no_op(): + emb.record_best_chunk(None, {1: {"index": 0, "text": "x"}}) # must not raise + + +# --------------------------------------------------------------------------- +# Only what survived the bar is published +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_chunks_are_published_only_for_results_that_came_back(): + """Keyed off the returned rows, so a caller can look up every result it has + and never holds passages for records it was not shown.""" + keep, drop = MagicMock(id=1), MagicMock(id=2) + session = _searching([ + (keep, 0.10, 0, "kept"), + (drop, 0.95, 0, "below the bar"), + ]) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + await emb.semantic_search_milestones( + 1, "q", limit=5, threshold=0.5, report=report, + ) + assert set(report["best_chunk"]) == {1} diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 0d893be..3fc6a63 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -78,8 +78,12 @@ async def test_build_autoinject_hint_titles_only_with_margin_gate(): assert out["note_ids"] == [11, 22] assert '#11 [note] "Pool sizing decision" (0.80)' in out["context"] assert "#33" not in out["context"] - # Title-first: no body text, ever. + # Title-first when the search reports no matched passage — which is this + # test, whose mock returns bare (score, note) pairs and fills no report. + # A record whose passage IS known gets it on a second line; that is + # test_the_menu_shows_the_passage_that_matched below. assert "get_note(id)" in out["context"] + assert "↳" not in out["context"] # Telemetry fired for BOTH retrievals this path runs: the scored menu and # the reuse-slot query competing against it. The slot's query used to be # the one unlogged retrieval on this path — the hit it displaced was in @@ -552,3 +556,83 @@ async def test_the_config_stand_in_carries_every_key_the_real_one_does(): "a key the real config has and the stand-in does not turns an arm " "into a silent no-op under test" ) + + +# ─── the passage that matched travels onto the menu (#4243, #4250) ─────────── + + +@pytest.mark.asyncio +async def test_the_menu_shows_the_passage_that_matched(): + """A title is a headline. For a lesson or a snippet it carries the trigger + and answers "does this apply to me"; for an issue or a dev-log the reason + this record matched is a sentence somewhere inside it, and the reader was + being handed the one part guaranteed not to say so.""" + from scribe.services import plugin_context as pc + + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1))] + + # Only the FIRST call is the menu's own search; the reuse and lesson slots + # run their own queries afterwards and must not contribute chunks, which is + # also what makes the count assertion below deterministic. `report` is + # optional on this interface, so it is written only when one was passed. + calls: list[int] = [] + + async def _menu_search(*_a, **kw): + calls.append(1) + if len(calls) > 1: + return [] + if kw.get("report") is not None: + kw["report"]["best_chunk"] = { + 11: {"index": 3, "text": "we set max_overflow to 5 after the leak"} + } + 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", _menu_search), \ + patch.object(pc, "record_retrieval", MagicMock()): + out = await pc.build_autoinject_hint(1, "pool", project_id=2) + + assert "we set max_overflow to 5 after the leak" in out["context"] + # Indented under its line, so the menu still reads as a list of records + # rather than a wall of prose. + assert "> ↳ we set max_overflow" in out["context"] + + +@pytest.mark.asyncio +async def test_a_record_with_no_stored_chunk_gets_no_invented_passage(): + """The reserved lesson and reuse slots are fetched by their own queries, so + they are absent from this search's report. Falling back to the body's + opening would put a line of preamble under them dressed as the reason they + matched — and once indented identically, a reader cannot tell the two + apart.""" + from scribe.services import plugin_context as pc + + hits = [(0.80, fake_note(id=11, title="Has a chunk", user_id=1)), + (0.78, fake_note(id=22, title="Has none", user_id=1, + body="A long body whose opening says nothing."))] + + calls: list[int] = [] + + async def _menu_search(*_a, **kw): + calls.append(1) + if len(calls) > 1: + return [] + if kw.get("report") is not None: + kw["report"]["best_chunk"] = { + 11: {"index": 0, "text": "the real reason"} + } + 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", _menu_search), \ + patch.object(pc, "record_retrieval", MagicMock()): + out = await pc.build_autoinject_hint(1, "q", project_id=2) + + assert "the real reason" in out["context"] + assert "A long body whose opening" not in out["context"] + # Exactly one passage line, for the one record that had a passage. + assert out["context"].count("↳") == 1