From 5c51e29f269805894dc9d527c1c936b78d42d600 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 31 Jul 2026 22:46:57 -0400 Subject: [PATCH] fix(embeddings): embed in the service, so every caller gets it (#2056) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A note or task created through MCP was not semantically searchable until the next restart's backfill ran. Embedding fired at the five REST route handlers and nowhere else; the MCP tools call the service directly, so they skipped it. The shape of this bug is the reason to care: it is invisible on an instance that redeploys constantly (this one does, per rule 46) and permanent on one that doesn't. Rule 115 — the product has to stand up for the install that restarts twice a year, not just for the one that restarts hourly. Moved to services/notes.embed_note(), called from create_note and update_note, and deleted from all five routes. Every caller — REST, MCP, recurrence, snippets — now gets it by construction rather than by remembering. Two things fall out of having one implementation instead of six: - It uses note.user_id, the OWNER. The routes were inconsistent: some passed the caller's uid, some the owner's. On a shared record the caller's id mints a second embedding row that nothing reads. - services/snippets.py's _embed_snippet existed only because snippets are created via MCP and the routes couldn't cover them. Every one of its four call sites goes through notes_svc, so the helper and its four calls are gone, along with the eight test patches that existed to neutralise it. RuntimeError (no running loop — unit tests, scripts) and any indexing failure are both swallowed: a write that succeeded must not be failed by its index. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- src/scribe/routes/notes.py | 11 ----- src/scribe/routes/tasks.py | 8 ---- src/scribe/services/notes.py | 38 +++++++++++++++ src/scribe/services/snippets.py | 26 ----------- tests/test_services_notes.py | 64 ++++++++++++++++++++++++++ tests/test_shared_write_access.py | 6 +-- tests/test_snippet_merge_provenance.py | 9 ++-- tests/test_snippet_unmerge.py | 3 -- 8 files changed, 107 insertions(+), 58 deletions(-) create mode 100644 tests/test_services_notes.py diff --git a/src/scribe/routes/notes.py b/src/scribe/routes/notes.py index b70cc44..3dcca5a 100644 --- a/src/scribe/routes/notes.py +++ b/src/scribe/routes/notes.py @@ -1,8 +1,6 @@ -import asyncio import logging import re -from scribe.services.embeddings import upsert_note_embedding from quart import Blueprint, jsonify, request @@ -113,9 +111,6 @@ async def create_note_route(): ) except ValueError as e: return jsonify({"error": str(e)}), 400 - text = f"{note.title}\n{note.body}".strip() if note.body else (note.title or "") - if text: - asyncio.create_task(upsert_note_embedding(note.id, uid, text)) return jsonify(note.to_dict()), 201 @@ -221,9 +216,6 @@ async def update_note_route(note_id: int): return jsonify({"error": str(e)}), 400 if note is None: return not_found("Note") - text = f"{note.title}\n{note.body}".strip() if note.body else (note.title or "") - if text: - asyncio.create_task(upsert_note_embedding(note.id, owner_uid, text)) return jsonify(note.to_dict()) @@ -259,9 +251,6 @@ async def patch_note_route(note_id: int): return jsonify({"error": str(e)}), 400 if note is None: return not_found("Note") - text = f"{note.title}\n{note.body}".strip() if note.body else (note.title or "") - if text: - asyncio.create_task(upsert_note_embedding(note.id, owner_uid, text)) return jsonify(note.to_dict()) diff --git a/src/scribe/routes/tasks.py b/src/scribe/routes/tasks.py index c76f4f5..d564b55 100644 --- a/src/scribe/routes/tasks.py +++ b/src/scribe/routes/tasks.py @@ -1,4 +1,3 @@ -import asyncio from datetime import date from quart import Blueprint, jsonify, request @@ -8,7 +7,6 @@ from scribe.models.note import TaskPriority, TaskStatus from scribe.routes.utils import not_found, parse_iso_date, parse_pagination from scribe.services.access import can_write_note from scribe.services import systems as systems_svc -from scribe.services.embeddings import upsert_note_embedding from scribe.services.notes import ( create_note, get_note_for_user, @@ -149,9 +147,6 @@ async def create_task_route(): ) if data.get("system_ids") is not None: await systems_svc.set_record_systems(uid, task.id, data["system_ids"]) - text = f"{task.title}\n{task.body}".strip() if task.body else (task.title or "") - if text: - asyncio.create_task(upsert_note_embedding(task.id, uid, text)) out = task.to_dict() out["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task.id)] return jsonify(out), 201 @@ -255,9 +250,6 @@ async def update_task_route(task_id: int): return not_found("Task") if data.get("system_ids") is not None: await systems_svc.set_record_systems(uid, task_id, data["system_ids"]) - text = f"{task.title}\n{task.body}".strip() if task.body else (task.title or "") - if text: - asyncio.create_task(upsert_note_embedding(task.id, task_note.user_id, text)) out = task.to_dict() out["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task_id)] return jsonify(out) diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index f36d08b..1644787 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -10,6 +10,40 @@ from scribe.models.note import Note, TaskPriority, TaskStatus logger = logging.getLogger(__name__) +def embed_note(note) -> None: + """Refresh a note's embedding, fire-and-forget. + + Lives HERE — at the service, not the route — so every caller gets it by + construction. Previously each REST route made this call itself and the MCP + tools did not, so a record created through MCP stayed out of semantic search + and auto-inject until the next restart's backfill ran (#2056). That is + invisible on an instance that redeploys constantly and permanent on one that + doesn't, which is the worst shape a bug can have: it only appears where + nobody is looking. + + Uses `note.user_id` — the OWNER — rather than the caller. Embeddings belong + to the record, and a collaborator editing a shared note must refresh the + owner's row rather than mint a second one under their own id. + + Import is lazy so importing this module doesn't pull in the embedding model; + exceptions are swallowed because a record that saved must not fail on its + index refresh. No running loop (unit tests, scripts) is an ordinary case, + not an error. + """ + text = f"{note.title}\n{note.body}".strip() if note.body else (note.title or "") + if not text: + return + try: + import asyncio + + from scribe.services.embeddings import upsert_note_embedding + asyncio.create_task(upsert_note_embedding(note.id, note.user_id, text)) + 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 note %s", note.id) + + def _normalize_tags(tags: list[str]) -> list[str]: """Lowercase, strip, deduplicate, and drop empty tags.""" seen: set[str] = set() @@ -115,6 +149,8 @@ async def create_note( await session.commit() await session.refresh(note) + embed_note(note) + if project_id is not None: await _maybe_reactivate_project(project_id) @@ -329,6 +365,8 @@ async def update_note(user_id: int, note_id: int, **fields: object) -> Note | No from scribe.services.note_versions import create_version await create_version(user_id, note_id, old_body, old_title, old_tags) + embed_note(note) + if note.project_id is not None: await _maybe_reactivate_project(note.project_id) diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index fc95425..5be773a 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -28,7 +28,6 @@ came from. """ from __future__ import annotations -import asyncio import hashlib import logging import re @@ -52,25 +51,6 @@ SNIPPET_TAG = "snippet" UNSET: object = object() -def _embed_snippet(note) -> None: - """Fire-and-forget embedding refresh for a snippet. - - A snippet's whole value is *immediate* recall — it must join the semantic / - auto-inject pool the moment it's recorded, not wait for the startup backfill. - Unlike a plain note (embedded at the REST-route boundary only, so its MCP - create path defers to restart-backfill), a snippet is recorded primarily via - MCP, so we embed here in the service — covering BOTH the MCP tool and the - REST route by construction. Mirrors the route pattern: fire-and-forget, - text = title + body. Import lazily so the pure serialize/parse helpers can be - imported without pulling in the embedding model. - """ - text = f"{note.title}\n{note.body}".strip() if note.body else (note.title or "") - if not text: - return - from scribe.services.embeddings import upsert_note_embedding - asyncio.create_task(upsert_note_embedding(note.id, note.user_id, text)) - - # --- serialize: structured fields -> note (title/body/tags) ------------------ def compose_title(name: str, when_to_use: str = "") -> str: @@ -640,7 +620,6 @@ async def create_snippet( language=language, code=code, locations=locations, ), ) - _embed_snippet(note) return note @@ -801,9 +780,6 @@ async def update_snippet( # As the OWNER: update_note is owner-scoped, so a shared editor's own id # would find nothing. The write was authorised by can_write_note above. updated = await notes_svc.update_note(note.user_id, snippet_id, **fields) - if updated is not None: - # Title/body changed → refresh the embedding so recall reflects the edit. - _embed_snippet(updated) return updated @@ -1025,7 +1001,6 @@ async def merge_snippets(user_id: int, target_id: int, source_ids: list[int]): if batch is not None: merged_ids.append(s.id) - _embed_snippet(updated) return updated, merged_ids @@ -1134,5 +1109,4 @@ async def unmerge_snippet(user_id: int, survivor_id: int, source_id: int): ) if updated is None: return None - _embed_snippet(updated) return updated, restored diff --git a/tests/test_services_notes.py b/tests/test_services_notes.py new file mode 100644 index 0000000..da74997 --- /dev/null +++ b/tests/test_services_notes.py @@ -0,0 +1,64 @@ +"""services/notes.py — the inline embedding moved here from the routes (#2056). + +Why it moved: embedding was triggered at each REST route and nowhere else, so a +record created through MCP stayed out of semantic search and auto-inject until +the next restart's backfill. Putting it in the service means every caller — REST, +MCP, recurrence, snippets — gets it by construction rather than by remembering. + +These test the helper directly. The point of the change is that there is now ONE +place to test. +""" +from scribe.services import notes as notes_svc + +# --- inline embedding (#2056) ----------------------------------------------- + +def test_embed_note_uses_the_OWNER_not_the_caller(): + """LOAD-BEARING for shared records. An embedding belongs to the record; a + collaborator editing a shared note must refresh the owner's row rather than + mint a second one under their own id. The routes this replaced passed the + caller's uid on one path and the owner's on another — exactly the kind of + inconsistency that moving it to one place removes.""" + from unittest.mock import MagicMock, patch + + note = MagicMock(id=5, user_id=42, title="T", body="B") + with patch("scribe.services.embeddings.upsert_note_embedding") as upsert, \ + patch("asyncio.create_task") as create_task: + notes_svc.embed_note(note) + + assert create_task.called + upsert.assert_called_once() + assert upsert.call_args.args[0] == 5 + assert upsert.call_args.args[1] == 42 # owner, never the caller + assert upsert.call_args.args[2] == "T\nB" + + +def test_embed_note_skips_a_record_with_no_text(): + """An empty embedding is worse than none — it is a row that matches nothing + and hides the fact that the record was never indexed.""" + from unittest.mock import MagicMock, patch + + note = MagicMock(id=5, user_id=42, title="", body="") + with patch("asyncio.create_task") as create_task: + notes_svc.embed_note(note) + assert not create_task.called + + +def test_embed_note_without_a_running_loop_is_not_an_error(): + """Unit tests and scripts call create_note with no event loop. That must be + an ordinary case: a write that succeeded cannot be failed by its index + refresh.""" + from unittest.mock import MagicMock, patch + + note = MagicMock(id=5, user_id=42, title="T", body="B") + with patch("asyncio.create_task", side_effect=RuntimeError("no running loop")): + notes_svc.embed_note(note) # must not raise + + +def test_embed_note_swallows_an_indexing_failure(): + """Same reason, wider net: the embedding model being unavailable must not + turn a successful save into a 500.""" + from unittest.mock import MagicMock, patch + + note = MagicMock(id=5, user_id=42, title="T", body="B") + with patch("asyncio.create_task", side_effect=ValueError("model gone")): + notes_svc.embed_note(note) # must not raise diff --git a/tests/test_shared_write_access.py b/tests/test_shared_write_access.py index 99e498a..50c90bb 100644 --- a/tests/test_shared_write_access.py +++ b/tests/test_shared_write_access.py @@ -40,8 +40,7 @@ async def test_editor_share_can_update_and_writes_as_the_owner(): with patch.object(svc, "get_snippet", AsyncMock(return_value=note)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch.object(svc.notes_svc, "update_note", - AsyncMock(return_value=updated)) as mock_update, \ - patch.object(svc, "_embed_snippet", MagicMock()): + AsyncMock(return_value=updated)) as mock_update: got = await svc.update_snippet(7, 1, name="formatDuration") assert got is updated @@ -107,8 +106,7 @@ async def test_merge_skips_sources_owned_by_someone_else(): with patch.object(svc, "get_snippet", AsyncMock(side_effect=fake_get)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch.object(svc.notes_svc, "update_note", AsyncMock(return_value=target)), \ - patch("scribe.services.trash.delete", AsyncMock(return_value=object())), \ - patch.object(svc, "_embed_snippet", MagicMock()): + patch("scribe.services.trash.delete", AsyncMock(return_value=object())): _note, merged_ids = await svc.merge_snippets(7, 1, [2, 3]) assert merged_ids == [2] diff --git a/tests/test_snippet_merge_provenance.py b/tests/test_snippet_merge_provenance.py index cda60ab..b465f1b 100644 --- a/tests/test_snippet_merge_provenance.py +++ b/tests/test_snippet_merge_provenance.py @@ -39,8 +39,7 @@ async def _run_merge(target, sources, source_ids): patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch.object(s.notes_svc, "update_note", AsyncMock(return_value=target)) as mock_update, \ - patch("scribe.services.trash.delete", AsyncMock(return_value=object())), \ - patch.object(s, "_embed_snippet", MagicMock()): + patch("scribe.services.trash.delete", AsyncMock(return_value=object())): await s.merge_snippets(7, target.id, source_ids) return mock_update.await_args.kwargs @@ -88,8 +87,7 @@ async def test_an_ordinary_edit_carries_provenance_forward(): with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch.object(s.notes_svc, "update_note", - AsyncMock(return_value=note)) as mock_update, \ - patch.object(s, "_embed_snippet", MagicMock()): + AsyncMock(return_value=note)) as mock_update: await s.update_snippet(7, 1, signature="f(ms) -> string") kwargs = mock_update.await_args.kwargs @@ -106,8 +104,7 @@ async def test_a_pre_0070_row_keeps_provenance_through_the_body(): with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch.object(s.notes_svc, "update_note", - AsyncMock(return_value=note)) as mock_update, \ - patch.object(s, "_embed_snippet", MagicMock()): + AsyncMock(return_value=note)) as mock_update: await s.update_snippet(7, 1, when_to_use="humanize a ms count") assert "**Merged from:** #5" in mock_update.await_args.kwargs["body"] diff --git a/tests/test_snippet_unmerge.py b/tests/test_snippet_unmerge.py index 4470392..1009504 100644 --- a/tests/test_snippet_unmerge.py +++ b/tests/test_snippet_unmerge.py @@ -44,7 +44,6 @@ async def _run_unmerge(survivor, *, source_alive=None, restore=1): patch("scribe.services.trash.restore_entity", AsyncMock(return_value=restore)), patch.object(s.notes_svc, "update_note", AsyncMock(return_value=survivor)) as upd, - patch.object(s, "_embed_snippet", MagicMock()), ): await s.unmerge_snippet(7, 1, 2) return upd.await_args.kwargs @@ -127,7 +126,6 @@ async def test_a_purged_source_is_refused_and_the_survivor_is_untouched(): patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), patch("scribe.services.trash.restore_entity", AsyncMock(return_value=None)), patch.object(s.notes_svc, "update_note", AsyncMock()) as upd, - patch.object(s, "_embed_snippet", MagicMock()), ): with pytest.raises(s.UnmergeError, match="purged"): await s.unmerge_snippet(7, 1, 2) @@ -150,7 +148,6 @@ async def test_an_entry_without_attribution_is_refused_not_guessed(): patch.object(s, "get_snippet", AsyncMock(side_effect=fake_get)), patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), patch.object(s.notes_svc, "update_note", AsyncMock()) as upd, - patch.object(s, "_embed_snippet", MagicMock()), ): with pytest.raises(s.UnmergeError, match="provenance"): await s.unmerge_snippet(7, 1, 2)