From 1fca8c2808fc7bea9970114ab249c7f664eed03b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 11:25:53 -0400 Subject: [PATCH] feat(retrieval): a System's charter becomes an answer, not just a filter (#4251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 2 of #4251. A System's `description` is a charter — several hundred words saying what belongs in that area and what does not — and it is the answer to "which part of this codebase does X live in". There was no semantic path to one: `list_systems` enumerates, and `search(system_id=…)` uses a System as a FILTER over notes. So a System could narrow a search and could never be the answer to one, and an agent asking where a record belonged had to read every charter or guess. ITS OWN SEARCH, not a `content_type` over notes, for the reason note 3163 gives about milestones: the row could be shared, the search cannot. A charter competing with the whole note corpus for one top-k is outranked by the records filed under it — the right answer crowded out by its own contents — and "where does this belong?" is a different question from "what prior art is there?", which a caller asking one should not have to read past answers to. So `system_embeddings` (0107) joins note_, rule_ and milestone_embeddings as the fourth sibling, with `system_document`, `upsert_system_embedding`, `semantic_search_systems`, a startup backfill and `search(content_type= "system")`. Scoped like milestones: with a project_id, that project's Systems if the caller can read the project (rule 78); without one, the caller's own. Archived Systems are excluded — an archived area is one the operator has said is no longer where things go, which is exactly the question being asked. `system_document` is the plainest of the four shapes on purpose. A charter is already written as the thing this search has to match, in the words someone asking would use — so there is no trigger to synthesise as `rule_document` must, and no second record to gather as `task_document` must. The stored charter IS the sharp document, the way a snippet's is. `color`, `status` and `order_index` stay out: presentation and bookkeeping, and a vector carrying them would be answering a question nobody asks of a charter. The search publishes `report["best_chunk"]` from the start rather than being retrofitted, which is what #4251 asked of any fourth search. It matters more here than anywhere: a charter runs long and a result shows its NAME, so a match on the paragraph that actually decides where a record belongs would otherwise be previewed by two words that cannot say. The id that comes back is the one `system_id`, `system_ids` and `list_system_records` already take, so the answer to "where does this belong?" is directly usable as "show me what is there" and as "file it here". `embed_system` sits beside `notes.embed_note` at the service for #2056's reason — every door gets it by construction. Not called on delete: that is a soft delete and the search joins through `System`, so the vectors are already unreachable, and leaving them means a restore is findable again immediately. `system_embeddings` is declared in backup's `_NOT_INCLUDED` as derived, beside its three siblings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- alembic/versions/0107_system_embeddings.py | 67 +++++++ src/scribe/app.py | 10 +- src/scribe/mcp/tools/search.py | 63 ++++++- src/scribe/models/__init__.py | 4 +- src/scribe/models/embedding.py | 39 ++++ src/scribe/services/backup.py | 3 +- src/scribe/services/embeddings.py | 206 +++++++++++++++++++++ src/scribe/services/systems.py | 34 ++++ tests/test_mcp_tool_search.py | 82 +++++++- tests/test_system_charter_search.py | 164 ++++++++++++++++ 10 files changed, 663 insertions(+), 9 deletions(-) create mode 100644 alembic/versions/0107_system_embeddings.py create mode 100644 tests/test_system_charter_search.py diff --git a/alembic/versions/0107_system_embeddings.py b/alembic/versions/0107_system_embeddings.py new file mode 100644 index 0000000..e98251c --- /dev/null +++ b/alembic/versions/0107_system_embeddings.py @@ -0,0 +1,67 @@ +"""system_embeddings — a charter becomes an ANSWER, not just a filter (#4251) + +Revision ID: 0107 +Revises: 0106 +Create Date: 2026-09-21 + +A System's `description` is a charter: several hundred words saying what +belongs in that area and what does not. It is the answer to "which part of +this codebase does X live in", and there was no semantic path to it. +`list_systems` enumerates and `search(system_id=…)` uses a System as a FILTER +over notes — so a System could narrow a search and could never be the answer +to one, and an agent asking where a record belonged had to read every charter +or guess. + +The fourth sibling of note_embeddings (0067), rule_embeddings (0089) and +milestone_embeddings (0102), and for the same reason note 3163 gives about the +third: the row could be shared, the search cannot. "Where does this belong?" +is a different question from "what prior art is there?", and no note or rule +search can answer it, because a charter is not a note. + +The vectors are DERIVED: nothing is backfilled here, the startup backfill +writes them. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0107" +down_revision = "0106" +branch_labels = None +depends_on = None + +# Matches its three siblings — bge-small-en-v1.5, 384-dim. +_EMBEDDING_DIM = 384 + + +def upgrade() -> None: + op.create_table( + "system_embeddings", + sa.Column( + "system_id", sa.Integer(), + sa.ForeignKey("systems.id", ondelete="CASCADE"), primary_key=True, + ), + sa.Column("chunk_index", sa.Integer(), primary_key=True), + sa.Column("chunk_text", sa.Text(), nullable=False), + sa.Column("chunker_version", sa.Integer(), nullable=False), + sa.Column( + "updated_at", sa.DateTime(timezone=True), nullable=False, + server_default=sa.text("now()"), + ), + ) + # Raw DDL for the vector column, as 0067, 0089 and 0102 do: the type comes + # from the pgvector extension, not SQLAlchemy's type system. + op.execute( + f"ALTER TABLE system_embeddings ADD COLUMN embedding vector({_EMBEDDING_DIM}) NOT NULL" + ) + op.execute( + """ + CREATE INDEX ix_system_embeddings_embedding_hnsw + ON system_embeddings + USING hnsw (embedding vector_cosine_ops) + """ + ) + + +def downgrade() -> None: + op.execute("DROP INDEX IF EXISTS ix_system_embeddings_embedding_hnsw") + op.drop_table("system_embeddings") diff --git a/src/scribe/app.py b/src/scribe/app.py index a0e3362..0ebfa7d 100644 --- a/src/scribe/app.py +++ b/src/scribe/app.py @@ -173,7 +173,8 @@ def create_app() -> Quart: from scribe.services.auth import start_auth_token_retention_loop from scribe.services.embeddings import ( - backfill_milestone_embeddings, backfill_note_embeddings, backfill_rule_embeddings, + backfill_milestone_embeddings, backfill_note_embeddings, + backfill_rule_embeddings, backfill_system_embeddings, ) from scribe.services.logging import start_log_retention_loop from scribe.services.notifications import start_notification_loop @@ -243,6 +244,13 @@ def create_app() -> Quart: await backfill_milestone_embeddings() except Exception: logger.warning("Milestone embedding backfill failed", exc_info=True) + # Systems got vectors in #4251, so before this pass every charter + # ever written is unfindable — a System could narrow a search and + # never be the answer to one. + try: + await backfill_system_embeddings() + except Exception: + logger.warning("System embedding backfill failed", exc_info=True) # Snippets written before migration 0070 have no `notes.data` mirror, # and the location reverse lookup queries that column — an unfilled # row would read as "no snippet here" rather than as a gap. Separate diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 6f05632..2ffc329 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -15,7 +15,7 @@ from scribe.services.knowledge import content_type_filters 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, + semantic_search_rules, semantic_search_systems, ) from scribe.services import rulebooks as rulebooks_svc from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary @@ -38,7 +38,7 @@ _EXCERPT_CHARS = 1000 # These two do not reach `semantic_search_notes` at all: they have their own # search and their own result shape, so they are dispatched before the mapping # and passed in only so the refusal message lists everything THIS door takes. -_OWN_SEARCH = ("rule", "milestone") +_OWN_SEARCH = ("rule", "milestone", "system") async def _search_rules(uid: int, q: str, limit: int, project_id: int) -> dict: @@ -141,6 +141,53 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d } +async def _search_systems(uid: int, q: str, limit: int, project_id: int) -> dict: + """Systems by meaning — "where does this belong?" (#4251). + + A System's `description` is a charter: several hundred words saying what + belongs in that area and what does not. `list_systems` enumerates them and + `system_id` filters by one, so before this a System could NARROW a search + and could never be the answer to one — an agent asking where a record + belonged had to read every charter or guess. + + Its own result shape and its own search, not a `content_type` over notes, + because the question is different: "where does this belong?" is not "what + prior art is there?". A charter competing with the whole note corpus for + one top-k would also be outranked by the records filed under it, and the + right answer would be crowded out by its own contents. + + The charter's `matched` passage comes along rather than the whole thing. + A charter is long and the paragraph that decides where a record belongs is + the one worth reading; the rest is get_system (#4243). + + The id that comes back is the one `search(system_id=…)`, + `list_system_records` and every `system_ids` argument take — so the answer + to "where does this belong?" is directly usable as "show me what is there" + and as "file it here". + """ + report: dict = {} + raw = await semantic_search_systems( + uid, q, project_id=project_id or None, limit=limit, report=report, + ) + chunks = report.get("best_chunk") or {} + return { + "results": [ + { + "id": sys_.id, + "name": sys_.name, + **excerpt_fields( + sys_.description or "", chunks.get(int(sys_.id)), + _EXCERPT_CHARS, key="matched", + ), + "project_id": sys_.project_id, + "similarity": float(score), + } + for score, sys_ in raw + ], + "total": len(raw), + } + + def result_excerpt(note, chunk: dict | None) -> dict: """The part of a record a caller judges "should I open this?" on. @@ -204,8 +251,14 @@ async def search( claim that the corpus holds nothing, and a typo must not be able to make that claim. - Or 'rule' (RULES only — the operator's standing - instructions, searchable by meaning since milestone 307). + THREE KINDS WITH THEIR OWN SEARCH AND THEIR OWN RESULT SHAPE, + because each answers a question no note search can: + 'rule' (the operator's standing instructions — searchable by + meaning since milestone 307), 'milestone' ("is there already a + plan for this?"), and 'system' ("where does this belong?" — a + System's charter says what belongs in an area and what does not, + and the id it returns is the one `system_id` and `system_ids` + take). Reach for 'rule' when you want to know whether a standing instruction covers something: "is there a rule about release tagging?". A hit carries the rule's `why` and `how_to_apply`, @@ -258,6 +311,8 @@ async def search( return await _search_rules(uid, q, limit, project_id) if content_type == "milestone": return await _search_milestones(uid, q, limit, project_id) + if content_type == "system": + return await _search_systems(uid, q, limit, project_id) filters = content_type_filters(content_type, extra=_OWN_SEARCH) is_task = filters.get("is_task") t0 = time.perf_counter() diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index 1584b5f..abdcec7 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -53,7 +53,9 @@ from scribe.models.user import User # noqa: E402, F401 from scribe.models.app_log import AppLog # noqa: E402, F401 from scribe.models.password_reset import PasswordResetToken # noqa: E402, F401 from scribe.models.invitation import InvitationToken # noqa: E402, F401 -from scribe.models.embedding import MilestoneEmbedding, NoteEmbedding, RuleEmbedding # noqa: E402, F401 +from scribe.models.embedding import ( # noqa: E402, F401 + MilestoneEmbedding, NoteEmbedding, RuleEmbedding, SystemEmbedding, +) from scribe.models.retrieval_log import RetrievalLog # noqa: E402, F401 from scribe.models.retrieval_tuning import RetrievalTuningEvent # noqa: E402, F401 from scribe.models.note_usage import NoteUsageEvent # noqa: E402, F401 diff --git a/src/scribe/models/embedding.py b/src/scribe/models/embedding.py index 3ab02a2..de5797c 100644 --- a/src/scribe/models/embedding.py +++ b/src/scribe/models/embedding.py @@ -128,3 +128,42 @@ class MilestoneEmbedding(Base): DateTime(timezone=True), default=lambda: datetime.now(timezone.utc), ) + + +class SystemEmbedding(Base): + """One embedding vector per CHUNK of a System's charter (#4251). + + The fourth sibling, for the reason note 3163 gives about the third: the row + could be shared, the search cannot. A System's `description` is a charter — + several hundred words saying what belongs in that area and what does not — + and it is the answer to "which part of this codebase does X live in". + Before this there was no semantic path to one: `list_systems` enumerates, + and `search(system_id=…)` uses a System as a FILTER over notes. So a System + could narrow a search and could never be the answer to one. + + "Where does this belong?" is a different question from "what prior art is + there?", which is why this is its own search rather than a note_type: a + charter competing with two thousand notes for the same top-k would be + outranked by the records filed under it, and the right answer would be + crowded out by its own contents. + + The document is the name and the charter. Derived data: the startup + backfill regenerates it, which is also how a chunker-version bump is + handled. + """ + + __tablename__ = "system_embeddings" + + system_id: Mapped[int] = mapped_column( + Integer, + ForeignKey("systems.id", ondelete="CASCADE"), + primary_key=True, + ) + chunk_index: Mapped[int] = mapped_column(Integer, primary_key=True) + embedding: Mapped[list] = mapped_column(Vector(EMBEDDING_DIM), nullable=False) + chunk_text: Mapped[str] = mapped_column(Text, nullable=False) + chunker_version: Mapped[int] = mapped_column(Integer, nullable=False) + updated_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), + default=lambda: datetime.now(timezone.utc), + ) diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 80bd84b..0f515fc 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -123,7 +123,7 @@ _BACKED_UP = [ # Tables intentionally NOT in the backup, surfaced in the payload so the gap is # explicit rather than silent. ACL (groups/shares) is a coherent follow-up; -# note_embeddings and rule_embeddings are derived (regenerated at startup +# the four *_embeddings tables are derived (regenerated at startup # from the records themselves, which is also how a chunker bump is handled); api_keys are # sensitive credentials; retrieval_logs is observational telemetry that nothing # reads for correctness and that grows per query; the rest are @@ -135,6 +135,7 @@ _BACKED_UP = [ _NOT_INCLUDED = [ "groups", "group_memberships", "project_shares", "note_shares", "api_keys", "note_embeddings", "rule_embeddings", "milestone_embeddings", + "system_embeddings", "app_logs", "notifications", "invitation_tokens", "password_reset_tokens", "user_profiles", "retrieval_logs", diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 7927fd6..eba3c5f 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -29,6 +29,7 @@ from scribe.services.access import can_read_project, notes_visibility_clause if TYPE_CHECKING: # resolves forward refs without importing at runtime from scribe.models.milestone import Milestone from scribe.models.rulebook import Rule + from scribe.models.system import System logger = logging.getLogger(__name__) @@ -1451,6 +1452,211 @@ async def semantic_search_milestones( return kept +# --- Systems: the charter as an ANSWER, not a filter (#4251) ----------------- + + +def system_document( + name: str | None, description: str | None +) -> tuple[str | None, str | None]: + """The (title, body) a System is EMBEDDED as — its name and its charter. + + The plainest of the four document shapes, and deliberately so. A System's + `description` is already written as the thing this search has to match: it + says what belongs in the area and what does not, which is the answer to + "where does this go?" in the words someone asking would use. There is no + trigger to synthesise, as `rule_document` must, and no separate record to + gather in, as `task_document` must — the stored charter IS the sharp + document, the way a snippet's is. + + `color`, `status` and `order_index` are left out. They are presentation and + bookkeeping; a vector that carried them would be answering a question + nobody asks of a charter. + + A System with no charter yet degrades to its name. It still embeds, just + weakly — a bare name is exactly what the model docstring says is never + enough, and this is an argument for writing the charter, not for padding + the document with whatever is to hand. + """ + return (name or "").strip() or None, (description or "").strip() or None + + +async def upsert_system_embedding( + system_id: int, name: str | None, description: str | None +) -> None: + """Chunk, embed and persist a System's vectors. Safe to fire-and-forget. + + The third sibling's contract exactly: one document definition shared by the + write path and the backfill, and an atomic per-System replacement guarded + by the parent-row claim (#3262), so a System deleted mid-refresh wins. + """ + from scribe.models.embedding import SystemEmbedding + from scribe.models.system import System + + doc_title, doc_body = system_document(name, description) + chunks = chunk_document(doc_title, doc_body) + try: + if not chunks: + async with async_session() as session: + await session.execute( + delete(SystemEmbedding).where(SystemEmbedding.system_id == system_id) + ) + await session.commit() + return + except Exception: + logger.warning("Failed to clear embedding for system %d", system_id, exc_info=True) + return + + try: + vectors = await get_embeddings(chunks) + except Exception: + logger.debug("Skipping embedding for system %d — embedder unavailable", system_id) + return + + try: + async with async_session() as session: + if not await _claim_parent_row(session, System.id, system_id, "system"): + return + await session.execute( + delete(SystemEmbedding).where(SystemEmbedding.system_id == system_id) + ) + for index, (chunk, vector) in enumerate(zip(chunks, vectors)): + session.add(SystemEmbedding( + system_id=system_id, chunk_index=index, embedding=vector, + chunk_text=chunk, chunker_version=CHUNKER_VERSION, + )) + await session.commit() + except Exception: + logger.warning("Failed to persist embedding for system %d", system_id, exc_info=True) + + +async def semantic_search_systems( + user_id: int, + query: str, + *, + project_id: int | None = None, + limit: int = 5, + threshold: float = _SIMILARITY_THRESHOLD, + report: dict | None = None, +) -> list[tuple[float, "System"]]: + """Return up to *limit* (score, system) pairs most like *query*. + + Answers "where does this belong?" — the question asked before filing a + record or opening a file, and the one that had no tool: `list_systems` + enumerates and `system_id` filters, so a charter could narrow a search and + could never be the answer to one. + + ITS OWN SEARCH rather than a note kind, for what note 3163 says about + milestones: a charter competing with the whole note corpus for one top-k + would be outranked by the records filed under it, and the right answer + would be crowded out by its own contents. The questions are different too — + "where does this belong?" is not "what prior art is there?" — and a caller + asking one should not have to read past answers to the other. + + SCOPE. With `project_id`, that project's Systems, provided the caller can + read the project (access.can_read_project, rule 78) — a collaborator on a + shared project sees its areas, which is the point of a charter. Without + one, the Systems the caller owns across their projects. Archived Systems + are excluded: an archived area is one the operator has said is no longer + where things go, and that is exactly the question being asked. + + Collapses to best-chunk-per-System and publishes the winning chunk, like + the sibling searches — from the start rather than retrofitted (#4243). + Returns an empty list if the embedder is unavailable, the project is not + readable, or on any error: a recall aid must never break the call it + serves. + """ + from scribe.models.embedding import SystemEmbedding + from scribe.models.system import System + + if not query or not query.strip(): + return [] + try: + query_vec = await get_embedding(query) + except Exception: + logger.debug("System search skipped — embedder unavailable") + return [] + + distance = SystemEmbedding.embedding.cosine_distance(query_vec) + try: + if project_id: + if not await can_read_project(user_id, project_id): + return [] + scope = System.project_id == project_id + else: + scope = System.user_id == user_id + async with async_session() as session: + rows = (await session.execute( + select( + System, + distance.label("distance"), + SystemEmbedding.chunk_index, + SystemEmbedding.chunk_text, + ) + .select_from(SystemEmbedding) + .join(System, SystemEmbedding.system_id == System.id) + .where( + scope, + System.deleted_at.is_(None), + System.status != "archived", + ) + .order_by(distance) + .limit(limit * _CHUNK_OVERFETCH) + )).all() + except Exception: + logger.warning("System semantic search failed", exc_info=True) + return [] + + best: dict[int, tuple[float, object]] = {} + # A charter runs to several hundred words and a result shows its NAME — so + # a match on the paragraph that actually decides where a record belongs + # would be previewed by two words that cannot. The winning chunk is what + # the caller should see (#4243). + won: dict[int, dict] = {} + for system, dist, chunk_index, chunk_text in rows: + score = 1.0 - float(dist) + if system.id not in best or score > best[system.id][0]: + best[system.id] = (score, system) + won[int(system.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(sys_.id): won[int(sys_.id)] for _s, sys_ in kept if int(sys_.id) in won + }) + return kept + + +async def backfill_system_embeddings() -> None: + """Embed Systems that have no current vectors. Runs at startup beside the + note, rule and milestone backfills; a CHUNKER_VERSION bump re-embeds. + + This is also the pass that makes every existing charter findable at all — + Systems got vectors in #4251, so before it runs there are none.""" + from scribe.models.embedding import SystemEmbedding + from scribe.models.system import System + + try: + async with async_session() as session: + current = select(SystemEmbedding.system_id).where( + SystemEmbedding.chunker_version == CHUNKER_VERSION + ) + stale = (await session.execute( + select(System.id, System.name, System.description) + .where(System.deleted_at.is_(None), System.id.notin_(current)) + )).all() + except Exception: + logger.warning("System embedding backfill: failed to query systems", exc_info=True) + return + + if not stale: + logger.info("System embedding backfill: all systems current at chunker v%d", CHUNKER_VERSION) + return + logger.info("System embedding backfill: embedding %d system(s)", len(stale)) + for system_id, name, description in stale: + await upsert_system_embedding(system_id, name, description) + + async def backfill_milestone_embeddings() -> None: """Embed milestones that have no current vectors. Runs at startup beside the note and rule backfills; a CHUNKER_VERSION bump re-embeds.""" diff --git a/src/scribe/services/systems.py b/src/scribe/services/systems.py index d2f5552..7f2f0bc 100644 --- a/src/scribe/services/systems.py +++ b/src/scribe/services/systems.py @@ -114,6 +114,38 @@ async def seed_standard_systems(user_id: int, project_id: int) -> list[System]: return out +def embed_system(system) -> None: + """Refresh a System's charter vectors, fire-and-forget (#4251). + + The twin of `notes.embed_note`, and here for the same reason (#2056): at + the service, so every door gets it by construction rather than each route + and tool remembering. A charter edited through one door and not another + would stay findable by what it used to say, and nothing would report it. + + Not called on delete. `delete_system` is a SOFT delete and the search joins + through `System`, so a deleted System's vectors are already unreachable — + and leaving them means a restore is findable again immediately instead of + waiting for the next startup backfill. + + Import is lazy so importing this module doesn't pull in the embedding + model; a missing event loop (unit tests, scripts) is ordinary, not an + error; exceptions are swallowed because a System that saved must not fail + on its index refresh. + """ + try: + import asyncio + + from scribe.services.embeddings import upsert_system_embedding + + asyncio.create_task( + upsert_system_embedding(system.id, system.name, system.description) + ) + except RuntimeError: + pass # no running loop — a sync caller, not a failure + except Exception: # noqa: BLE001 - never let indexing break a write + logger.exception("embedding refresh failed for system %s", system.id) + + async def create_system( user_id: int, project_id: int, @@ -144,6 +176,7 @@ async def create_system( session.add(system) await session.commit() await session.refresh(system) + embed_system(system) return system @@ -195,6 +228,7 @@ async def update_system(user_id: int, system_id: int, **fields: object) -> Syste system.updated_at = datetime.now(timezone.utc) await session.commit() await session.refresh(system) + embed_system(system) return system diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index ee167c1..2fff7a7 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -1,7 +1,7 @@ """search tool — proves the tool pattern (context + service call + dict shape). Service call is mocked; no DB needed.""" -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -223,7 +223,8 @@ async def test_the_docstring_names_every_kind_the_tool_accepts(): doc = search_tool.__doc__ or "" for facet in FACET_TYPES: assert f"'{facet}'" in doc, f"{facet} is accepted but never documented" - assert "'rule'" in doc and "'milestone'" in doc and "'all'" in doc + for own in ("'rule'", "'milestone'", "'system'", "'all'"): + assert own in doc, f"{own} dispatches somewhere and is never documented" @pytest.mark.asyncio @@ -329,3 +330,80 @@ async def test_milestone_search_is_its_own_shape_and_scopes_to_the_project(): "status": "active", "project_id": 30, "total": 0, "completed": 0, "similarity": 0.81, }] + + +# --- systems: the charter as an answer, not a filter (#4251) ----------------- + + +@pytest.mark.asyncio +async def test_system_search_is_its_own_shape_and_carries_the_matched_passage(): + """A System's charter runs to several hundred words and a result shows its + NAME — so a match on the paragraph that actually decides where a record + belongs would be previewed by two words that cannot. The passage comes + along, marked as the passage (#4243).""" + _user_id_ctx.set(7) + charter = ( + "Opening sentence about the area in general. " + + "x" * 400 + + " Retrieval telemetry and the floors it judges belong here." + ) + fake = MagicMock(id=3, project_id=2, description=charter) + # `name` is MagicMock's own constructor kwarg — passed in, it names the + # mock and leaves `.name` a mock object, which is note #2833's whole point. + fake.name = "Retrieval & recall" + + async def _system_search(uid, q, **kwargs): + kwargs["report"]["best_chunk"] = { + 3: {"index": 2, "text": "Retrieval telemetry and the floors it judges belong here."} + } + return [(0.81, fake)] + + with patch("scribe.mcp.tools.search.semantic_search_systems", _system_search): + out = await search(q="where does retrieval telemetry go?", + content_type="system", project_id=2) + + assert out["total"] == 1 + row = out["results"][0] + assert row["id"] == 3 + assert row["name"] == "Retrieval & recall" + assert row["project_id"] == 2 + assert row["similarity"] == 0.81 + # The passage that won, not the charter's opening — and SAID to be that. + assert row["matched"] == ( + "Retrieval telemetry and the floors it judges belong here." + ) + assert row["matched_is"] == "matched_passage" + assert row["body_length"] == len(charter) + assert "read_full" in row, "a 56-character span of a 500-character charter" + # The charter itself does not ride along: get_system reads it. + assert "description" not in row + + +@pytest.mark.asyncio +async def test_system_search_scopes_to_the_project_it_was_given(): + """Systems are per-project and a charter from another project is not an + answer to "where does this belong here?".""" + _user_id_ctx.set(7) + mock = AsyncMock(return_value=[]) + with patch("scribe.mcp.tools.search.semantic_search_systems", mock): + await search(q="x", content_type="system", project_id=30) + assert mock.call_args.kwargs["project_id"] == 30 + + mock.reset_mock() + with patch("scribe.mcp.tools.search.semantic_search_systems", mock): + await search(q="x", content_type="system") + assert mock.call_args.kwargs["project_id"] is None + + +@pytest.mark.asyncio +async def test_a_system_search_does_not_go_through_the_note_search(): + """Its own search, not a note_type. A charter competing with the whole note + corpus for one top-k is outranked by the records filed under it.""" + _user_id_ctx.set(7) + notes = AsyncMock(return_value=[]) + with ( + patch("scribe.mcp.tools.search.semantic_search_notes", notes), + patch("scribe.mcp.tools.search.semantic_search_systems", AsyncMock(return_value=[])), + ): + await search(q="x", content_type="system") + notes.assert_not_awaited() diff --git a/tests/test_system_charter_search.py b/tests/test_system_charter_search.py new file mode 100644 index 0000000..bc91f23 --- /dev/null +++ b/tests/test_system_charter_search.py @@ -0,0 +1,164 @@ +"""Systems become findable by meaning — the charter as an ANSWER (#4251). + +A System's `description` is a charter: several hundred words saying what +belongs in that area and what does not. It is the answer to "where does this +go?", and there was no semantic path to it — `list_systems` enumerates and +`search(system_id=…)` uses a System as a FILTER over notes, so a System could +narrow a search and could never be the answer to one. + +These pin the three halves: the document shape, the search's scoping and +best-chunk contract, and that a charter edited through any door is re-indexed. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import embeddings as emb +from scribe.services import systems as systems_svc +from tests.helpers import make_mock_session + + +def _system(system_id=3, name="Retrieval & recall", description="the charter"): + s = MagicMock(id=system_id, project_id=2, description=description) + s.name = name # never via the constructor — see note #2833 + return s + + +# --- the document shape ------------------------------------------------------ + + +def test_the_document_is_the_name_and_the_charter(): + assert emb.system_document("Retrieval", "what belongs here") == ( + "Retrieval", "what belongs here" + ) + + +def test_a_system_with_no_charter_yet_degrades_to_its_name(): + """It still embeds, just weakly. That is an argument for writing the + charter, not for padding the document with whatever is to hand.""" + assert emb.system_document("Retrieval", None) == ("Retrieval", None) + assert emb.system_document("Retrieval", " ") == ("Retrieval", None) + assert emb.chunk_document(*emb.system_document("Retrieval", None)) == ["Retrieval"] + + +def test_an_empty_system_produces_no_document_at_all(): + """Callers gate on falsiness to clear vectors rather than embed nothing.""" + assert emb.system_document("", "") == (None, None) + assert emb.chunk_document(*emb.system_document("", "")) == [] + + +# --- the search -------------------------------------------------------------- + + +def _rows_ctx(rows): + session = MagicMock() + result = MagicMock() + result.all.return_value = rows + session.execute = AsyncMock(return_value=result) + ctx = MagicMock() + ctx.__aenter__ = AsyncMock(return_value=session) + ctx.__aexit__ = AsyncMock(return_value=False) + return session, ctx + + +@pytest.mark.asyncio +async def test_the_search_collapses_to_best_chunk_and_reports_which_one_won(): + """A charter is long, and a result shows its NAME — so the paragraph that + actually decides where a record belongs has to come back with it, or the + caller judges from two words that cannot say (#4243). Published from the + start here rather than retrofitted, which is what #4251 asked for.""" + a, b = _system(3), _system(4, name="Data Model") + rows = [ + (a, 0.10, 2, "retrieval telemetry belongs here"), + (b, 0.22, 0, "the persistence layer"), + (a, 0.40, 0, "a weaker paragraph of the same charter"), + ] + _session, ctx = _rows_ctx(rows) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=ctx), + 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_systems( + 7, "where does telemetry go?", + project_id=2, threshold=0.0, report=report, + ) + + assert [s.id for _score, s in out] == [3, 4] + assert out[0][0] == 1.0 - 0.10 # the BEST chunk's score + assert report["best_chunk"][3] == { + "index": 2, "text": "retrieval telemetry belongs here", + } + + +@pytest.mark.asyncio +async def test_a_project_the_caller_cannot_read_returns_nothing(): + """Rule 78 — the charter of a project someone was not given is not an + answer to any question they are entitled to ask.""" + _session, ctx = _rows_ctx([]) + with ( + patch.object(emb, "async_session", return_value=ctx), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=False)), + ): + assert await emb.semantic_search_systems(7, "q", project_id=99) == [] + + +@pytest.mark.asyncio +async def test_an_empty_query_never_reaches_the_embedder(): + with patch.object(emb, "get_embedding", AsyncMock()) as embed: + assert await emb.semantic_search_systems(7, " ") == [] + embed.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_a_search_that_fails_returns_nothing_rather_than_raising(): + """A recall aid must never break the call it serves.""" + with patch.object(emb, "get_embedding", AsyncMock(side_effect=RuntimeError)): + assert await emb.semantic_search_systems(7, "q") == [] + + +# --- staying current --------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_creating_a_system_indexes_its_charter(): + session = make_mock_session() + with ( + patch.object(systems_svc, "async_session", return_value=session), + patch.object(systems_svc.access, "can_write_project", AsyncMock(return_value=True)), + patch.object(systems_svc, "embed_system") as embed, + ): + await systems_svc.create_system(7, 2, "Retrieval", description="the charter") + embed.assert_called_once() + + +@pytest.mark.asyncio +async def test_editing_a_charter_re_indexes_it(): + """A charter edited and not re-indexed stays findable by what it used to + say — the stale-vector case, and the one that matters most for a record + whose whole job is to say where things belong now.""" + system = _system() + session = make_mock_session() + session.get = AsyncMock(return_value=system) + system.deleted_at = None + with ( + patch.object(systems_svc, "async_session", return_value=session), + patch.object(systems_svc.access, "can_write_project", AsyncMock(return_value=True)), + patch.object(systems_svc, "embed_system") as embed, + ): + await systems_svc.update_system(7, 3, description="a new charter") + embed.assert_called_once_with(system) + + +def test_embedding_a_system_never_breaks_the_write_that_saved_it(): + """A System that saved must not fail on its index refresh. ValueError + rather than RuntimeError deliberately: RuntimeError is also what "no + running loop" raises, which is an ordinary sync caller and has its own + branch — this has to land in the general swallow to prove it exists.""" + with patch( + "scribe.services.embeddings.upsert_system_embedding", + side_effect=ValueError("boom"), + ): + systems_svc.embed_system(_system()) # returns quietly, does not raise