fix(lessons): a derived mirror survives the generic note door, by kind not by name (#3734)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m31s
CI & Build / Build & push image (push) Successful in 23s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m31s
CI & Build / Build & push image (push) Successful in 23s
Groundwork for step 7, and a data-integrity fix in its own right. Two kinds keep a queryable mirror in `notes.data` derived from their body: snippets and, since milestone 385, lessons. Every read prefers the mirror — deliberately, because parsing markdown to answer what an index can answer is how a hot path rots. So a write that moves the body must move the mirror. #3128 found that hole for snippets and plugged it with a hard-coded `if note.note_type == SNIPPET_NOTE_TYPE`. The plug was correct and did not generalise: lessons arrived with the same design and none of the protection, which is precisely the "don't add a fourth instance" defect #3734 was told to avoid. The cost is higher for a lesson. A stale snippet mirror reports the wrong path. A stale lesson mirror reports the wrong TRIGGER, and the trigger is the whole retrieval story — the lesson goes on firing for the situation it used to name while displaying the one it now names. Silent, and confident. So `update_note` now dispatches through `_mirror_recomposers()`, a note_type -> recomposer table. A kind with a derived mirror is covered by registering it, not by someone remembering to widen an if. `lessons.recompose_data` is the lesson's entry. It recovers the subject with `embeddings.untrigger_title` — new, and deliberately placed beside the join it inverts rather than in the caller that wanted it, because a separator spelled in two files is a separator that will one day be changed in one of them (#3207). `TRIGGER_SEP` is now the one spelling, and `parse_snippet_fields` uses it too; it had the third copy inline. The two inverses stay distinct on purpose: a snippet partitions at the first separator (its name is a symbol), a lesson strips an exact known suffix (its subject may legitimately contain a dash). Different algorithms, one constant, so they cannot disagree about where the seam is. Provenance is DROPPED when the body drops it, which is the opposite call from a snippet's `verification` — that is carried because it was never in the body to delete. The body is the authority; carrying a value the reader just removed is the failure the recompose exists to prevent. Tests: test_snippet_mirror_generic_door.py becomes test_derived_mirror_generic_door.py, since the concern is now plural. The registry property is asserted directly (every kind with a mirror is in the table; the dispatch names no kind inline), plus the lesson cases and the join/inverse round-trip. `fake_lesson` moves to tests/helpers.py — it existed in test_lesson_surfacing.py and a second copy was about to be written — and gains the explicit `None`s `fake_snippet` carries, because update_note reads `verify_with` and a MagicMock is truthy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
import logging
|
||||
import re
|
||||
from collections.abc import Iterable
|
||||
from collections.abc import Callable, Iterable
|
||||
from datetime import date, datetime, timezone
|
||||
|
||||
from sqlalchemy import func, or_, select, text
|
||||
@@ -11,14 +11,44 @@ from scribe.models.base import iso
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# The fields `snippets.parse_snippet_fields` reads. Writing any of them can
|
||||
# change what a snippet's derived `data` mirror should say, so update_note
|
||||
# recomposes the mirror when one moves. Kept here as a set of NAMES rather
|
||||
# than imported, because it describes update_note's own `fields` dict, not the
|
||||
# parser's signature.
|
||||
# The fields a derived `data` mirror is parsed out of. Writing any of them can
|
||||
# change what the mirror should say, so update_note recomposes it when one
|
||||
# moves. Kept here as a set of NAMES rather than imported, because it
|
||||
# describes update_note's own `fields` dict, not any parser's signature.
|
||||
_PARSED_FROM_BODY = frozenset({"title", "body", "tags"})
|
||||
|
||||
|
||||
def _mirror_recomposers() -> dict[str, Callable[[Note], dict]]:
|
||||
"""note_type -> the function that rebuilds that kind's derived `data`.
|
||||
|
||||
A TABLE rather than a chain of `if note_type == ...`, because the previous
|
||||
shape tested one constant and the next kind with a derived mirror was
|
||||
silently not covered — which is exactly what happened: the snippet guard
|
||||
(#3128) was hard-coded, and lessons arrived in milestone 385 with the same
|
||||
body-is-authority/`data`-is-mirror design and none of the protection.
|
||||
|
||||
The failure is invisible from here. Nothing raises, nothing logs; the row
|
||||
simply keeps answering queries from a mirror that no longer matches its
|
||||
body, and every surface that prefers the mirror — which is all of them, by
|
||||
design, because parsing markdown to answer what an index can answer is how
|
||||
a hot path rots — reports the old value confidently.
|
||||
|
||||
Imported inside the function, not at module scope: both services call back
|
||||
into this module (`update_snippet`/`update_lesson` -> `update_note`), so a
|
||||
top-level import is a cycle.
|
||||
"""
|
||||
from scribe.services.lessons import (
|
||||
LESSON_NOTE_TYPE, recompose_data as _lesson_mirror,
|
||||
)
|
||||
from scribe.services.snippets import (
|
||||
SNIPPET_NOTE_TYPE, recompose_data as _snippet_mirror,
|
||||
)
|
||||
return {
|
||||
SNIPPET_NOTE_TYPE: _snippet_mirror,
|
||||
LESSON_NOTE_TYPE: _lesson_mirror,
|
||||
}
|
||||
|
||||
|
||||
# Text fields where EMPTY MEANS NULL (milestone 317). The sweep's whole signal
|
||||
# is `verify_with IS NULL` = "this is a decision, there is nothing to go and
|
||||
# check". An empty string that is not NULL makes a norm look like a constraint
|
||||
@@ -604,23 +634,19 @@ async def update_note(
|
||||
# costs exactly what the sweep exists to catch.
|
||||
if note.verify_with != check_before:
|
||||
note.verified_at = None
|
||||
# A snippet's `data` is DERIVED from its body — so a write that moves
|
||||
# the body through this generic door must move the mirror with it
|
||||
# (#3128). Without this, PATCH /api/notes/<snippet_id> {body} left the
|
||||
# mirror behind, and snippet_fields PREFERS the mirror: the row went on
|
||||
# reporting its old repo/path/symbol to prior-art recall while showing
|
||||
# its new body. `update_snippet` composes the mirror itself and passes
|
||||
# it explicitly, so an explicit `data` always wins — the caller that
|
||||
# knows the field set beats the one that can only re-read the body.
|
||||
# Some kinds derive `data` from their body — so a write that moves the
|
||||
# body through this generic door must move the mirror with it (#3128).
|
||||
# Without this, PATCH /api/notes/<id> {body} left the mirror behind,
|
||||
# and every read PREFERS the mirror: a snippet went on reporting its
|
||||
# old repo/path/symbol to prior-art recall while showing its new body,
|
||||
# and a lesson would go on being retrieved for the situation it used to
|
||||
# name. The kind's own updater composes the mirror itself and passes it
|
||||
# explicitly, so an explicit `data` always wins — the caller that knows
|
||||
# the field set beats the one that can only re-read the body.
|
||||
if "data" not in fields and not _PARSED_FROM_BODY.isdisjoint(fields):
|
||||
# Imported here, not at module scope: services/snippets.py calls
|
||||
# back into this module (update_snippet -> update_note), so a
|
||||
# top-level import is a cycle.
|
||||
from scribe.services.snippets import (
|
||||
SNIPPET_NOTE_TYPE, recompose_data,
|
||||
)
|
||||
if note.note_type == SNIPPET_NOTE_TYPE:
|
||||
note.data = recompose_data(note)
|
||||
recompose = _mirror_recomposers().get(note.note_type or "")
|
||||
if recompose is not None:
|
||||
note.data = recompose(note)
|
||||
# Auto-set lifecycle timestamps on status transitions
|
||||
if "status" in fields:
|
||||
_now = datetime.now(timezone.utc)
|
||||
|
||||
Reference in New Issue
Block a user