Merge pull request #179 from bvandeusen/dev

fix(lessons): a multi-paragraph trigger no longer grows a copy of itself on every edit (#4272)

src/scribe/services/lessons.py

@@ -436,7 +436,7 @@ def lesson_to_dict(note) -> dict:
         "what": (note.data or {}).get("what", "") if isinstance(note.data, dict) else "",
         "when_to_apply": lesson_trigger(note),
         "learned_from": lesson_sources(note),
-        "insight": _strip_composed_lines(note.body),
+        "insight": lesson_insight(note),
         "tags": list(note.tags or []),
         "project_id": note.project_id,
         "note_type": note.note_type,
@@ -569,7 +569,7 @@ async def update_lesson(
         else normalize_sources(learned_from)
     )
     if insight is None:
-        insight = _strip_composed_lines(note.body)
+        insight = lesson_insight(note)
 
     title, body = lesson_document(
         merged_what, merged_trigger, insight, merged_sources,
@@ -585,17 +585,43 @@ async def update_lesson(
     return await notes_svc.update_note(user_id, lesson_id, **fields)
 
 
-def _strip_composed_lines(body: str | None) -> str:
+def lesson_insight(note) -> str:
-    """The insight alone — the body with the lines `compose_body` wrote removed.
+    """The authored insight alone — the body with what `compose_body` added removed.
 
-    An update that keeps the insight has to hand it back to `compose_body`,
+    An update that keeps the insight hands it back to `compose_body`, which
-    which will re-add the trigger and provenance lines. Without this the two
+    re-adds the trigger and provenance. So this has to remove EXACTLY what was
-    composed lines accumulate a copy per edit, and since the trigger line is
+    added; anything it leaves behind is re-prepended and the body grows by that
-    half of what makes the document rank, the duplicates would look like the
+    much on every edit.
-    shape working rather than a bug.
+
+    TAKES THE NOTE, NOT THE BODY, and that is the whole fix (#4272). The
+    previous shape filtered out lines matching `_BODY_TRIGGER_RE`, which is
+    `re.M` and therefore matches ONE LINE. `when_to_apply` is free text and is
+    routinely several paragraphs — `compose_body` writes all of it after the
+    marker, so paragraphs 2..n were ordinary lines the filter never touched.
+    They came back as part of "the insight", the full trigger was prepended
+    again, and the record gained (n-1) paragraphs per update. Silent, and
+    compounding.
+
+    What makes an exact strip possible is that the trigger is stored: the
+    mirror in `data` holds its full text, so `composed` below is
+    byte-for-byte what `compose_body` put at the head, and removing it is
+    subtraction rather than pattern-matching.
+
+    The fallback is not dead code. A row whose mirror is missing degrades to
+    the old line-wise removal, which is right for a one-paragraph trigger and
+    is the best that can be done without knowing where the trigger ends —
+    there is no marker for its last line, which is why guessing was wrong.
+
+    The provenance line needs no such care: `compose_body` builds it by
+    joining ids with ", ", so it cannot contain a newline.
     """
-    kept = [
+    body = getattr(note, "body", None) or ""
-        line for line in (body or "").splitlines()
+    trigger = lesson_trigger(note).strip()
-        if not _BODY_TRIGGER_RE.match(line) and not _BODY_SOURCES_RE.match(line)
+    head = body.lstrip("\n")
-    ]
+    composed = f"**When to apply:** {trigger}" if trigger else ""
+    if composed and head.startswith(composed):
+        lines = head[len(composed):].splitlines()
+    else:
+        lines = [ln for ln in body.splitlines() if not _BODY_TRIGGER_RE.match(ln)]
+    kept = [ln for ln in lines if not _BODY_SOURCES_RE.match(ln)]
     return "\n".join(kept).strip()

tests/test_lesson_insight_round_trip.py

@@ -0,0 +1,230 @@
+"""Reading a lesson's insight back and re-composing it returns the same body (#4272).
+
+THE PROPERTY. `update_lesson` is field-wise: an edit that touches only
+`system_ids`, or only `what`, passes `insight=None`, and the service then
+derives the insight from the stored body and hands it back to
+`compose_body`, which re-adds the trigger and the provenance line. So the
+derivation and the composition are inverses, and if they are not, EVERY edit
+that does not restate the insight rewrites the record slightly wrong.
+
+    lesson_document(what, lesson_trigger(n), lesson_insight(n), lesson_sources(n))
+        == (n.title, n.body)
+
+That is the whole of what these tests assert, and it is the shape of guard
+worth having wherever a record is stored composed and edited in parts: not
+"does the strip remove the marker" — it did — but "does an edit that changes
+nothing change nothing".
+
+WHAT WENT WRONG. The derivation used to filter out body LINES matching the
+trigger marker's pattern, which is `re.M` and therefore matches one line.
+`when_to_apply` is free text and is routinely several paragraphs; the
+composer writes all of it after the marker, so paragraphs 2..n were ordinary
+lines that the filter had no reason to touch. They came back as "the
+insight", the full trigger was prepended again, and the stored lesson gained
+(n-1) paragraphs per update — silently, compounding, and in the half of the
+document that does the retrieving.
+
+Measured on the one real row this damaged (lesson #4207, a three-paragraph
+trigger): the body grew by exactly 607 characters per edit — the length of
+paragraphs 2 and 3 with their separators — taking it from 1291 characters to
+7361 over ten edits, at which point the trigger appears eleven times and the
+insight is 17% of the record.
+
+WHY A MARKER COULD NOT HAVE FIXED IT. There is no end-of-trigger marker in
+the body and adding one would not repair a row already written without it.
+What makes an exact strip possible is that the trigger is MIRRORED in
+`data[TRIGGER_KEY]`, so the composed head can be reconstructed byte for byte
+and removed by subtraction rather than matched by pattern. The line-wise
+removal survives as the fallback for a row with no mirror, where it is right
+for a one-paragraph trigger and is the best available without one — see
+`test_a_stale_mirror_falls_back_rather_than_cutting_the_wrong_thing`.
+
+KNOWN AND UNCHANGED: a line inside the insight that itself begins
+`**Learned from:**` is absorbed into the provenance line, as it always was.
+That is a different defect from this one and is not addressed here.
+"""
+from __future__ import annotations
+
+import pytest
+
+from scribe.services import lessons as lessons_svc
+from tests.helpers import fake_lesson
+
+WHAT = "A column written once at create time is a snapshot, not a field"
+
+ONE_LINE = "whenever a stored value is written only on the insert path"
+
+# The shape that broke, reduced. Three paragraphs, ordinary prose, nothing
+# unusual about it — which is the point: this is what a real trigger looks
+# like once someone writes a good one.
+MANY_PARAGRAPHS = (
+    "Whenever a stored field is written only on the insert path — the tell is "
+    "a write guarded by `if isNew` with no setter anywhere.\n\n"
+    "Also whenever the value is captured BEFORE the thing it describes is "
+    "fully known: an async metadata fetch, a webhook that fills in details "
+    "later, a placeholder from a search result.\n\n"
+    "Finally: whenever a sync reports a steady bucket of records it \"can't "
+    "find\", and the records turn out to be healthy when you look at them."
+)
+
+INSIGHT = (
+    "A write-once column is a snapshot of a moment. A field is a current "
+    "fact.\n\n"
+    "## The lesson\n\n"
+    "Decide which you meant, and if it is a field, give it a writer that runs "
+    "when the fact changes."
+)
+
+# (name, trigger, insight, sources)
+SHAPES = [
+    ("one-line trigger", ONE_LINE, INSIGHT, [4219, 4211]),
+    ("multi-paragraph trigger", MANY_PARAGRAPHS, INSIGHT, [4219, 4211, 4199]),
+    ("multi-paragraph trigger, no sources", MANY_PARAGRAPHS, INSIGHT, []),
+    ("no trigger at all", "", INSIGHT, [4219]),
+    ("single-line insight", ONE_LINE, "Check the inputs agree.", [4199]),
+    # The boundary case: the insight OPENS with the same words the trigger
+    # CLOSES with. An exact prefix removal is right here and a "cut up to the
+    # last matching paragraph" heuristic is not.
+    ("insight echoes the trigger's tail", "alpha\n\nbeta", "beta, and more besides.", []),
+]
+IDS = [s[0] for s in SHAPES]
+
+
+def _stored(trigger: str, insight: str, sources: list[int]):
+    """A lesson exactly as `create_lesson` writes it — title, body and mirror
+    composed together, which is the only way a real row is ever written.
+
+    `fake_lesson` rather than `fake_note`, and the difference is load-bearing
+    here: `lesson_sources` falls back to `arose_from_id`, which on a bare
+    MagicMock auto-creates truthy and coerces to `1`. A no-sources shape then
+    recomposes with `**Learned from:** #1` and the round trip fails for a
+    reason that has nothing to do with the trigger. `fake_lesson` pins that
+    field to None — note #2109's point, which is why the fixture exists.
+    """
+    title, body = lessons_svc.lesson_document(WHAT, trigger, insight, sources)
+    data = lessons_svc.compose_data(WHAT, trigger, sources)
+    return fake_lesson(title=title, body=body, data=data)
+
+
+def _edit(note):
+    """One no-op edit, along `update_lesson`'s own path when `insight is None`
+    and no other field is given: every part is read back from the row and
+    re-composed."""
+    return lessons_svc.lesson_document(
+        (note.data or {}).get("what", WHAT),
+        lessons_svc.lesson_trigger(note),
+        lessons_svc.lesson_insight(note),
+        lessons_svc.lesson_sources(note),
+    )
+
+
+@pytest.mark.parametrize("_name,trigger,insight,sources", SHAPES, ids=IDS)
+def test_the_derived_insight_is_the_authored_insight(_name, trigger, insight, sources):
+    """The direct claim, before any round trip: what comes back out is what
+    went in. Asserted separately from the round trip because a derivation can
+    be self-consistently wrong — strip too much and re-composing too little
+    still balances."""
+    note = _stored(trigger, insight, sources)
+    assert lessons_svc.lesson_insight(note) == insight.strip()
+
+
+@pytest.mark.parametrize("_name,trigger,insight,sources", SHAPES, ids=IDS)
+def test_an_edit_that_changes_nothing_changes_nothing(_name, trigger, insight, sources):
+    """THE guard. Not about markers or patterns — about whether the pair of
+    functions that every partial edit runs through is actually an identity."""
+    note = _stored(trigger, insight, sources)
+    title, body = _edit(note)
+    # Reported as a size delta rather than left to a diff of two multi-kilobyte
+    # strings: the failure this catches is duplication, and the number of
+    # characters gained says which paragraph came back far faster than an
+    # equality dump does (rule 167 — a guard has to name what broke).
+    assert (title, body) == (note.title, note.body), (
+        f"a no-op edit rewrote the lesson: title {len(title) - len(note.title):+d} "
+        f"chars, body {len(body) - len(note.body):+d} chars"
+    )
+
+
+@pytest.mark.parametrize("_name,trigger,insight,sources", SHAPES, ids=IDS)
+def test_ten_edits_do_not_grow_the_record(_name, trigger, insight, sources):
+    """Because the damage compounds rather than appearing once.
+
+    A single round trip off by one paragraph looks like a formatting wobble
+    in a diff. Ten of them is a lesson whose trigger appears eleven times in
+    its own body, which is both unreadable and — since the trigger is what
+    ranks — a document that has drowned out its own insight. Tagging a lesson
+    to a System is one such edit; so is fixing a typo in its title."""
+    note = _stored(trigger, insight, sources)
+    first = note.body
+    for _ in range(10):
+        title, body = _edit(note)
+        note = fake_lesson(title=title, body=body, data=note.data)
+    assert note.body == first, (
+        f"body grew by {len(note.body) - len(first)} characters over ten no-op edits"
+    )
+
+
+def test_no_paragraph_of_the_trigger_survives_into_the_insight():
+    """Stated on the paragraphs themselves, so the failure names the defect.
+
+    An equality assertion reports "these two long strings differ"; this one
+    reports which paragraph of the trigger leaked, which is the sentence
+    somebody needs in order to understand the bug at all."""
+    note = _stored(MANY_PARAGRAPHS, INSIGHT, [4219])
+    derived = lessons_svc.lesson_insight(note)
+    for n, para in enumerate(MANY_PARAGRAPHS.split("\n\n"), start=1):
+        assert para not in derived, (
+            f"paragraph {n} of the trigger came back as part of the insight; "
+            f"the next update will prepend the whole trigger again on top of it"
+        )
+
+
+def test_the_insight_is_derived_from_the_row_not_from_the_body_alone():
+    """Why the function takes a note.
+
+    The body on its own does not say where the trigger ends — that is the
+    information the old shape did not have and guessed at. The mirror in
+    `data` is where it comes from, so a caller holding only the body string
+    cannot be served, and this pins that the argument is the record."""
+    note = _stored(MANY_PARAGRAPHS, INSIGHT, [])
+    assert lessons_svc.lesson_insight(note) == INSIGHT.strip()
+    # The same body with the mirror removed cannot reach the same answer.
+    bare = fake_lesson(title=note.title, body=note.body, data=None)
+    assert lessons_svc.lesson_insight(bare) != INSIGHT.strip()
+
+
+def test_without_a_mirror_a_one_paragraph_trigger_still_strips():
+    """The fallback doing its job. A row written before the mirror existed,
+    or one whose `data` was dropped by a generic update, still reads back
+    correctly for the common shape — degrading to the old behaviour rather
+    than to nothing."""
+    _, body = lessons_svc.lesson_document(WHAT, ONE_LINE, INSIGHT, [4199])
+    bare = fake_lesson(title="t", body=body, data=None)
+    assert lessons_svc.lesson_insight(bare) == INSIGHT.strip()
+
+
+def test_a_stale_mirror_falls_back_rather_than_cutting_the_wrong_thing():
+    """If the mirror and the body disagree, the body wins by default.
+
+    `recompose_data` exists because a generic `update_note(body=...)` can
+    leave the mirror describing a trigger the body no longer has. Removing a
+    stored prefix that is not actually at the head of the body would cut into
+    the insight, so the removal happens only on an exact match and the
+    line-wise fallback takes over otherwise. Nothing authored is lost."""
+    _, body = lessons_svc.lesson_document(WHAT, ONE_LINE, INSIGHT, [])
+    stale = fake_lesson(
+        title="t", body=body,
+        data={"what": WHAT, lessons_svc.TRIGGER_KEY: "a trigger that is not in this body"},
+    )
+    derived = lessons_svc.lesson_insight(stale)
+    assert derived == INSIGHT.strip()
+    assert "a trigger that is not in this body" not in derived
+
+
+def test_a_lesson_with_neither_trigger_nor_sources_round_trips():
+    """The degenerate row — body is the insight and nothing else. Pinned
+    because the removal builds its prefix from the trigger, and an empty
+    trigger must mean "remove nothing", not "remove the marker text"."""
+    note = _stored("", INSIGHT, [])
+    assert note.body == INSIGHT.strip()
+    assert lessons_svc.lesson_insight(note) == INSIGHT.strip()
+    assert _edit(note) == (note.title, note.body)