diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 17affb3..1078637 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -140,6 +140,9 @@ async def search( enter_project) — otherwise this searches across ALL projects and bleeds unrelated work into the result set. 0 = search everything (use only when you genuinely want a cross-project sweep). + A LESSON is the exception and arrives whatever the scope: the kind + records an insight that transfers, so it is reachable from a + project it was not written on. system_id: Narrow to records tagged to one System (a named subsystem/area — enter_project lists them). Use when investigating a specific subsystem: it cuts the candidates to records someone @@ -167,6 +170,14 @@ async def search( uid, q, limit=limit, is_task=is_task, project_id=project_id or None, system_id=system_id or None, + # A LESSON is reachable from any project (milestone 385). The kind + # exists to carry an insight to the next project, so a project filter + # that hid it would hide it precisely where it is worth having. Only + # the project filter widens — everything else about the scoping holds, + # and a caller narrowing by `content_type` still gets what it asked + # for. This is the explicit search, where the operator asked; the + # unasked-for arms decide their own budget separately. + include_global_kinds=True, # An explicit search reaches everything the operator may read, including # records shared with them one-to-one. scope="read", diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 7dc11fe..8734ae4 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -508,6 +508,25 @@ async def upsert_note_embedding( logger.warning("Failed to persist embedding for note %d", note_id, exc_info=True) +# Kinds that belong to no single project, and are therefore reachable from a +# project-scoped search of a DIFFERENT project when a caller asks for them +# (milestone 385 step 3). +# +# A lesson is the whole reason this exists. "A better way to think about this +# problem" is not true only where it was learned, and a lesson confined to its +# origin project would be unreachable exactly where it is most useful — on the +# next project, which is the case the kind was created for (#3727). +# `semantic_search_rules` has always had this property: it scopes by OWNERSHIP +# rather than by what binds a given project, because "is there a rule about +# this" is a question asked across a whole rulebook. A lesson asks the same +# kind of question. +# +# Spelled as a literal rather than imported from `services.lessons`, which +# imports `trigger_title` from this module and would close a cycle. A guard +# pins the two equal instead. +GLOBAL_NOTE_TYPES: tuple[str, ...] = ("lesson",) + + # Both searches rank WITHOUT the threshold and apply it in Python, so the best # rejected score stays observable (#3670). The qualifying set is provably # unchanged: rows arrive ordered by distance ascending, so every above-bar row @@ -536,6 +555,7 @@ async def semantic_search_notes( note_type: str | Sequence[str] | None = None, task_kind: str | Sequence[str] | None = None, orphan_only: bool = False, + include_global_kinds: bool = False, scope: str = "own", demote_superseded: bool = True, system_id: int | None = None, @@ -570,6 +590,19 @@ async def semantic_search_notes( 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. + `include_global_kinds` lets a project-scoped search ALSO reach the kinds in + GLOBAL_NOTE_TYPES — records that belong to no single project — so a lesson + written on one project is found from another. It widens the PROJECT filter + only: a caller that also passes `note_type` still gets exactly the kinds it + asked for, so narrowing to snippets does not quietly acquire lessons. + + Off by default, because two callers depend on the project filter holding. + The near-duplicate gate compares a record only against its own project on + purpose, and a globally-visible kind would let a lesson block an unrelated + note's create on a project its author never touched. Ordinary note recall + is project-scoped for the same reason — the point of the carve-out is that + ONE kind escapes, not that scoping is weaker. + `scope` ("own" | "browse" | "read", see access.notes_visibility_clause) decides how far this may see. It exists because this one function serves three different kinds of act: an explicit search, which should reach @@ -631,7 +664,12 @@ async def semantic_search_notes( if orphan_only: stmt = stmt.where(Note.project_id.is_(None)) elif project_id is not None: - stmt = stmt.where(Note.project_id == project_id) + in_project = Note.project_id == project_id + if include_global_kinds: + in_project = or_( + in_project, Note.note_type.in_(GLOBAL_NOTE_TYPES) + ) + stmt = stmt.where(in_project) # Narrow to records tagged to one System (subsystem/area). An # association filter, not a ranking signal — membership in the # candidate set, decided before scoring, like project_id above. diff --git a/src/scribe/services/lessons.py b/src/scribe/services/lessons.py index cf4fd6c..08b496f 100644 --- a/src/scribe/services/lessons.py +++ b/src/scribe/services/lessons.py @@ -136,3 +136,61 @@ def compose_title(what: str, when_to_apply: str = "") -> str: from scribe.services.embeddings import trigger_title return trigger_title(what, when_to_apply) + + +def compose_body(insight: str, when_to_apply: str = "") -> str: + """The lesson body — the trigger line first, the insight after. + + The mirror of `compose_title` on the other half of the document, and the + reason the pair is what makes a lesson findable: `chunk_document` joins + them as `{title}\\n{body}`, so a lesson composed here states WHEN IT + APPLIES in the title and again in the first line of the body. That is the + twice-in-a-short-document shape note #2485 measured as the only sharp one + in the corpus, reached the way a snippet reaches it — by being in the text + — rather than by a second document builder at embed time. + + `**When to apply:**` rather than plain text: the body is the READABLE + form, `data` is the queryable mirror, and `_BODY_TRIGGER_RE` reads this + line back when the mirror is missing. Its markdown must therefore match + what that pattern expects, which is why neither is written by hand + anywhere else. + + The insight goes in the body rather than being held out of the document. + `rule_document` excludes a rule's `why` because long dated narrative made + sixteen dev-logs land on the centroid of "development" — but that finding + predates chunking (#280). A body over the budget is now split into several + chunks, EACH prefixed with the title, so a lesson's story no longer + averages itself into its trigger: it occupies its own vectors, and every + one of them still carries the trigger in its prefix. Holding it out would + cost the reader the only part that explains the insight and would buy a + sharpness the chunker already provides. + """ + lines = [] + trigger = (when_to_apply or "").strip() + if trigger: + lines.append(f"**When to apply:** {trigger}") + insight = (insight or "").strip() + if insight: + lines.append(insight) + return "\n\n".join(lines) + + +def lesson_document( + what: str, when_to_apply: str = "", insight: str = "", +) -> tuple[str, str]: + """The (title, body) a lesson is STORED — and therefore embedded — as. + + One call so the two halves cannot be composed apart. A lesson whose title + carried the trigger and whose body did not would embed as an ordinary + note wearing a label, and nothing would report it: the record would look + right in every listing and simply never be retrieved at the moment it + applies. + + Deliberately returns what is STORED, not a separate embed-time shape. + Rules need `rule_document` because a rule keeps its trigger in a column + and its title is a plain name, so the sharp document has to be synthesised + for the ranker and exists nowhere else. A lesson follows the snippet + instead — the stored record IS the sharp document — which is why nothing + re-embeds and `CHUNKER_VERSION` does not move. + """ + return compose_title(what, when_to_apply), compose_body(insight, when_to_apply) diff --git a/tests/test_integration_lesson_reach.py b/tests/test_integration_lesson_reach.py new file mode 100644 index 0000000..24c7c40 --- /dev/null +++ b/tests/test_integration_lesson_reach.py @@ -0,0 +1,143 @@ +"""A lesson is reachable from a project it was not written on (step 3). + +WHY THIS IS AN INTEGRATION TEST + +The carve-out is one `OR` inside the query's project filter, and what has to be +proved is which ROWS come back — a mock session returns whatever it was told to +and would pass with the predicate inverted. Every note here embeds identically +to the query, so the only thing that can separate them is the scoping: a leak +and a correct result are otherwise indistinguishable. + +The embedder is stubbed, as in the other pgvector tests, so this depends on +Postgres and the query rather than on a downloaded model. No similarity number +is asserted — only membership. +""" +import uuid +from unittest.mock import AsyncMock, patch + +import pytest +import pytest_asyncio + +from scribe.models import async_session +from scribe.models.embedding import EMBEDDING_DIM, NoteEmbedding +from scribe.models.note import Note +from scribe.models.project import Project +from scribe.services import lessons as lessons_svc +from scribe.services.embeddings import CHUNKER_VERSION, semantic_search_notes +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + +QUERY_VEC = [1.0] + [0.0] * (EMBEDDING_DIM - 1) + + +@pytest_asyncio.fixture +async def corpus(): + """A lesson and a plain note on project A, plus a lesson on project B. + + Fresh users per run: every row matches the query equally, so a record left + behind by another test would read exactly like a scoping leak. + """ + tag = uuid.uuid4().hex[:8] + async with async_session() as s: + owner = await ensure_user(s, f"lesson_reach_owner_{tag}") + await s.flush() + a = Project(user_id=owner.id, title="Where it was learned") + b = Project(user_id=owner.id, title="Somewhere else entirely") + s.add_all([a, b]) + await s.flush() + + rows = { + "lesson_on_a": Note( + user_id=owner.id, project_id=a.id, + note_type=lessons_svc.LESSON_NOTE_TYPE, + title="Suspect the guard — a test fails on correct code", + body="**When to apply:** a test fails on correct code", + ), + "note_on_a": Note( + user_id=owner.id, project_id=a.id, note_type="note", + title="An ordinary note", body="ordinary body", + ), + "lesson_on_b": Note( + user_id=owner.id, project_id=b.id, + note_type=lessons_svc.LESSON_NOTE_TYPE, + title="A lesson that lives on B", body="**When to apply:** on B", + ), + } + s.add_all(rows.values()) + await s.flush() + for note in rows.values(): + s.add(NoteEmbedding( + note_id=note.id, chunk_index=0, user_id=owner.id, + embedding=QUERY_VEC, chunk_text=note.title, + chunker_version=CHUNKER_VERSION, + )) + ids = {k: n.id for k, n in rows.items()} + ids["owner"], ids["a"], ids["b"] = owner.id, a.id, b.id + await s.commit() + return ids + + +async def _search(uid, **kw): + with patch( + "scribe.services.embeddings.get_embedding", AsyncMock(return_value=QUERY_VEC) + ): + hits = await semantic_search_notes(uid, "when does this apply", limit=20, **kw) + return {note.id for _score, note in hits} + + +async def test_a_lesson_is_found_from_another_project(corpus): + """THE acceptance this step exists for. Searching project B reaches the + lesson written on project A — the case the kind was created for, because a + transferable insight is most useful on the project that has not learned it + yet.""" + found = await _search( + corpus["owner"], project_id=corpus["b"], include_global_kinds=True, + ) + + assert corpus["lesson_on_a"] in found + assert corpus["lesson_on_b"] in found + + +async def test_an_ordinary_note_stays_where_it_was_written(corpus): + """The other half, and the one that would make this change a bug. Project + scoping is deliberate for every other kind; the carve-out admits ONE kind + rather than weakening the filter.""" + found = await _search( + corpus["owner"], project_id=corpus["b"], include_global_kinds=True, + ) + + assert corpus["note_on_a"] not in found + + +async def test_the_carve_out_is_off_unless_asked_for(corpus): + """Default off, because the near-duplicate gate and ordinary recall both + depend on the project filter holding. A globally visible kind arriving + there would let a lesson block an unrelated note's create on a project its + author never touched.""" + found = await _search(corpus["owner"], project_id=corpus["b"]) + + assert found == {corpus["lesson_on_b"]} + + +async def test_the_home_project_is_unchanged(corpus): + """Searching the project a lesson was written on returns it either way — + the carve-out adds reach, it does not move anything.""" + for flag in (False, True): + found = await _search( + corpus["owner"], project_id=corpus["a"], include_global_kinds=flag, + ) + assert corpus["lesson_on_a"] in found + assert corpus["note_on_a"] in found + + +async def test_a_kind_filter_still_means_what_it_says(corpus): + """The carve-out widens the PROJECT filter only. A caller narrowing to + snippets asked for snippets, and quietly handing it lessons would make + `note_type` mean something different depending on a flag it did not set.""" + found = await _search( + corpus["owner"], project_id=corpus["b"], + include_global_kinds=True, note_type="snippet", + ) + + assert found == set() diff --git a/tests/test_lesson_document_shape.py b/tests/test_lesson_document_shape.py new file mode 100644 index 0000000..b868aad --- /dev/null +++ b/tests/test_lesson_document_shape.py @@ -0,0 +1,139 @@ +"""The document a lesson is embedded as (milestone 385 step 3). + +WHY THIS IS THE STEP THAT DECIDES THE MILESTONE + +Everything before this is storage. A lesson stored with a trigger but embedded +as ordinary prose is a note wearing a label: it would look right in every +listing and simply never be retrieved at the moment it applies, and nothing +anywhere would report that. + +WHY THERE IS NO `lesson_document()` BESIDE `rule_document()` + +The step anticipated one. There isn't, and the difference is where the sharp +shape LIVES rather than whether it exists. + +A rule keeps its trigger in a column and its title is a plain name, so the +`{title} — {trigger}` document has to be synthesised at embed time and exists +nowhere else — that is what `rule_document` is for. A snippet, which note #2485 +measured as the only sharp record in the corpus (a 0.153 top-to-second gap +against 0.010–0.023 for everything else), gets there the other way: its STORED +title is already the join and its stored body already opens with the trigger, +so the ordinary `title\\nbody` join is the sharp document. A lesson follows the +snippet, which is what step 1 decided and step 2 built. + +The consequence worth stating: `chunk_document` is untouched, so +`CHUNKER_VERSION` does not move and nothing re-embeds. The step's "Re-embed" +section describes a change this design does not make. + +These guards therefore assert the composed record, then assert that the generic +chunker turns it into the intended document — the two halves of the same claim. +No similarity number is asserted anywhere: a threshold pins the embedder's +behaviour rather than this code's, and breaks on a model change that is not a +regression. +""" +from __future__ import annotations + +from scribe.services import lessons as lessons_svc +from scribe.services.embeddings import chunk_document, embedding_text + +TRIGGER = "a test fails on code you believe is correct" +SUBJECT = "Suspect the guard before the code" +INSIGHT = "Check whether the assertion still describes the property it was written for." + + +def test_the_trigger_appears_twice_in_the_document(): + """THE guard. Purpose stated twice in a short document is the entire + measured cause of a snippet's sharpness, and it is the one property that + distinguishes a lesson's vector from a plain note's.""" + title, body = lessons_svc.lesson_document(SUBJECT, TRIGGER, INSIGHT) + document = embedding_text(title, body) + + assert document.count(TRIGGER) == 2 + # Once in each half, not twice in one of them. + assert TRIGGER in title + assert TRIGGER in body + + +def test_the_document_leads_with_when_it_applies(): + """The title is `{what} — {when}` and the body's FIRST line restates it, so + the opening of the document is about the situation rather than the topic. + A lesson buried behind a paragraph of narrative would rank on the + narrative.""" + title, body = lessons_svc.lesson_document(SUBJECT, TRIGGER, INSIGHT) + + assert title == f"{SUBJECT} — {TRIGGER}" + assert body.splitlines()[0] == f"**When to apply:** {TRIGGER}" + + +def test_a_short_lesson_is_exactly_one_chunk(): + """`chunk_document`'s first contract line: a record inside the window + yields one chunk identical to the historical `title\\nbody`. A lesson that + split into several would spread the trigger's weight across vectors that + each carry less of it.""" + title, body = lessons_svc.lesson_document(SUBJECT, TRIGGER, INSIGHT) + chunks = chunk_document(title, body) + + assert len(chunks) == 1 + assert chunks[0].count(TRIGGER) == 2 + + +def test_a_long_lesson_keeps_the_trigger_on_every_chunk(): + """The narrative question, answered by the chunker rather than by holding + the story out of the record. + + `rule_document` excludes a rule's `why` because long dated narrative made + sixteen dev-logs land on the centroid of "development". That finding + predates chunking (#280): a body over budget is now split, and EVERY chunk + is prefixed with the title — which for a lesson carries the trigger. So the + story occupies its own vectors instead of averaging itself into the + trigger's, and each of those vectors is still anchored to when the lesson + applies. + + This is why the insight stays in the body where a reader can see it. Holding + it out would cost the reader the only part that explains the lesson, to buy + a sharpness the chunker already provides. + """ + narrative = "\n\n".join( + f"## Section {i}\n" + ("An unrelated sentence about deployment. " * 40) + for i in range(6) + ) + title, body = lessons_svc.lesson_document(SUBJECT, TRIGGER, narrative) + chunks = chunk_document(title, body) + + assert len(chunks) > 1, "the fixture must actually exceed the chunk budget" + assert all(TRIGGER in chunk for chunk in chunks) + + +def test_a_lesson_with_no_trigger_still_embeds(): + """Degrades to title + insight, the way a rule with no trigger does — less + sharply, and still findable. That is an argument for prompting hard for a + trigger at write time, not for padding the document with whatever text is + to hand.""" + title, body = lessons_svc.lesson_document(SUBJECT, "", INSIGHT) + + assert title == SUBJECT + assert body == INSIGHT + assert chunk_document(title, body) == [f"{SUBJECT}\n{INSIGHT}"] + + +def test_the_composed_body_is_the_one_the_reader_is_parsed_back_from(): + """`compose_body` writes the trigger line and `lesson_trigger` reads it. A + lesson whose mirror in `data` is missing still answers correctly, so the + two must agree on the exact markdown — which is why neither is written by + hand at a call site.""" + from types import SimpleNamespace + + _, body = lessons_svc.lesson_document(SUBJECT, TRIGGER, INSIGHT) + no_mirror = SimpleNamespace(data=None, body=body) + + assert lessons_svc.lesson_trigger(no_mirror) == TRIGGER + + +def test_the_title_and_body_are_composed_by_one_call(): + """`lesson_document` returns both halves so they cannot be built apart. A + title carrying the trigger over a body that does not would embed as an + ordinary note, and every listing would still look correct.""" + assert lessons_svc.lesson_document(SUBJECT, TRIGGER, INSIGHT) == ( + lessons_svc.compose_title(SUBJECT, TRIGGER), + lessons_svc.compose_body(INSIGHT, TRIGGER), + ) diff --git a/tests/test_lesson_kind.py b/tests/test_lesson_kind.py index 6d1e46f..25fdd2a 100644 --- a/tests/test_lesson_kind.py +++ b/tests/test_lesson_kind.py @@ -99,6 +99,18 @@ def test_the_kind_is_in_the_browse_vocabulary(): assert lessons_svc.LESSON_NOTE_TYPE in knowledge_svc.NON_TASK_FACETS +def test_the_global_kinds_list_names_the_lesson_and_nothing_else(): + """`embeddings.GLOBAL_NOTE_TYPES` spells "lesson" as a literal because + `services.lessons` imports `trigger_title` from that module, so importing + it back would close a cycle (milestone 385 step 3). The copy is pinned + here instead — the one thing a literal costs is that it can drift, and + this is what stops it. + """ + from scribe.services.embeddings import GLOBAL_NOTE_TYPES + + assert GLOBAL_NOTE_TYPES == (lessons_svc.LESSON_NOTE_TYPE,) + + def test_a_lesson_is_not_a_task_on_either_arm_of_the_filter(): """The cell left empty on purpose (#3163). diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index 002db62..951becf 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -76,6 +76,28 @@ async def test_fable_search_content_type_filters_at_service_layer(): assert mock_search.call_args.kwargs["is_task"] is None +@pytest.mark.asyncio +async def test_an_explicit_search_reaches_a_lesson_from_any_project(): + """The wiring half of milestone 385 step 3. + + A lesson records an insight that transfers, so a project filter that hid it + would hide it precisely on the project that has not learned it yet. This is + the EXPLICIT search — the operator asked — so it opts in; the unasked-for + injection arms decide their own budget separately (step 5). + + Asserted on the kwarg rather than on results, because what can regress here + is the wiring: the service grew the capability and a call site that never + passes it leaves the whole kind unreachable, with every unit test still + green. + """ + _user_id_ctx.set(7) + mock_search = AsyncMock(return_value=[]) + with patch("scribe.mcp.tools.search.semantic_search_notes", mock_search): + await search(q="x", project_id=3) + + assert mock_search.call_args.kwargs["include_global_kinds"] is True + + @pytest.mark.asyncio async def test_fable_search_limit_is_clamped(): _user_id_ctx.set(7)