diff --git a/src/scribe/services/lessons.py b/src/scribe/services/lessons.py index 2d2577f..0f8080f 100644 --- a/src/scribe/services/lessons.py +++ b/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: - """The insight alone — the body with the lines `compose_body` wrote removed. +def lesson_insight(note) -> str: + """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`, - which will re-add the trigger and provenance lines. Without this the two - composed lines accumulate a copy per edit, and since the trigger line is - half of what makes the document rank, the duplicates would look like the - shape working rather than a bug. + An update that keeps the insight hands it back to `compose_body`, which + re-adds the trigger and provenance. So this has to remove EXACTLY what was + added; anything it leaves behind is re-prepended and the body grows by that + much on every edit. + + 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 = [ - line for line in (body or "").splitlines() - if not _BODY_TRIGGER_RE.match(line) and not _BODY_SOURCES_RE.match(line) - ] + body = getattr(note, "body", None) or "" + trigger = lesson_trigger(note).strip() + 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() diff --git a/tests/test_lesson_insight_round_trip.py b/tests/test_lesson_insight_round_trip.py new file mode 100644 index 0000000..dd6bde4 --- /dev/null +++ b/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)