diff --git a/alembic/versions/0089_rule_embeddings.py b/alembic/versions/0089_rule_embeddings.py new file mode 100644 index 0000000..3537c82 --- /dev/null +++ b/alembic/versions/0089_rule_embeddings.py @@ -0,0 +1,58 @@ +"""rule_embeddings — rules become findable by meaning (milestone 307 step 4, +decision note 3026) + +Revision ID: 0089 +Revises: 0088 +Create Date: 2026-08-26 + +Rules were the only major record type with no vector, so `search` could never +return one and a rule could only ever arrive by being preloaded. That single +fact is what made every rule compete for the same always-on budget. + +A sibling table rather than a generalisation of note_embeddings: the row could +have been made polymorphic, but the SEARCH could not — semantic_search_notes is +Note-specific scoping end to end, and a rule shares none of it. See the model +docstring for the full reasoning. + +The vectors are DERIVED data. Nothing is backfilled here: the startup backfill +regenerates them, which is also how a chunker-version bump is handled. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0089" +down_revision = "0088" +branch_labels = None +depends_on = None + +# Matches note_embeddings — bge-small-en-v1.5, 384-dim unit-normalized. +_EMBEDDING_DIM = 384 + + +def upgrade() -> None: + op.create_table( + "rule_embeddings", + sa.Column("rule_id", sa.BigInteger(), sa.ForeignKey("rules.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()")), + ) + # The vector column is added by raw DDL for the same reason 0067 did it: + # the type comes from the pgvector extension, not from SQLAlchemy's + # type system. + op.execute(f"ALTER TABLE rule_embeddings ADD COLUMN embedding vector({_EMBEDDING_DIM}) NOT NULL") + # HNSW for cosine distance — matches Vector.cosine_distance (`<=>`), so the + # search is an indexed ORDER BY ... LIMIT k rather than a full scan. + op.execute( + """ + CREATE INDEX ix_rule_embeddings_embedding_hnsw + ON rule_embeddings + USING hnsw (embedding vector_cosine_ops) + """ + ) + + +def downgrade() -> None: + op.execute("DROP INDEX IF EXISTS ix_rule_embeddings_embedding_hnsw") + op.drop_table("rule_embeddings") diff --git a/src/scribe/app.py b/src/scribe/app.py index da2e5ee..e158003 100644 --- a/src/scribe/app.py +++ b/src/scribe/app.py @@ -161,7 +161,7 @@ def create_app() -> Quart: import asyncio from scribe.services.auth import start_auth_token_retention_loop - from scribe.services.embeddings import backfill_note_embeddings + from scribe.services.embeddings import backfill_note_embeddings, backfill_rule_embeddings from scribe.services.logging import start_log_retention_loop from scribe.services.notifications import start_notification_loop @@ -176,6 +176,12 @@ def create_app() -> Quart: await backfill_note_embeddings() except Exception: logger.warning("Embedding backfill failed", exc_info=True) + # Rules got vectors in milestone 307; every rule written before it + # has none, so this is the pass that makes them findable at all. + try: + await backfill_rule_embeddings() + except Exception: + logger.warning("Rule 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 b1e5775..e7eb1c4 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -11,10 +11,44 @@ import time from scribe.mcp._context import current_user_id from scribe.services.access import owner_names_for -from scribe.services.embeddings import DEFAULT_SIMILARITY_THRESHOLD, semantic_search_notes +from scribe.services.embeddings import ( + DEFAULT_SIMILARITY_THRESHOLD, semantic_search_notes, semantic_search_rules, +) from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary +async def _search_rules(uid: int, q: str, limit: int) -> dict: + """Rules by meaning — a separate result shape because a rule IS different. + + A rule hit carries `why` and `how_to_apply`: they are the operational half + of a rule and the session-start payload never includes them, so a caller + who went looking should get the whole thing rather than a summary they then + have to re-fetch. + + Rules are not project-scoped the way notes are (a family rule belongs to no + project), so `project_id` and `system_id` do not apply here. + """ + raw = await semantic_search_rules(uid, q, limit=limit) + return { + "results": [ + { + "id": rule.id, + "title": rule.title, + "statement": rule.statement, + "when_to_apply": rule.when_to_apply or "", + "tier": rule.tier, + "why": rule.why or "", + "how_to_apply": rule.how_to_apply or "", + "topic_id": rule.topic_id, + "project_id": rule.project_id, + "similarity": float(score), + } + for score, rule in raw + ], + "total": len(raw), + } + + async def search( q: str, content_type: str = "all", @@ -33,7 +67,13 @@ async def search( Args: q: search query string. - content_type: 'all' (default), 'note' (notes only), or 'task' (tasks only). + content_type: 'all' (default), 'note' (notes only), 'task' (tasks + only), or 'rule' (RULES only — the operator's standing + instructions, searchable by meaning since milestone 307). + 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`, + which the session-start payload does not. limit: maximum number of results (1-50). project_id: Scope results to one project. PASS THE ACTIVE PROJECT'S ID whenever a project is in scope (the one you entered with @@ -56,6 +96,8 @@ async def search( """ uid = current_user_id() limit = max(1, min(limit, 50)) + if content_type == "rule": + return await _search_rules(uid, q, limit) is_task = {"note": False, "task": True}.get(content_type) # None => any t0 = time.perf_counter() raw = await semantic_search_notes( diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index 549a984..d55a116 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -25,7 +25,7 @@ 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 NoteEmbedding # noqa: E402, F401 +from scribe.models.embedding import NoteEmbedding, RuleEmbedding # noqa: E402, F401 from scribe.models.retrieval_log import RetrievalLog # noqa: E402, F401 from scribe.models.note_usage import NoteUsageEvent # noqa: E402, F401 from scribe.models.project import Project # noqa: E402, F401 diff --git a/src/scribe/models/embedding.py b/src/scribe/models/embedding.py index 9cf4f50..e206334 100644 --- a/src/scribe/models/embedding.py +++ b/src/scribe/models/embedding.py @@ -1,7 +1,7 @@ from datetime import datetime, timezone from pgvector.sqlalchemy import Vector -from sqlalchemy import DateTime, ForeignKey, Integer, Text +from sqlalchemy import BigInteger, DateTime, ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base @@ -45,3 +45,49 @@ class NoteEmbedding(Base): DateTime(timezone=True), default=lambda: datetime.now(timezone.utc), ) + + +class RuleEmbedding(Base): + """One embedding vector per CHUNK of a rule (milestone 307, note 3026). + + A SIBLING of NoteEmbedding rather than a generalisation of it, decided + deliberately: + + - The embedding ROW could have been made polymorphic. The SEARCH could not. + `semantic_search_notes` is a long function of Note-specific scoping — + the visibility clause, the supersession penalty, note_type/task_kind and + system filters — and a rule shares none of it. Rules scope by rulebook + ownership and project applicability instead. + - Generalising the row while still needing two searches is the worst of + both: a polymorphic key with referential integrity to neither table, on + the path every session start runs, to share four columns. + - What is genuinely common is BEHAVIOUR, not storage — get_embedding, + chunk_document, embedding_text and CHUNKER_VERSION are already free + functions and are reused as-is. Sharing those is the DRY win; sharing + the table would have been the DRY costume. + + No `user_id`: NoteEmbedding carries one and its own search deliberately + ignores it (scoping on the note instead, or shared records become + unreachable). Rather than repeat a column that exists to be ignored, a + rule's reach is resolved by joining the rule. + """ + + __tablename__ = "rule_embeddings" + + rule_id: Mapped[int] = mapped_column( + BigInteger, + ForeignKey("rules.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) + # Exactly what this vector encodes — inspectable when a ranking surprises. + # For a rule this is the trigger-first document, NOT the rule's `why`: + # `why` is dated incident narrative and would drag every rule toward one + # centroid (measured in note 2485). + 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 eaec68f..7140da1 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -81,7 +81,8 @@ _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 are derived (regenerated from note bodies); api_keys are +# note_embeddings and rule_embeddings 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 # transient/operational. @@ -91,7 +92,7 @@ _BACKED_UP = [ # like coverage while naming nothing the schema could confirm. _NOT_INCLUDED = [ "groups", "group_memberships", "project_shares", "note_shares", - "api_keys", "note_embeddings", "app_logs", "notifications", + "api_keys", "note_embeddings", "rule_embeddings", "app_logs", "notifications", "invitation_tokens", "password_reset_tokens", "user_profiles", "retrieval_logs", # Sensitive credentials, same reasoning as api_keys: a backup that carries diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index e5defcc..49255de 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -19,7 +19,7 @@ from collections.abc import Sequence from sqlalchemy import delete, or_, select from scribe.models import async_session -from scribe.models.embedding import NoteEmbedding +from scribe.models.embedding import NoteEmbedding, RuleEmbedding from scribe.models.note import Note from scribe.services.access import notes_visibility_clause @@ -612,3 +612,186 @@ async def backfill_note_embeddings() -> None: await asyncio.sleep(0.05) # gentle pacing logger.info("Embedding backfill complete: %d/%d notes embedded", success, len(notes_to_embed)) + + +# ── Rules (milestone 307, note 3026) ──────────────────────────────────── + +def rule_document( + title: str | None, statement: str | None, when_to_apply: str | None, +) -> tuple[str | None, str | None]: + """The (title, body) a rule is EMBEDDED as — trigger first, `why` never. + + Both halves of this are measured, not guessed (note 2485). That pass found + the snippet was the only sharp record in the corpus — a 0.153 top-to-second + gap against 0.010–0.023 for everything else — and that the cause was its + SHAPE: `{name} — {when_to_use}` as the title and `**When to use:** …` + repeated in the body, so purpose appears twice in a short document and + dominates the vector. This mirrors that exactly. + + And it excludes `why` on the same evidence. `why` is dated incident + narrative — rule 46's runs to 4,300 characters of it — and long, + multi-topic prose is precisely what made sixteen dev-logs mutually + indistinguishable: the average lands on the centroid of "development", + which every one of them shares. Adding `why` would not give the vector more + to work with; it would give every rule the same thing to work with. + + A rule with no trigger yet degrades to title + statement. It still embeds, + just less sharply — which is an argument for backfilling triggers, not an + argument for padding the document with whatever text is lying around. + """ + trigger = (when_to_apply or "").strip() + name = (title or "").strip() + body = (statement or "").strip() + if not trigger: + return name or None, body or None + return ( + f"{name} — {trigger}" if name else trigger, + f"When to apply: {trigger}\n\n{body}" if body else f"When to apply: {trigger}", + ) + + +async def upsert_rule_embedding( + rule_id: int, title: str | None, statement: str | None, + when_to_apply: str | None = None, +) -> None: + """Chunk, embed and persist a rule's vectors. Safe to fire-and-forget. + + The note twin's contract, for the same reasons: the document is built HERE + so the write path, the backfill and any re-embed share one definition, and + replacement is atomic per rule so a concurrent read sees the old chunk set + or the new one, never a mixture. + """ + doc_title, doc_body = rule_document(title, statement, when_to_apply) + chunks = chunk_document(doc_title, doc_body) + try: + if not chunks: + async with async_session() as session: + await session.execute( + delete(RuleEmbedding).where(RuleEmbedding.rule_id == rule_id) + ) + await session.commit() + return + except Exception: + logger.warning("Failed to clear embedding for rule %d", rule_id, exc_info=True) + return + + try: + vectors = await get_embeddings(chunks) + except Exception: + logger.debug("Skipping embedding for rule %d — embedder unavailable", rule_id) + return + + try: + async with async_session() as session: + await session.execute( + delete(RuleEmbedding).where(RuleEmbedding.rule_id == rule_id) + ) + for index, (chunk, vector) in enumerate(zip(chunks, vectors)): + session.add( + RuleEmbedding( + rule_id=rule_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 rule %d", rule_id, exc_info=True) + + +async def semantic_search_rules( + user_id: int, + query: str, + limit: int = 5, + threshold: float = _SIMILARITY_THRESHOLD, +) -> list[tuple[float, "Rule"]]: + """Return up to *limit* (score, rule) pairs most relevant to *query*. + + Scoped by OWNERSHIP — a rule is the caller's if they own its rulebook or + its project. Deliberately not filtered to what currently BINDS a given + project: this answers "is there a rule about this", which a person asking + wants answered across their whole rulebook. Deciding which rules bind where + is the surfacing question, and it has its own machinery + (get_applicable_rules) rather than a second, subtly different copy here. + + Collapses to best-chunk-per-rule like the note search, so a long rule split + across chunks competes once rather than crowding the results with itself. + + Returns an empty list if the embedder is unavailable or on any error. + """ + from scribe.models.project import Project + from scribe.models.rulebook import Rule, Rulebook, RulebookTopic + + if not query or not query.strip(): + return [] + try: + query_vec = await get_embedding(query) + except Exception: + logger.debug("Rule search skipped — embedder unavailable") + return [] + + max_distance = min(2.0, max(0.0, 1.0 - threshold)) + distance = RuleEmbedding.embedding.cosine_distance(query_vec) + + try: + async with async_session() as session: + rows = (await session.execute( + select(Rule, distance.label("distance")) + .select_from(RuleEmbedding) + .join(Rule, RuleEmbedding.rule_id == Rule.id) + .outerjoin(RulebookTopic, Rule.topic_id == RulebookTopic.id) + .outerjoin(Rulebook, RulebookTopic.rulebook_id == Rulebook.id) + .outerjoin(Project, Rule.project_id == Project.id) + .where( + Rule.deleted_at.is_(None), + distance <= max_distance, + # topic_id XOR project_id, so exactly one arm can match. + or_( + Rulebook.owner_user_id == user_id, + Project.user_id == user_id, + ), + ) + # Overfetch so collapsing chunks to their best row still fills + # the page — the same reason the note search overfetches. + .order_by(distance) + .limit(limit * _CHUNK_OVERFETCH) + )).all() + except Exception: + logger.warning("Rule semantic search failed", exc_info=True) + return [] + + best: dict[int, tuple[float, object]] = {} + for rule, dist in rows: + score = 1.0 - float(dist) + if rule.id not in best or score > best[rule.id][0]: + best[rule.id] = (score, rule) + ranked = sorted(best.values(), key=lambda pair: pair[0], reverse=True) + return ranked[:limit] + + +async def backfill_rule_embeddings() -> None: + """Embed rules that have no current vectors. Runs at startup beside the + note backfill; a CHUNKER_VERSION bump re-embeds rather than wiping.""" + from scribe.models.rulebook import Rule + + try: + async with async_session() as session: + current = select(RuleEmbedding.rule_id).where( + RuleEmbedding.chunker_version == CHUNKER_VERSION + ) + stale = (await session.execute( + select(Rule.id, Rule.title, Rule.statement, Rule.when_to_apply) + .where(Rule.deleted_at.is_(None), Rule.id.notin_(current)) + )).all() + except Exception: + logger.warning("Rule embedding backfill: failed to query rules", exc_info=True) + return + + if not stale: + logger.info("Rule embedding backfill: all rules current at chunker v%d", CHUNKER_VERSION) + return + logger.info("Rule embedding backfill: embedding %d rule(s)", len(stale)) + for rule_id, title, statement, when_to_apply in stale: + await upsert_rule_embedding(rule_id, title, statement, when_to_apply) diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index acdcca7..1c5cde7 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -334,6 +334,30 @@ def rule_brief(rule: Rule, **extra) -> dict: return out +def _refresh_rule_embedding(rule: Rule) -> None: + """Re-index a rule after a write. Fire-and-forget, like the note twin. + + Lazy import so this module doesn't pull in the embedder; every exception + swallowed because a rule that SAVED must not fail on its index refresh — + a stale vector costs a missed search hit, a raised exception costs the + write. No running loop (unit tests, scripts) is ordinary, not an error. + """ + try: + import asyncio + + from scribe.services.embeddings import upsert_rule_embedding + + asyncio.create_task( + upsert_rule_embedding( + rule.id, rule.title, rule.statement, rule.when_to_apply, + ) + ) + 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 rule %s", rule.id) + + async def rule_detail(user_id: int, rule: Rule, system_ids: list[int] | None = None) -> dict: """The full record, with its areas and edges attached. @@ -380,6 +404,7 @@ async def create_rule( session.add(rule) await session.commit() await session.refresh(rule) + _refresh_rule_embedding(rule) return rule @@ -410,6 +435,7 @@ async def create_project_rule( session.add(rule) await session.commit() await session.refresh(rule) + _refresh_rule_embedding(rule) return rule @@ -609,6 +635,7 @@ async def update_rule(rule_id: int, user_id: int, **fields) -> Optional[Rule]: setattr(rule, key, _valid_tier(value) if key == "tier" else value) await session.commit() await session.refresh(rule) + _refresh_rule_embedding(rule) return rule diff --git a/tests/test_services_rule_embeddings.py b/tests/test_services_rule_embeddings.py new file mode 100644 index 0000000..02e7a09 --- /dev/null +++ b/tests/test_services_rule_embeddings.py @@ -0,0 +1,84 @@ +"""The document a rule is EMBEDDED as (milestone 307 step 4, note 3026). + +This shape is measured, not chosen. Note 2485 probed the live corpus and found +the snippet was the only discriminative record in it — a 0.153 top-to-second +gap against 0.010-0.023 for everything else — and that the cause was its shape: +purpose stated twice in a short, single-topic document. These cases pin that +recipe onto rules, and pin the exclusion that matters more than any of it. +""" +import pytest + +from scribe.services.embeddings import chunk_document, rule_document + + +def test_the_trigger_appears_twice_which_is_what_makes_a_vector_sharp(): + """Repetition of purpose + brevity is the measured cause of the snippet's + separation. The rule document reproduces it exactly: the trigger in the + title, and again as the body's first line.""" + title, body = rule_document( + "Release — never without explicit request", + "Never cut a release without the operator explicitly asking.", + "before cutting any release", + ) + assert title == "Release — never without explicit request — before cutting any release" + assert body.startswith("When to apply: before cutting any release") + assert "Never cut a release" in body + + +def test_why_is_never_embedded(): + """The exclusion this whole design turns on. `why` is dated incident + narrative — rule 46's runs to 4,300 characters — and long multi-topic prose + is what made sixteen dev-logs mutually indistinguishable: the average lands + on a centroid they all share. Adding it would not give the vector more to + work with; it would give every rule the SAME thing to work with. + + rule_document takes no `why` parameter at all, which is the strongest form + of this guarantee: it cannot be passed in by a caller who means well. + """ + import inspect + + assert "why" not in inspect.signature(rule_document).parameters + + +def test_a_rule_with_no_trigger_still_embeds_just_less_sharply(): + """Every rule written before milestone 307 has no trigger. Degrading to + title + statement keeps them findable; it does NOT pad the document with + whatever text is lying around, which would be the tempting fix and the + wrong one.""" + title, body = rule_document("dev is home", "Work directly on dev.", "") + assert title == "dev is home" + assert body == "Work directly on dev." + assert "When to apply" not in body + + +def test_an_empty_rule_yields_no_document_and_therefore_no_vector(): + """Callers gate on falsiness to skip embedding — the same contract + chunk_document has, so an emptied record stops being findable by its old + content rather than keeping a stale vector.""" + assert rule_document("", "", "") == (None, None) + assert chunk_document(*rule_document("", "", "")) == [] + + +def test_a_long_rule_chunks_and_every_chunk_carries_the_trigger(): + """Rule 46's statement is ~3,600 characters across two headed sections. + The existing chunker splits it at headings and prefixes each chunk with the + title — which now CONTAINS the trigger, so each half stays anchored to what + the rule is for. This is why splitting a merged rule costs nothing at + retrieval time.""" + statement = ( + "## The tags\n\n" + ("Four tags, four jobs. " * 60) + + "\n\n## The artifact's own version\n\n" + ("Two version values. " * 60) + ) + title, body = rule_document("Versioning", statement, "when cutting a release") + chunks = chunk_document(title, body) + assert len(chunks) > 1 + assert all("when cutting a release" in chunk for chunk in chunks) + + +@pytest.mark.parametrize("trigger,expected_title", [ + ("before any git push", "dev is home — before any git push"), + (" before any git push ", "dev is home — before any git push"), +]) +def test_the_trigger_is_trimmed_before_it_reaches_the_vector(trigger, expected_title): + title, _ = rule_document("dev is home", "Work on dev.", trigger) + assert title == expected_title