From fdc07f2a2b5a0df07968069e7d6a028bc5a5435a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 08:50:16 -0400 Subject: [PATCH] fix(search): show the passage that matched, not the opening of the body (#4243) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raised by the operator: are we limiting what comes back by character count, and how do we verify the pertinent part is the part displayed? We were not. mcp/tools/search.py sent (note.body or "")[:240] — a head cut, with no marker that anything had been removed, so a 240-character preview of a 4000-character record was indistinguishable from a complete short one. The opening is the wrong span. The match is semantic and per chunk, and semantic_search_notes collapses to best-chunk-per-note — its own comment at the collapse says "the first appearance of a note is its best chunk". So the system identified the passage that earned the hit and then discarded it: select(Note, distance) kept no chunk column. A record could rank first on its sixth paragraph, be previewed by its first, and be judged irrelevant on a span the search had already scored lower. That biases against long records, and it is self-concealing — the caller who does not open it never learns the preview was misleading. - embeddings: chunk_index/chunk_text ride along in the select, and the collapse records the winner in report["best_chunk"]. Carried in `report`, NOT by widening the return tuple: ten callers unpack (score, note) at ~18 sites and nothing would catch the misses (lesson #4207). `report` is the side-channel this function already uses for best_available_score. - search(): excerpt / excerpt_is / body_length, and read_full when there is more. A caller that cannot tell a matched passage from a document opening cannot judge whether to look deeper, which is the only decision the field supports. elide() moves to services/text.py so both callers share one copy, and it keeps BOTH ends with a stated gap — it is the fallback for when nothing identifies a better span than "all of it", not the goal. Also fixes a guard that produced a false failure on the previous commit: test_pull_telemetry checked `"project_id: int = 0" in body.split("\n")[0]`, which sees only the first line, so wrapping get_task's signature over four lines made it report a function that does take the project as one that does not. Parsed with ast now, and proven to still reject an absent or wrongly-typed parameter rather than being appeased by reflowing the code. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/search.py | 67 +++++++++++++++++++++++++- src/scribe/mcp/tools/tasks.py | 26 +--------- src/scribe/services/embeddings.py | 39 +++++++++++++-- src/scribe/services/text.py | 38 +++++++++++++++ tests/test_mcp_tool_search.py | 75 +++++++++++++++++++++++++++-- tests/test_pull_telemetry.py | 23 ++++++++- tests/test_task_work_log_surface.py | 27 ++++++----- 7 files changed, 245 insertions(+), 50 deletions(-) create mode 100644 src/scribe/services/text.py diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 4623ca9..027f397 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -11,6 +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.embeddings import ( DEFAULT_SIMILARITY_THRESHOLD, semantic_search_milestones, semantic_search_notes, semantic_search_rules, @@ -102,6 +103,55 @@ 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. + + This used to be `(note.body or "")[:240]` — the document's opening, with + no marker that anything had been cut, so a 240-character preview of a + 4000-character record was indistinguishable from a complete short one. + + The opening is the wrong span. The match was semantic and per-chunk, and + `semantic_search_notes` collapses to best-chunk-per-note — so the system + already knows which passage earned the hit and used to discard it. A + record could rank first on its sixth paragraph and be previewed by its + 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. + """ + 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." + return out + + async def search( q: str, content_type: str = "all", @@ -150,9 +200,21 @@ async def search( list_system_records gives the same slice unranked. Returns: - {"results": [{"id", "title", "body", "is_task", "tags", "similarity"}], + {"results": [{"id", "title", "excerpt", "excerpt_is", "body_length", + "is_task", "tags", "similarity"}], "total": int} + `excerpt` is a SPAN of the record, not the record. `excerpt_is` says + which span: "matched_passage" is the chunk that actually earned the + hit — the evidence the ranking was built on, and the right thing to + judge relevance from. "body_opening" is a fallback for a record with + no stored chunk, and is only the beginning of the text, which may say + nothing about why it matched. `body_length` is the whole record's + size, so a long record previewed by a short span is visible as one; + `read_full` appears when there is more, and opening the id by + get_note / get_task is how you get it. Judge from the passage, not + from the fact that a preview looked thin. + A result marked `shared: true` with an `owner` belongs to another user — that person's suggestion, not the operator's own record or settled practice. Weigh it on its merits and say whose it is when you use it. @@ -195,12 +257,13 @@ async def search( owners = await owner_names_for( {int(note.user_id) for _s, note in raw if note.user_id != uid} ) + chunks = report.get("best_chunk") or {} return { "results": [ { "id": note.id, "title": note.title, - "body": (note.body or "")[:240], + **result_excerpt(note, chunks.get(int(note.id))), "is_task": bool(note.is_task), "tags": list(note.tags or []), "similarity": float(score), diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 9562d38..e33e56e 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -45,6 +45,7 @@ from scribe.services import task_logs as task_logs_svc from scribe.services import trash as trash_svc from scribe.services.note_usage import record_pulled from scribe.services.record_refs import refuse_guessed_ids +from scribe.services.text import elide # A work log entry is prose, often long — the discipline asks for what was @@ -64,31 +65,6 @@ _WORK_LOG_ADVICE = ( ) -def elide(text: str, budget: int) -> tuple[str, bool]: - """Cut to `budget` characters from the MIDDLE, keeping both ends. - - A head-only cut — `text[:800]` — decides what a reader sees by character - position, which is uncorrelated with what matters. Prose does not put its - conclusion first: a log entry that opens with what was tried and closes - with "so this shipped in 04775c3" loses exactly the sentence that answers - the question, and the reader cannot tell, because a truncation marker says - that something was removed and never whether it mattered. - - So keep the opening (what this entry is about) AND the closing (where it - landed), and say in between how much went. Two thirds to the head because - that is where the subject is established; the tail needs less to carry a - conclusion. `budget` of 0 means no cut. - """ - if budget <= 0 or len(text) <= budget: - return text, False - head_len = max(1, budget * 2 // 3) - tail_len = max(1, budget - head_len) - omitted = len(text) - head_len - tail_len - head = text[:head_len].rstrip() - tail = text[-tail_len:].lstrip() - return f"{head}\n\n[… {omitted} characters omitted …]\n\n{tail}", True - - def work_log_payload( logs: list, total: int, chars: int, latest_chars: int = _WORK_LOG_LATEST_CHARS ) -> dict: diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index c29baed..6ac135e 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -686,7 +686,16 @@ async def semantic_search_notes( # to the note's owner, so filtering it would pin every scope to "own" # and leave shared records unreachable by meaning. stmt = ( - select(Note, distance.label("distance")) + # chunk_index/chunk_text ride along so the collapse below can + # say WHICH passage matched. Without them the caller is left + # previewing the head of the body — a span this query has + # already determined is not why the record ranked (#4243). + select( + Note, + distance.label("distance"), + NoteEmbedding.chunk_index, + NoteEmbedding.chunk_text, + ) .select_from(NoteEmbedding) .join(Note, NoteEmbedding.note_id == Note.id) .where( @@ -774,11 +783,23 @@ async def semantic_search_notes( # Recover similarity (1 - distance); order stays highest-first. scored: list[tuple[float, Note]] = [] seen: set[int] = set() - for note, dist in rows: + # The winning row IS the best chunk, by the ordering above — so this is the + # one place that knows which passage earned the hit. Kept beside the score + # rather than returned with it: the return type is list[tuple[float, Note]] + # and ten callers unpack it at ~18 sites, so widening the tuple would be an + # interface change to every one of them with nothing to catch the misses + # (lesson #4207). `report` is the side-channel this function already uses + # for best_available_score. + best_chunk: dict[int, dict] = {} + for note, dist, chunk_index, chunk_text in rows: if int(note.id) in seen: continue seen.add(int(note.id)) scored.append((1.0 - float(dist), note)) + best_chunk[int(note.id)] = { + "index": int(chunk_index), + "text": chunk_text or "", + } # The best score anything reached, bar or no bar. Recorded BEFORE the # filter because a call that returns nothing is exactly when it matters. if report is not None: @@ -797,8 +818,18 @@ async def semantic_search_notes( report["best_available_id"] = int(best[1].id) if best else None scored = [pair for pair in scored if pair[0] >= threshold] if not demote_superseded: - return scored[:limit] - return await _apply_supersession_penalty(scored, limit) + final = scored[:limit] + else: + 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 + } + return final async def backfill_note_embeddings() -> None: diff --git a/src/scribe/services/text.py b/src/scribe/services/text.py new file mode 100644 index 0000000..f966b89 --- /dev/null +++ b/src/scribe/services/text.py @@ -0,0 +1,38 @@ +"""Text shortening for surfaces that cannot show a record whole. + +One function, in one place, because both doors shorten and two copies would +drift — and because the reasoning below is the part that matters and should +not have to be re-derived at each call site. +""" + + +def elide(text: str, budget: int) -> tuple[str, bool]: + """Cut to `budget` characters from the MIDDLE, keeping both ends. + + A head-only cut — `text[:800]` — decides what a reader sees by character + position, which is uncorrelated with what matters. Prose does not put its + conclusion first: a passage that opens with what was attempted and closes + with "so this shipped in 04775c3" loses exactly the sentence that answers + the question. Worse, the reader cannot tell: a truncation marker says that + something was removed, never whether it mattered, so the decision "should + I look deeper?" gets made on evidence selected by length. + + So keep the opening (what this is about) AND the closing (where it landed), + and state in between how much went. Two thirds to the head because that is + where the subject is established; a conclusion needs less room to carry. + + Returns (text, was_cut). A `budget` of 0 or less means no cut. + + This is the fallback, not the goal. Where the system knows WHICH span of a + record is the relevant one — a semantic search knows exactly that, and + stores it (#4243) — show that span and say so. Reach for this only when + nothing identifies a better part than "all of it". + """ + if budget <= 0 or len(text) <= budget: + return text, False + head_len = max(1, budget * 2 // 3) + tail_len = max(1, budget - head_len) + omitted = len(text) - head_len - tail_len + head = text[:head_len].rstrip() + tail = text[-tail_len:].lstrip() + return f"{head}\n\n[… {omitted} characters omitted …]\n\n{tail}", True diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index 951becf..138df7b 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -39,23 +39,88 @@ async def test_fable_search_returns_repackaged_results(): r = out["results"][0] assert r["id"] == 1 assert r["title"] == "kafka rebalance" - assert r["body"] == "HPA details" + # The whole body, because it fits — and named as the opening, not as the + # passage that matched, since this call patched the search and so carries + # no chunk. + assert r["excerpt"] == "HPA details" + assert r["excerpt_is"] == "body_opening" + assert r["body_length"] == len("HPA details") assert r["is_task"] is False assert r["tags"] == ["ops"] assert r["similarity"] == pytest.approx(0.93) @pytest.mark.asyncio -async def test_fable_search_body_is_truncated_to_240_chars(): +async def test_search_shows_the_passage_that_matched_not_the_opening(): + """The point of #4243. A record can rank on its sixth paragraph; showing + its first is showing the caller a span the search already judged less + relevant, and letting them decide from it.""" _user_id_ctx.set(7) - long_body = "x" * 500 - fake = fake_note(id=1, title="t", body=long_body) + body = "Chapter one, about nothing. " * 40 + " THE ANSWER IS 04775c3." + fake = fake_note(id=1, title="t", body=body) + + async def _search(*a, **kw): + kw["report"]["best_chunk"] = { + 1: {"index": 3, "text": "THE ANSWER IS 04775c3."} + } + return [(0.5, fake)] + + with patch("scribe.mcp.tools.search.semantic_search_notes", _search): + out = await search(q="answer") + r = out["results"][0] + assert r["excerpt"] == "THE ANSWER IS 04775c3." + assert r["excerpt_is"] == "matched_passage" + assert r["chunk_index"] == 3 + # And the caller is told there is more record behind the passage. + assert r["body_length"] == len(body) + assert "read_full" in r + + +@pytest.mark.asyncio +async def test_a_result_says_whether_its_excerpt_is_the_match_or_the_opening(): + """A caller that cannot tell the two apart cannot judge whether looking + deeper is worth it, which is the only decision this field supports.""" + _user_id_ctx.set(7) + fake = fake_note(id=1, title="t", body="short body") with patch( "scribe.mcp.tools.search.semantic_search_notes", AsyncMock(return_value=[(0.5, fake)]), ): out = await search(q="x") - assert len(out["results"][0]["body"]) == 240 + assert out["results"][0]["excerpt_is"] == "body_opening" + + +@pytest.mark.asyncio +async def test_a_long_excerpt_keeps_both_ends_and_says_how_much_went(): + """The fallback is still an elision, and an elision that drops the tail + drops wherever the conclusion was.""" + _user_id_ctx.set(7) + body = "OPENING. " + ("m" * 3000) + " CLOSING." + fake = fake_note(id=1, title="t", body=body) + with patch( + "scribe.mcp.tools.search.semantic_search_notes", + AsyncMock(return_value=[(0.5, fake)]), + ): + out = await search(q="x") + excerpt = out["results"][0]["excerpt"] + assert excerpt.startswith("OPENING.") + assert excerpt.rstrip().endswith("CLOSING.") + assert "characters omitted" in excerpt + assert out["results"][0]["body_length"] == len(body) + + +@pytest.mark.asyncio +async def test_a_short_record_arrives_whole_and_unmarked(): + """Fragmenting a 200-character note serves nobody.""" + _user_id_ctx.set(7) + fake = fake_note(id=1, title="t", body="all of it") + with patch( + "scribe.mcp.tools.search.semantic_search_notes", + AsyncMock(return_value=[(0.5, fake)]), + ): + out = await search(q="x") + assert out["results"][0]["excerpt"] == "all of it" + assert "read_full" not in out["results"][0] @pytest.mark.asyncio diff --git a/tests/test_pull_telemetry.py b/tests/test_pull_telemetry.py index dfbf399..6ead572 100644 --- a/tests/test_pull_telemetry.py +++ b/tests/test_pull_telemetry.py @@ -224,13 +224,34 @@ def _pulling_getters(): yield module, name, body +def _takes_reading_project(body: str) -> bool: + """Does this function declare `project_id: int = 0`? + + Parsed, not string-matched against the first line. The first version read + `body.split("\n")[0]`, which sees only as far as the first newline — so + adding a parameter to `get_task` wrapped its signature over four lines and + the guard reported a function that DOES take the project as one that does + not. A guard that fails on formatting is a guard that gets appeased by + reflowing the code it was meant to check. + """ + node = ast.parse(body).body[0] + args = node.args + params = list(args.posonlyargs) + list(args.args) + list(args.kwonlyargs) + for arg in params: + if arg.arg != "project_id": + continue + ann = getattr(arg, "annotation", None) + return isinstance(ann, ast.Name) and ann.id == "int" + return False + + def test_every_getter_that_pulls_takes_the_reading_project(): """Asserted on structure (rule 167): a behavioural test cannot see a parameter that was never threaded through.""" missing = [ f"{module}.{name}" for module, name, body in _pulling_getters() - if "project_id: int = 0" not in body.split("\n")[0] + if not _takes_reading_project(body) ] assert not missing, ( f"these getters record a pull but cannot say where the reader was: " diff --git a/tests/test_task_work_log_surface.py b/tests/test_task_work_log_surface.py index 9376173..80d241f 100644 --- a/tests/test_task_work_log_surface.py +++ b/tests/test_task_work_log_surface.py @@ -20,6 +20,15 @@ from scribe.mcp.tools.tasks import ( elide, get_task, list_tasks, work_log_payload, ) from scribe.services.notes import brief_row +# Bound at IMPORT time, which is before any fixture runs — so these names +# keep pointing at the real implementations even though conftest's autouse +# _no_task_log_arm replaces the module attributes for every test. Reaching +# them as `task_logs.logs_for_task` would get the stub and test nothing. +from scribe.services.task_logs import ( + count_logs_for_task, + log_counts_for_tasks, + logs_for_task, +) from tests.helpers import fake_task, make_mock_session pytestmark = pytest.mark.usefixtures("_bind_user") @@ -304,8 +313,6 @@ async def test_logs_for_task_is_not_filtered_by_who_wrote_the_entry(): """`list_logs` filters TaskLog.user_id == user_id, which hands a shared collaborator an empty list that reads as "no work has been done". The work log belongs to the task.""" - from scribe.services import task_logs as svc - session = make_mock_session() session.execute = AsyncMock(return_value=MagicMock( scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))) @@ -314,7 +321,7 @@ async def test_logs_for_task_is_not_filtered_by_who_wrote_the_entry(): AsyncMock(return_value=True)), \ patch("scribe.services.task_logs.async_session", MagicMock(return_value=session)): - await svc.logs_for_task(7, 42) + await logs_for_task(7, 42) stmt = str(session.execute.await_args.args[0]) assert "task_logs.task_id" in stmt assert "task_logs.user_id" not in stmt @@ -324,26 +331,22 @@ async def test_logs_for_task_is_not_filtered_by_who_wrote_the_entry(): async def test_logs_for_task_refuses_a_task_the_caller_cannot_read(): """Unscoped would have been the mirror-image hole: rule #78 is about routing the question through the access layer, in both directions.""" - from scribe.services import task_logs as svc - opened = MagicMock() with patch("scribe.services.task_logs.can_read_note", AsyncMock(return_value=False)), \ patch("scribe.services.task_logs.async_session", opened): - out = await svc.logs_for_task(7, 42) + out = await logs_for_task(7, 42) assert out == [] assert opened.call_count == 0 @pytest.mark.asyncio async def test_count_for_an_unreadable_task_is_zero_not_a_leak(): - from scribe.services import task_logs as svc - opened = MagicMock() with patch("scribe.services.task_logs.can_read_note", AsyncMock(return_value=False)), \ patch("scribe.services.task_logs.async_session", opened): - assert await svc.count_logs_for_task(7, 42) == 0 + assert await count_logs_for_task(7, 42) == 0 assert opened.call_count == 0 @@ -351,15 +354,13 @@ async def test_count_for_an_unreadable_task_is_zero_not_a_leak(): async def test_log_counts_for_tasks_scopes_by_readability_in_the_same_query(): """A per-row can_read_note would be the N+1 this function exists to avoid, so the permission goes in as set membership instead.""" - from scribe.services import task_logs as svc - session = make_mock_session() session.execute = AsyncMock(return_value=MagicMock( all=MagicMock(return_value=[(1, 3)]) )) with patch("scribe.services.task_logs.async_session", MagicMock(return_value=session)): - out = await svc.log_counts_for_tasks(7, [1, 2]) + out = await log_counts_for_tasks(7, [1, 2]) assert out == {1: 3} stmt = str(session.execute.await_args.args[0]) assert "notes" in stmt.lower() @@ -371,5 +372,5 @@ async def test_no_ids_asks_the_database_nothing(): opened = MagicMock() with patch("scribe.services.task_logs.async_session", opened): - assert await svc.log_counts_for_tasks(7, []) == {} + assert await log_counts_for_tasks(7, []) == {} assert opened.call_count == 0