From 8de97b3190b00f9e7cf7b0f5ad3feb65e6927c34 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 16:35:51 -0400 Subject: [PATCH 1/3] fix(lessons): strip a lesson's trigger by what was stored, not by matching a line (#4272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update_lesson` is field-wise: an edit that touches only `system_ids`, or only the title, passes `insight=None`. The service then derives the insight from the stored body and hands it straight back to `compose_body`, which re-adds the trigger and the provenance line. The derivation and the composition have to be inverses, and they were not. `_strip_composed_lines` filtered out body LINES matching the trigger marker's pattern. That pattern is `re.M`, so it 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 had no reason to touch. They came back as part of "the insight", the whole trigger was prepended again on top of them, and the record gained (n-1) paragraphs per update. Silent, compounding, and in the half of the document that does the retrieving: a lesson edited a few times ends up stating when it applies several times over and burying the insight below it. No marker could fix this. 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 rebuilt byte for byte and removed by subtraction rather than matched by pattern. Hence `lesson_insight(note)` rather than `_strip_composed_lines(body)` — it takes the record because the body alone does not say where the trigger ends. The line-wise removal survives as the fallback for a row whose mirror is missing, where it is right for a one-paragraph trigger and is the best available without one; a mirror that disagrees with the body falls back rather than cutting into the insight. tests/test_lesson_insight_round_trip.py asserts the property rather than the strip: `lesson_document(what, trigger(n), insight(n), sources(n)) == (n.title, n.body)` across six shapes, and again over ten consecutive no-op edits. Falsified against the old code before committing: 11 of 23 fail on HEAD, the headline one reporting a body grown by 3230 characters over ten edits. The one damaged row in the corpus (#4207, two paragraphs duplicated by a `system_ids` edit) was repaired by hand against its authored text. #4221 is the only other lesson with a multi-paragraph trigger and has never been updated, so its body is intact — its derived `insight` was wrong, its storage was not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/lessons.py | 52 ++++-- tests/test_lesson_insight_round_trip.py | 217 ++++++++++++++++++++++++ 2 files changed, 256 insertions(+), 13 deletions(-) create mode 100644 tests/test_lesson_insight_round_trip.py 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..abc9ab7 --- /dev/null +++ b/tests/test_lesson_insight_round_trip.py @@ -0,0 +1,217 @@ +"""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. On the corpus's worst real shape it grew +the body by 520 characters over ten round trips. + +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_note + +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.""" + title, body = lessons_svc.lesson_document(WHAT, trigger, insight, sources) + data = lessons_svc.compose_data(WHAT, trigger, sources) + return fake_note(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_note(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_note(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_note(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_note( + 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) -- 2.54.0 From 9883b010b369926eb2d071ecb70cda414d0577fe Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 16:37:10 -0400 Subject: [PATCH 2/3] docs(lessons): give the round-trip guard the measured growth, not a remembered one (#4272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module docstring said the defect grew the worst real body by 520 characters over ten edits. That number was recalled, not measured, and it is wrong — as was the 2500 I then got by deriving it on paper instead of running it. Replayed lesson #4207's actual stored shape through the old code: 607 characters per edit, exactly the length of the trigger's paragraphs 2 and 3 with their separators, taking the body from 1291 to 7361 over ten edits. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_lesson_insight_round_trip.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/test_lesson_insight_round_trip.py b/tests/test_lesson_insight_round_trip.py index abc9ab7..8db7698 100644 --- a/tests/test_lesson_insight_round_trip.py +++ b/tests/test_lesson_insight_round_trip.py @@ -22,8 +22,13 @@ 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. On the corpus's worst real shape it grew -the body by 520 characters over ten round trips. +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. -- 2.54.0 From b47dc97ca84f7a005fd2562be80dcf0aef442eec Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 16:39:31 -0400 Subject: [PATCH 3/3] fix(tests): the round-trip guard needs fake_lesson, not fake_note (#4272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 7208 failed 5 of 23 in the new file, all with a `**Learned from:** #1` that no shape asked for. `lesson_sources` falls back to `arose_from_id` when neither the mirror nor the body names a source. `fake_note` leaves that field to MagicMock, which auto-creates it truthy and coerces to 1 — so every no-sources shape recomposed with a provenance line and the round trip failed for a reason with nothing to do with the trigger. `fake_lesson` pins `arose_from_id` to None and its docstring says why; it is also the truthful stand-in, since these are lessons. Worth recording separately: the local harness that cleared this change used a plain stub class, so it was kinder than the real fixture and could not reproduce the truthiness. The stub has been given the same None. A stand-in that is more forgiving than the one the suite uses will verify a change that CI then rejects — note #2109's point, arriving from the other direction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_lesson_insight_round_trip.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/test_lesson_insight_round_trip.py b/tests/test_lesson_insight_round_trip.py index 8db7698..dd6bde4 100644 --- a/tests/test_lesson_insight_round_trip.py +++ b/tests/test_lesson_insight_round_trip.py @@ -48,7 +48,7 @@ from __future__ import annotations import pytest from scribe.services import lessons as lessons_svc -from tests.helpers import fake_note +from tests.helpers import fake_lesson WHAT = "A column written once at create time is a snapshot, not a field" @@ -92,10 +92,18 @@ 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.""" + 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_note(title=title, body=body, data=data) + return fake_lesson(title=title, body=body, data=data) def _edit(note): @@ -149,7 +157,7 @@ def test_ten_edits_do_not_grow_the_record(_name, trigger, insight, sources): first = note.body for _ in range(10): title, body = _edit(note) - note = fake_note(title=title, body=body, data=note.data) + 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" ) @@ -180,7 +188,7 @@ def test_the_insight_is_derived_from_the_row_not_from_the_body_alone(): 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_note(title=note.title, body=note.body, data=None) + bare = fake_lesson(title=note.title, body=note.body, data=None) assert lessons_svc.lesson_insight(bare) != INSIGHT.strip() @@ -190,7 +198,7 @@ def test_without_a_mirror_a_one_paragraph_trigger_still_strips(): 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_note(title="t", body=body, data=None) + bare = fake_lesson(title="t", body=body, data=None) assert lessons_svc.lesson_insight(bare) == INSIGHT.strip() @@ -203,7 +211,7 @@ def test_a_stale_mirror_falls_back_rather_than_cutting_the_wrong_thing(): 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_note( + stale = fake_lesson( title="t", body=body, data={"what": WHAT, lessons_svc.TRIGGER_KEY: "a trigger that is not in this body"}, ) -- 2.54.0