diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 8734ae4..c29baed 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -203,6 +203,12 @@ def embedding_text(title: str | None, body: str | None) -> str: return f"{title}\n{body}".strip() if body else title +# The join between a situation-keyed record's subject and its trigger. A +# CONSTANT because `untrigger_title` below has to spell the same thing to undo +# it, and two literals that must match are one edit away from not matching. +TRIGGER_SEP = " — " + + def trigger_title(subject: str | None, trigger: str | None) -> str: """`{subject} — {trigger}` — the title half of a situation-keyed document. @@ -226,10 +232,37 @@ def trigger_title(subject: str | None, trigger: str | None) -> str: subject = (subject or "").strip() trigger = (trigger or "").strip() if subject and trigger: - return f"{subject} — {trigger}" + return f"{subject}{TRIGGER_SEP}{trigger}" return subject or trigger +def untrigger_title(title: str | None, trigger: str | None) -> str: + """The subject back out of a `trigger_title` — the inverse of the join. + + Kept HERE, beside the join, for the reason the join itself was + consolidated: a separator spelled in two files is a separator that will one + day be changed in one of them. #3207 records the shape — derive it before + the third copy — and an inverse written in a caller is that third copy + wearing a different name. + + Needs the trigger passed in rather than guessing at the separator, because + a subject may legitimately contain an em dash. Given the trigger, the + suffix is exact and the split cannot be wrong. + + Degrades to the whole title when the suffix is absent — a record written + before the join existed, or one with no trigger yet, still answers with + something a human recognises rather than with "". + """ + title = (title or "").strip() + trigger = (trigger or "").strip() + if not trigger: + return title + suffix = f"{TRIGGER_SEP}{trigger}" + if title.endswith(suffix): + return title[: -len(suffix)].strip() + return title + + # --- chunking (#280): the document shape ------------------------------------ # # bge-small reads at most 512 tokens and fastembed silently truncates the rest, diff --git a/src/scribe/services/lessons.py b/src/scribe/services/lessons.py index bb8e8a0..29a7ce4 100644 --- a/src/scribe/services/lessons.py +++ b/src/scribe/services/lessons.py @@ -317,6 +317,46 @@ def compose_data( return data +def recompose_data(note) -> dict: + """Rebuild a lesson's `data` mirror from its own title and body. + + For the GENERIC note door. `update_lesson` composes the mirror itself from + the merged field set and never needs this; a plain `update_note(body=...)` + has no idea the mirror exists and would leave it behind. + + THE COST OF LEAVING IT BEHIND IS HIGHER HERE THAN FOR A SNIPPET. A stale + snippet mirror reports the wrong path. A stale lesson mirror reports the + wrong TRIGGER — and `lesson_trigger` prefers the mirror, so the lesson goes + on being retrieved for the situation it used to name while displaying the + one it now names. The trigger is the entire retrieval story (step 3), so + that is not a degraded record; it is a record that fires at the wrong + moment and looks right when it does. + + The body is the authority and the mirror is derived — already this file's + rule. This is its enforcement on the path that bypasses `update_lesson`. + + The subject comes back out of the title through `untrigger_title`, the + inverse of the join that composed it, rather than by splitting on a + separator spelled a second time here. + """ + from scribe.services.embeddings import untrigger_title + + body = getattr(note, "body", None) or "" + trigger_match = _BODY_TRIGGER_RE.search(body) + trigger = trigger_match.group(1).strip() if trigger_match else "" + what = untrigger_title(getattr(note, "title", None), trigger) + # Sources through the normal read, which already falls back body → + # arose_from_id. A body edit that drops the provenance line should drop + # the mirror's copy too: the body is the authority, and carrying a value + # the reader just deleted is the failure this function exists to prevent. + sources_match = _BODY_SOURCES_RE.search(body) + sources = ( + normalize_sources(_ID_RE.findall(sources_match.group(1))) + if sources_match else [] + ) + return compose_data(what, trigger, sources) + + async def create_lesson( user_id: int, *, diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 758e963..7a21123 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -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/ {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/ {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) diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index c36067b..2873f5d 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -275,9 +275,21 @@ def parse_snippet_fields( ``locations`` is a list of {repo,path,symbol}; ``repo``/``path``/``symbol`` mirror the FIRST location for back-compat with the single-location callers.""" + from scribe.services.embeddings import TRIGGER_SEP + title = title or "" body = body or "" - name, _, when_from_title = title.partition(" — ") + # The inverse of `embeddings.trigger_title`, at the SEPARATOR it composed + # with — imported rather than spelled again, because a separator written + # in two files is a separator that will one day be changed in one of them. + # + # `partition` rather than the `untrigger_title` a lesson uses: that one is + # handed the trigger and strips an exact suffix, which a lesson needs + # because its subject may legitimately contain a dash. A snippet's name is + # a symbol, so the first separator is the right split and no trigger has + # to be known in advance. Two inverses, suited to their callers; one + # constant, so they cannot disagree about where the seam is. + name, _, when_from_title = title.partition(TRIGGER_SEP) fields = { "name": name.strip(), "when_to_use": when_from_title.strip(), diff --git a/tests/helpers.py b/tests/helpers.py index d176e2d..c68f5da 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -191,6 +191,34 @@ def fake_snippet(**attrs) -> MagicMock: }, attrs) +def fake_lesson(**attrs) -> MagicMock: + """A stand-in lesson: a note whose `note_type` is what makes it one. + + The title carries the trigger because `compose_title` builds it that way — + `{what} — {when it applies}` — so a menu line rendering only the title is + already showing the reader when this lesson applies. Tests that used a bare + title here would be testing a record the product cannot create. + + The check fields and `arose_from_id` are explicitly None for the reason + `fake_snippet`'s `data` is: `update_note` reads `verify_with` and + `expires_when` to decide whether to run the check-field guard, and an + auto-created MagicMock attribute is truthy — so a default lesson driven + through the update path would take a branch no real record takes. + """ + attrs.setdefault( + "title", + "Give absolutely-positioned siblings an explicit stacking order — " + "placing two absolutely-positioned elements in the same area", + ) + attrs.setdefault("data", {"when_to_apply": "two absolute siblings overlap"}) + attrs.setdefault("status", None) + attrs.setdefault("arose_from_id", None) + attrs.setdefault("verify_with", None) + attrs.setdefault("expires_when", None) + attrs.setdefault("verified_at", None) + return fake_note(note_type="lesson", **attrs) + + def fake_project(**attrs) -> MagicMock: """design_system_id is explicit: a truthy auto-attribute would route every project through the design-system branch and out to a real database.""" diff --git a/tests/test_derived_mirror_generic_door.py b/tests/test_derived_mirror_generic_door.py new file mode 100644 index 0000000..7d70e68 --- /dev/null +++ b/tests/test_derived_mirror_generic_door.py @@ -0,0 +1,298 @@ +"""A DERIVED `data` mirror survives the GENERIC note door. + +Some kinds store a queryable mirror in `notes.data` that is derived from the +body: a snippet (name, when_to_use, locations…) and a lesson (what, the +trigger, what taught it). Their own updaters compose it from the field set they +just merged, so those were never the problem — the problem is every other way +the body can be written. `update_note` is a `hasattr` loop, and both doors +reach it: PATCH /api/notes/ and the MCP update_note tool. The Knowledge +feed hands you that path, because a card there routes to /notes/:id. + +The failure is silent and the wrong way round, because every read PREFERS the +mirror — deliberately, since parsing markdown to answer what an index can +answer is how a hot path rots. + + - A SNIPPET went on reporting its old repo/path/symbol to the location + reverse lookup and to prior-art recall while displaying its new body: a + record surfaced with full authority and wrong, which the drift-check + docstring calls worse than having no record at all (#3128). + + - A LESSON is worse. Its mirror holds the TRIGGER, and the trigger is the + entire retrieval story — a stale one keeps the lesson firing for the + situation it used to name while it displays the one it now names. + +#3128's fix was correct and did not generalise: it tested one constant, so +milestone 385's lesson arrived with the same design and none of the protection. +The registry tests below assert the PROPERTY — every kind with a derived mirror +is registered — so a third kind fails here rather than shipping quiet (#3734). +""" +import inspect + +import pytest +from tests.helpers import drive_update_note as _update +from tests.helpers import fake_lesson, fake_note, fake_snippet + +OLD_MIRROR = { + "name": "debounce", + "language": "javascript", + "locations": [{"repo": "Scribe", "path": "old/place.js", "symbol": "debounce"}], + "verification": {"status": "ok", "code_sha": "abc", "checked_at": "2026-01-01"}, + "provenance": {"commit_sha": "deadbeef"}, +} + +MOVED_BODY = ( + "**Locations:**\n" + "- `Scribe` · `new/place.ts` · `debounce`\n\n" + "```typescript\nexport const debounce = 1;\n```\n" +) + + +@pytest.mark.asyncio +async def test_a_body_write_moves_the_mirror_with_it(): + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, body=MOVED_BODY) + assert note.data["locations"] == [ + {"repo": "Scribe", "path": "new/place.ts", "symbol": "debounce"} + ], "the mirror still describes where the snippet used to live" + assert note.data["language"] == "typescript" + + +@pytest.mark.asyncio +async def test_the_verdict_and_provenance_are_carried_not_dropped(): + """Neither is in the body to parse, so recomposing must carry them. An + ordinary edit must not erase the last drift check — and it needs no + invalidation branch either: `code_sha` is recomputed from the new code, so + a verdict stamped against the old code expires itself on read.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, body=MOVED_BODY) + assert note.data["verification"] == OLD_MIRROR["verification"] + assert note.data["provenance"] == OLD_MIRROR["provenance"] + assert note.data["code_sha"] != OLD_MIRROR["verification"]["code_sha"] + + +@pytest.mark.asyncio +async def test_an_explicit_data_wins_over_recomposition(): + """`update_snippet` composes the mirror from the merged field set it holds + and passes it here. That caller knows things the body cannot be re-read for + — which locations were replaced, whether provenance survives the edit — so + an explicit mirror must not be recomputed out from under it.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + authoritative = {"name": "from the service", "locations": []} + await _update(note, body=MOVED_BODY, data=authoritative) + assert note.data == authoritative + + +@pytest.mark.asyncio +async def test_a_plain_note_is_left_alone(): + """Only snippets carry a mirror; a note's `data` must not be invented.""" + note = fake_note(note_type="note", data=None, project_id=None) + await _update(note, body="just some prose") + assert note.data is None + + +@pytest.mark.asyncio +async def test_a_write_that_cannot_change_the_parse_does_not_touch_the_mirror(): + """Status, priority, project — none of them is an input to the body parser, + so recomposing on them would be work for nothing and would rebuild a mirror + from a body nobody claimed to have changed.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, project_id=4) + assert note.data == OLD_MIRROR + + +@pytest.mark.asyncio +async def test_a_title_change_reaches_the_mirror_too(): + """A snippet's NAME lives in its title, not its body — `parse_snippet_fields` + reads both, so both are triggers.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, title="throttle — cap a callback's rate") + assert note.data["name"] == "throttle" + assert note.data["when_to_use"] == "cap a callback's rate" + + +# ── the registry, rather than a chain of ifs ───────────────────────────────── + + +def test_every_kind_with_a_derived_mirror_is_registered(): + """The load-bearing one. Before #3734 this was a single `if` naming + snippets, and the kind added next was simply not covered.""" + from scribe.services.lessons import LESSON_NOTE_TYPE + from scribe.services.notes import _mirror_recomposers + from scribe.services.snippets import SNIPPET_NOTE_TYPE + + table = _mirror_recomposers() + assert SNIPPET_NOTE_TYPE in table, "the #3128 fix was lost" + assert LESSON_NOTE_TYPE in table, ( + "a lesson's `data` holds its trigger and `lesson_trigger` prefers it, " + "so a body edit through the generic door would leave the lesson being " + "retrieved for a situation it no longer names (#3734)" + ) + + +def test_the_dispatch_is_a_lookup_not_a_named_kind(): + """Asserted on structure (rule 167). A lookup extends in one line in one + place; naming a kind inline is the shape that left lessons uncovered.""" + from scribe.services import notes as notes_module + + src = inspect.getsource(notes_module.update_note) + assert "_mirror_recomposers()" in src + assert "SNIPPET_NOTE_TYPE" not in src, ( + "update_note names one kind again — that is the shape #3734 replaced" + ) + + +def test_each_recomposer_takes_the_note_and_nothing_else(): + """A registry entry with the wrong signature fails inside a generic PATCH, + which is the one moment nobody is watching.""" + from scribe.services.notes import _mirror_recomposers + + for kind, fn in _mirror_recomposers().items(): + assert callable(fn), f"{kind} maps to something not callable" + assert len(inspect.signature(fn).parameters) == 1, kind + + +# ── a lesson's mirror moves with its body ──────────────────────────────────── + + +NEW_TRIGGER = "a CI run has sat in_progress far longer than its suite takes" + + +def _lesson_body(trigger, insight="Read the job log.", sources=None): + from scribe.services.lessons import compose_body + return compose_body(insight, trigger, sources) + + +@pytest.mark.asyncio +async def test_a_body_write_moves_a_lessons_trigger_with_it(): + from scribe.services.lessons import TRIGGER_KEY, compose_title + + what = "Read the job log before waiting longer" + note = fake_lesson( + title=compose_title(what, NEW_TRIGGER), + data={TRIGGER_KEY: "a CI run is slow", "what": what}, + project_id=None, + ) + await _update(note, body=_lesson_body(NEW_TRIGGER)) + assert note.data[TRIGGER_KEY] == NEW_TRIGGER, ( + "the mirror kept the old trigger — the lesson would still be retrieved " + "for the situation it no longer names" + ) + assert note.data["what"] == what + + +@pytest.mark.asyncio +async def test_a_subject_containing_an_em_dash_still_splits(): + """Why `untrigger_title` is given the trigger instead of splitting on the + separator: a subject may legitimately contain one.""" + from scribe.services.lessons import TRIGGER_KEY, compose_title + + what = "A wait with no deadline — the shape, not the symptom" + trigger = "you are about to await something crossing a process boundary" + note = fake_lesson( + title=compose_title(what, trigger), data=None, project_id=None, + ) + await _update(note, body=_lesson_body(trigger)) + assert note.data["what"] == what + assert note.data[TRIGGER_KEY] == trigger + + +@pytest.mark.asyncio +async def test_dropping_the_provenance_line_drops_it_from_the_mirror(): + """The body is the authority. Carrying a value the reader just deleted is + the failure this recompose exists to prevent, not a courtesy — the + opposite call from a snippet's `verification`, which is carried because it + was never in the body to delete.""" + from scribe.services.lessons import SOURCES_KEY, compose_title + + note = fake_lesson( + title=compose_title("Something learned", "a situation"), + data={SOURCES_KEY: [999]}, + project_id=None, + ) + await _update(note, body=_lesson_body("a situation")) # no Learned from: + assert SOURCES_KEY not in note.data + + +@pytest.mark.asyncio +async def test_an_explicit_data_wins_for_a_lesson_too(): + """`update_lesson` composes the mirror from the merged field set and passes + it here; that caller knows things a re-read of the body cannot recover.""" + from scribe.services.lessons import TRIGGER_KEY + + note = fake_lesson(data={TRIGGER_KEY: "old"}, project_id=None) + authoritative = {TRIGGER_KEY: "from the service", "what": "x"} + await _update(note, body=_lesson_body(NEW_TRIGGER), data=authoritative) + assert note.data == authoritative + + +@pytest.mark.asyncio +async def test_a_lesson_title_change_reaches_the_mirror(): + """A lesson's subject lives in its title, so a title edit is a trigger for + recomposition exactly as it is for a snippet's name.""" + from scribe.services.lessons import TRIGGER_KEY, compose_title + + trigger = "two absolute siblings overlap" + note = fake_lesson( + body=_lesson_body(trigger), + data={TRIGGER_KEY: trigger, "what": "the old subject"}, + project_id=None, + ) + await _update(note, title=compose_title("the new subject", trigger)) + assert note.data["what"] == "the new subject" + assert note.data[TRIGGER_KEY] == trigger + + +# ── the join and its inverse ───────────────────────────────────────────────── + + +@pytest.mark.parametrize( + ("subject", "trigger"), + [ + ("a subject", "a trigger"), + ("a subject — with a dash", "a trigger"), + ("a subject", ""), + ("", "a trigger"), + ("a subject", "a trigger — with a dash"), + ], +) +def test_untrigger_title_inverts_trigger_title(subject, trigger): + """#3207's shape: the join had three copies before it was consolidated, so + its inverse lives beside it rather than in whichever caller wanted it.""" + from scribe.services.embeddings import trigger_title, untrigger_title + + title = trigger_title(subject, trigger) + assert untrigger_title(title, trigger) == (subject or trigger).strip() + + +def test_untrigger_title_degrades_to_the_whole_title(): + """A record written before the join existed still answers with something a + human recognises rather than with "".""" + from scribe.services.embeddings import untrigger_title + + assert untrigger_title("a plain old title", "") == "a plain old title" + assert untrigger_title("a plain old title", "a trigger it lacks") == ( + "a plain old title" + ) + + +def test_the_trigger_separator_is_spelled_in_exactly_one_place(): + """Two literals that must match are one edit away from not matching.""" + import pathlib + + root = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe" + offenders = [ + str(p.relative_to(root)) for p in root.rglob("*.py") + if '" \u2014 "' in p.read_text() and p.name != "embeddings.py" + ] + assert not offenders, ( + f"the trigger separator is spelled inline in {offenders} — use " + f"TRIGGER_SEP, trigger_title or untrigger_title (#3207)" + ) + + +def test_the_mirror_guards_can_fail(): + """Rule 167: shown turning red once.""" + from scribe.services.lessons import TRIGGER_KEY, recompose_data + + bare = fake_lesson(title="just a title", body="no composed lines", data=None) + assert TRIGGER_KEY not in recompose_data(bare) diff --git a/tests/test_lesson_surfacing.py b/tests/test_lesson_surfacing.py index 238e4ad..05f27b1 100644 --- a/tests/test_lesson_surfacing.py +++ b/tests/test_lesson_surfacing.py @@ -22,7 +22,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from scribe.services import plugin_context as pc from scribe.services.lessons import LESSON_NOTE_TYPE -from tests.helpers import fake_note, writepath_cfg +from tests.helpers import fake_lesson, fake_note, writepath_cfg pytestmark = pytest.mark.usefixtures("_no_supersession") @@ -33,23 +33,6 @@ _CFG = {"enabled": True, "threshold": 0.55, "top_k": 3} _BINDING_PHRASE = "before deciding it does not apply" -def fake_lesson(**attrs): - """A stand-in lesson: a note whose `note_type` is what makes it one. - - The title carries the trigger because `compose_title` builds it that way — - `{what} — {when it applies}` — so a menu line rendering only the title is - already showing the reader when this lesson applies. Tests that used a bare - title here would be testing a record the product cannot create. - """ - attrs.setdefault( - "title", - "Give absolutely-positioned siblings an explicit stacking order — " - "placing two absolutely-positioned elements in the same area", - ) - attrs.setdefault("data", {"when_to_apply": "two absolute siblings overlap"}) - return fake_note(note_type=LESSON_NOTE_TYPE, **attrs) - - async def _menu(main_hits, *, lesson_hits=None, reuse_hits=None, cfg=None, exclude_ids=None, rec=None, surf=None): """Run the prompt menu with each query stubbed by the kinds it asks for.""" diff --git a/tests/test_snippet_mirror_generic_door.py b/tests/test_snippet_mirror_generic_door.py deleted file mode 100644 index 30af5e2..0000000 --- a/tests/test_snippet_mirror_generic_door.py +++ /dev/null @@ -1,95 +0,0 @@ -"""A snippet's `data` mirror survives the GENERIC note door. - -`notes.data` is derived from the body. The snippet service always composed it -from the field set it had just merged, so `update_snippet` was never the -problem — the problem was every other way a snippet's body could be written. -`update_note` is a `hasattr` loop with no snippet awareness, and both doors -reach it: PATCH /api/notes/ and the MCP update_note tool. The Knowledge -feed handed you that path, because a snippet card there routed to /notes/:id. - -The failure was silent and the wrong way round: `snippet_fields` PREFERS the -mirror, so the row went on reporting its old repo/path/symbol to the location -reverse lookup and to prior-art recall while displaying its new body — a record -surfaced with full authority and wrong, which the drift-check docstring calls -worse than having no record at all (#3128). -""" -import pytest -from tests.helpers import drive_update_note as _update -from tests.helpers import fake_note, fake_snippet - -OLD_MIRROR = { - "name": "debounce", - "language": "javascript", - "locations": [{"repo": "Scribe", "path": "old/place.js", "symbol": "debounce"}], - "verification": {"status": "ok", "code_sha": "abc", "checked_at": "2026-01-01"}, - "provenance": {"commit_sha": "deadbeef"}, -} - -MOVED_BODY = ( - "**Locations:**\n" - "- `Scribe` · `new/place.ts` · `debounce`\n\n" - "```typescript\nexport const debounce = 1;\n```\n" -) - - -@pytest.mark.asyncio -async def test_a_body_write_moves_the_mirror_with_it(): - note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) - await _update(note, body=MOVED_BODY) - assert note.data["locations"] == [ - {"repo": "Scribe", "path": "new/place.ts", "symbol": "debounce"} - ], "the mirror still describes where the snippet used to live" - assert note.data["language"] == "typescript" - - -@pytest.mark.asyncio -async def test_the_verdict_and_provenance_are_carried_not_dropped(): - """Neither is in the body to parse, so recomposing must carry them. An - ordinary edit must not erase the last drift check — and it needs no - invalidation branch either: `code_sha` is recomputed from the new code, so - a verdict stamped against the old code expires itself on read.""" - note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) - await _update(note, body=MOVED_BODY) - assert note.data["verification"] == OLD_MIRROR["verification"] - assert note.data["provenance"] == OLD_MIRROR["provenance"] - assert note.data["code_sha"] != OLD_MIRROR["verification"]["code_sha"] - - -@pytest.mark.asyncio -async def test_an_explicit_data_wins_over_recomposition(): - """`update_snippet` composes the mirror from the merged field set it holds - and passes it here. That caller knows things the body cannot be re-read for - — which locations were replaced, whether provenance survives the edit — so - an explicit mirror must not be recomputed out from under it.""" - note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) - authoritative = {"name": "from the service", "locations": []} - await _update(note, body=MOVED_BODY, data=authoritative) - assert note.data == authoritative - - -@pytest.mark.asyncio -async def test_a_plain_note_is_left_alone(): - """Only snippets carry a mirror; a note's `data` must not be invented.""" - note = fake_note(note_type="note", data=None, project_id=None) - await _update(note, body="just some prose") - assert note.data is None - - -@pytest.mark.asyncio -async def test_a_write_that_cannot_change_the_parse_does_not_touch_the_mirror(): - """Status, priority, project — none of them is an input to the body parser, - so recomposing on them would be work for nothing and would rebuild a mirror - from a body nobody claimed to have changed.""" - note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) - await _update(note, project_id=4) - assert note.data == OLD_MIRROR - - -@pytest.mark.asyncio -async def test_a_title_change_reaches_the_mirror_too(): - """A snippet's NAME lives in its title, not its body — `parse_snippet_fields` - reads both, so both are triggers.""" - note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) - await _update(note, title="throttle — cap a callback's rate") - assert note.data["name"] == "throttle" - assert note.data["when_to_use"] == "cap a callback's rate"