Design-system delivery, the contrast fix, recall scoping, and backup coverage #93

Merged
bvandeusen merged 10 commits from dev into main 2026-07-31 23:21:25 -04:00
8 changed files with 107 additions and 58 deletions
Showing only changes of commit 5c51e29f26 - Show all commits
-11
View File
@@ -1,8 +1,6 @@
import asyncio
import logging import logging
import re import re
from scribe.services.embeddings import upsert_note_embedding
from quart import Blueprint, jsonify, request from quart import Blueprint, jsonify, request
@@ -113,9 +111,6 @@ async def create_note_route():
) )
except ValueError as e: except ValueError as e:
return jsonify({"error": str(e)}), 400 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 return jsonify(note.to_dict()), 201
@@ -221,9 +216,6 @@ async def update_note_route(note_id: int):
return jsonify({"error": str(e)}), 400 return jsonify({"error": str(e)}), 400
if note is None: if note is None:
return not_found("Note") 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()) return jsonify(note.to_dict())
@@ -259,9 +251,6 @@ async def patch_note_route(note_id: int):
return jsonify({"error": str(e)}), 400 return jsonify({"error": str(e)}), 400
if note is None: if note is None:
return not_found("Note") 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()) return jsonify(note.to_dict())
-8
View File
@@ -1,4 +1,3 @@
import asyncio
from datetime import date from datetime import date
from quart import Blueprint, jsonify, request 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.routes.utils import not_found, parse_iso_date, parse_pagination
from scribe.services.access import can_write_note from scribe.services.access import can_write_note
from scribe.services import systems as systems_svc from scribe.services import systems as systems_svc
from scribe.services.embeddings import upsert_note_embedding
from scribe.services.notes import ( from scribe.services.notes import (
create_note, create_note,
get_note_for_user, get_note_for_user,
@@ -149,9 +147,6 @@ async def create_task_route():
) )
if data.get("system_ids") is not None: if data.get("system_ids") is not None:
await systems_svc.set_record_systems(uid, task.id, data["system_ids"]) 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 = task.to_dict()
out["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task.id)] out["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task.id)]
return jsonify(out), 201 return jsonify(out), 201
@@ -255,9 +250,6 @@ async def update_task_route(task_id: int):
return not_found("Task") return not_found("Task")
if data.get("system_ids") is not None: if data.get("system_ids") is not None:
await systems_svc.set_record_systems(uid, task_id, data["system_ids"]) 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 = task.to_dict()
out["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task_id)] out["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task_id)]
return jsonify(out) return jsonify(out)
+38
View File
@@ -10,6 +10,40 @@ from scribe.models.note import Note, TaskPriority, TaskStatus
logger = logging.getLogger(__name__) 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]: def _normalize_tags(tags: list[str]) -> list[str]:
"""Lowercase, strip, deduplicate, and drop empty tags.""" """Lowercase, strip, deduplicate, and drop empty tags."""
seen: set[str] = set() seen: set[str] = set()
@@ -115,6 +149,8 @@ async def create_note(
await session.commit() await session.commit()
await session.refresh(note) await session.refresh(note)
embed_note(note)
if project_id is not None: if project_id is not None:
await _maybe_reactivate_project(project_id) 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 from scribe.services.note_versions import create_version
await create_version(user_id, note_id, old_body, old_title, old_tags) await create_version(user_id, note_id, old_body, old_title, old_tags)
embed_note(note)
if note.project_id is not None: if note.project_id is not None:
await _maybe_reactivate_project(note.project_id) await _maybe_reactivate_project(note.project_id)
-26
View File
@@ -28,7 +28,6 @@ came from.
""" """
from __future__ import annotations from __future__ import annotations
import asyncio
import hashlib import hashlib
import logging import logging
import re import re
@@ -52,25 +51,6 @@ SNIPPET_TAG = "snippet"
UNSET: object = object() 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) ------------------ # --- serialize: structured fields -> note (title/body/tags) ------------------
def compose_title(name: str, when_to_use: str = "") -> str: def compose_title(name: str, when_to_use: str = "") -> str:
@@ -640,7 +620,6 @@ async def create_snippet(
language=language, code=code, locations=locations, language=language, code=code, locations=locations,
), ),
) )
_embed_snippet(note)
return 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 # 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. # would find nothing. The write was authorised by can_write_note above.
updated = await notes_svc.update_note(note.user_id, snippet_id, **fields) 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 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: if batch is not None:
merged_ids.append(s.id) merged_ids.append(s.id)
_embed_snippet(updated)
return updated, merged_ids 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: if updated is None:
return None return None
_embed_snippet(updated)
return updated, restored return updated, restored
+64
View File
@@ -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
+2 -4
View File
@@ -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)), \ with patch.object(svc, "get_snippet", AsyncMock(return_value=note)), \
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
patch.object(svc.notes_svc, "update_note", patch.object(svc.notes_svc, "update_note",
AsyncMock(return_value=updated)) as mock_update, \ AsyncMock(return_value=updated)) as mock_update:
patch.object(svc, "_embed_snippet", MagicMock()):
got = await svc.update_snippet(7, 1, name="formatDuration") got = await svc.update_snippet(7, 1, name="formatDuration")
assert got is updated 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)), \ with patch.object(svc, "get_snippet", AsyncMock(side_effect=fake_get)), \
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
patch.object(svc.notes_svc, "update_note", AsyncMock(return_value=target)), \ patch.object(svc.notes_svc, "update_note", AsyncMock(return_value=target)), \
patch("scribe.services.trash.delete", AsyncMock(return_value=object())), \ patch("scribe.services.trash.delete", AsyncMock(return_value=object())):
patch.object(svc, "_embed_snippet", MagicMock()):
_note, merged_ids = await svc.merge_snippets(7, 1, [2, 3]) _note, merged_ids = await svc.merge_snippets(7, 1, [2, 3])
assert merged_ids == [2] assert merged_ids == [2]
+3 -6
View File
@@ -39,8 +39,7 @@ async def _run_merge(target, sources, source_ids):
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
patch.object(s.notes_svc, "update_note", patch.object(s.notes_svc, "update_note",
AsyncMock(return_value=target)) as mock_update, \ AsyncMock(return_value=target)) as mock_update, \
patch("scribe.services.trash.delete", AsyncMock(return_value=object())), \ patch("scribe.services.trash.delete", AsyncMock(return_value=object())):
patch.object(s, "_embed_snippet", MagicMock()):
await s.merge_snippets(7, target.id, source_ids) await s.merge_snippets(7, target.id, source_ids)
return mock_update.await_args.kwargs 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)), \ with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
patch.object(s.notes_svc, "update_note", patch.object(s.notes_svc, "update_note",
AsyncMock(return_value=note)) as mock_update, \ AsyncMock(return_value=note)) as mock_update:
patch.object(s, "_embed_snippet", MagicMock()):
await s.update_snippet(7, 1, signature="f(ms) -> string") await s.update_snippet(7, 1, signature="f(ms) -> string")
kwargs = mock_update.await_args.kwargs 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)), \ with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \ patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
patch.object(s.notes_svc, "update_note", patch.object(s.notes_svc, "update_note",
AsyncMock(return_value=note)) as mock_update, \ AsyncMock(return_value=note)) as mock_update:
patch.object(s, "_embed_snippet", MagicMock()):
await s.update_snippet(7, 1, when_to_use="humanize a ms count") await s.update_snippet(7, 1, when_to_use="humanize a ms count")
assert "**Merged from:** #5" in mock_update.await_args.kwargs["body"] assert "**Merged from:** #5" in mock_update.await_args.kwargs["body"]
-3
View File
@@ -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("scribe.services.trash.restore_entity", AsyncMock(return_value=restore)),
patch.object(s.notes_svc, "update_note", patch.object(s.notes_svc, "update_note",
AsyncMock(return_value=survivor)) as upd, AsyncMock(return_value=survivor)) as upd,
patch.object(s, "_embed_snippet", MagicMock()),
): ):
await s.unmerge_snippet(7, 1, 2) await s.unmerge_snippet(7, 1, 2)
return upd.await_args.kwargs 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.access.can_write_note", AsyncMock(return_value=True)),
patch("scribe.services.trash.restore_entity", AsyncMock(return_value=None)), patch("scribe.services.trash.restore_entity", AsyncMock(return_value=None)),
patch.object(s.notes_svc, "update_note", AsyncMock()) as upd, patch.object(s.notes_svc, "update_note", AsyncMock()) as upd,
patch.object(s, "_embed_snippet", MagicMock()),
): ):
with pytest.raises(s.UnmergeError, match="purged"): with pytest.raises(s.UnmergeError, match="purged"):
await s.unmerge_snippet(7, 1, 2) 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.object(s, "get_snippet", AsyncMock(side_effect=fake_get)),
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)),
patch.object(s.notes_svc, "update_note", AsyncMock()) as upd, patch.object(s.notes_svc, "update_note", AsyncMock()) as upd,
patch.object(s, "_embed_snippet", MagicMock()),
): ):
with pytest.raises(s.UnmergeError, match="provenance"): with pytest.raises(s.UnmergeError, match="provenance"):
await s.unmerge_snippet(7, 1, 2) await s.unmerge_snippet(7, 1, 2)