bvandeusenandClaude Opus 5
5c51e29f26
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 15s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 57s
CI & Build / Build & push image (push) Successful in 29s
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs
65 lines
2.8 KiB
Python
65 lines
2.8 KiB
Python
"""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
|