feat(lessons): a lesson can be written, and it keeps every incident that taught it (#3731)

Milestone 385 step 4 — the write path.

ITS OWN TOOL MODULE, not create_note(note_type="lesson"), on the snippet and
process precedent and for the reason that precedent exists: a kind whose value
depends on one field being filled needs a door that ASKS for that field by
name. create_note would take a lesson through a generic body parameter and the
trigger — the whole of why a lesson is findable — would be something the writer
had to know to include.

THE TRIGGER IS REQUIRED, refused rather than flagged. Step 1 left the choice
open. Refusing is right for the same reason create_rule makes enforcement the
deciding question: a lesson with no trigger is not a weaker lesson, it is a
note that will never surface, and nothing downstream can tell the difference —
it saves, reads correctly in every listing, and is silently absent from the one
moment it was written for. A flag is a warning nobody is present to read; the
write path is where the writer still is. The message says SYMPTOM, because
"required" alone produces a topic where a situation was wanted.

The docstring carries the distinction this milestone exists to fix, in a line a
reader can apply: the difference between a lesson and a rule is FORCE, not
importance. If ignoring it would be a mistake it is a rule and needs the
operator's yes; if ignoring it just means someone re-derives it the slow way it
is a lesson, and nobody is bound.

CARDINALITY: a LIST, in notes.data under `taught_by`. The founding example
generalised three incidents into one claim about failure classes no CI lane can
see — generalising across incidents is the shape a good lesson HAS, and
arose_from_id holds one, so a single id keeps the first and drops two while
reading as complete. It lives in `data` rather than a join table for the reason
decision #4157 put the trigger there: a table would settle, for every note kind
at once, whether provenance is multi-valued — a question nothing has measured.
`arose_from_id` is filled only when there is exactly ONE source, because every
surface that renders it renders it as THE origin, and one of three would make
those surfaces state something false.

THE DUPLICATE GATE, which step 4 asked to check: a lesson is judged at a bar
ABOVE the sibling band, not the general 0.90. #2518 measured deliberately
parallel variants at 0.92 on a document that is mostly prose about the thing,
which is exactly a lesson's shape now — so at 0.90 two genuinely different
lessons about one area ("CI cannot see this class of failure") would refuse each
other. Its own constant rather than reusing the snippet's: the two are separate
facts that coincide today, and this number is inherited from a structurally
analogous corpus rather than measured on lessons, of which there are none yet.

Follows canon #2846 including the third registration point it names and this
change would otherwise have missed: get_lesson is in server._READ_ONLY_TOOLS
and the two writers in _WRITE_TOOLS, which test_mcp_auth requires.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy

tests/test_lesson_write_path.py

@@ -0,0 +1,200 @@
+"""Creating a lesson, and tying it to what taught it (milestone 385 step 4).
+
+THE TRIGGER IS THE RECORD, so the door refuses a lesson without one.
+
+Step 1 could have chosen "flag it visibly" instead. Refusing is the stronger
+answer for the same reason `create_rule` makes enforcement the deciding
+question: a lesson with no trigger is not a weaker lesson, it is a note that
+will never surface, and nothing downstream can tell the difference. It saves,
+it reads correctly in every listing, and it is silently absent from the one
+moment it was written for. A flag would be a warning nobody is present to read
+— the write path is where the writer still is.
+
+THE SOURCE IDS ARE A LIST, and `test_a_lesson_keeps_every_incident_that_taught
+_it` is why. The founding example generalised three incidents into one claim;
+`arose_from_id` holds one, and a record that keeps the first and drops two
+reads as complete.
+"""
+from __future__ import annotations
+
+from types import SimpleNamespace
+from unittest.mock import AsyncMock, patch
+
+import pytest
+
+from scribe.mcp._context import _user_id_ctx
+from scribe.mcp.tools.lessons import create_lesson, update_lesson
+from scribe.services import lessons as lessons_svc
+
+TRIGGER = "a test fails on code you believe is correct"
+SUBJECT = "Suspect the guard before the code"
+
+
+def _stub_note(**kw):
+    """A stand-in lesson row — every attribute the tool's _to_dict reads."""
+    base = dict(
+        id=1, title="t", body="b", tags=[], project_id=None,
+        note_type="lesson", data={}, arose_from_id=None,
+        created_at=None, updated_at=None,
+    )
+    base.update(kw)
+    return SimpleNamespace(**base)
+
+
+@pytest.mark.asyncio
+async def test_a_lesson_without_a_trigger_is_refused():
+    """THE guard. Falsifiable: it names the parameter, so a door that stopped
+    asking for it fails here rather than quietly writing an unfindable record."""
+    _user_id_ctx.set(7)
+    with pytest.raises(ValueError) as err:
+        await create_lesson(what=SUBJECT, when_to_apply="")
+
+    message = str(err.value)
+    assert "when_to_apply" in message
+    # It says what to write, not just that something is missing — the writer is
+    # here now, and "required" alone produces a topic where a symptom was wanted.
+    assert "symptom" in message.lower()
+
+
+@pytest.mark.asyncio
+async def test_whitespace_is_not_a_trigger():
+    """The refusal reads the stripped value. A door checking only falsiness
+    accepts a space and produces exactly the record it meant to prevent."""
+    _user_id_ctx.set(7)
+    with pytest.raises(ValueError):
+        await create_lesson(what=SUBJECT, when_to_apply="   ")
+
+
+@pytest.mark.asyncio
+async def test_a_lesson_keeps_every_incident_that_taught_it():
+    """Three sources in, three sources stored — and `arose_from_id` left NULL,
+    because one of three on a surface that renders it as THE origin would make
+    that surface state something false."""
+    _user_id_ctx.set(7)
+    created = AsyncMock(return_value=_stub_note())
+    with patch.object(lessons_svc, "create_lesson", created), \
+         patch("scribe.mcp.tools.lessons.dedup_svc.find_duplicate_note",
+               AsyncMock(return_value=None)), \
+         patch("scribe.mcp.tools.lessons.systems_tools.attach_systems", AsyncMock()):
+        await create_lesson(
+            what=SUBJECT, when_to_apply=TRIGGER, learned_from=[11, 22, 33],
+        )
+
+    assert created.await_args.kwargs["learned_from"] == [11, 22, 33]
+    assert lessons_svc.sole_source([11, 22, 33]) is None
+    assert lessons_svc.compose_data(SUBJECT, TRIGGER, [11, 22, 33])["taught_by"] == [
+        11, 22, 33,
+    ]
+
+
+def test_a_single_source_still_reaches_arose_from_id():
+    """The existing provenance field keeps working for the ordinary case — a
+    lesson drawn from one issue is not made less connected by the list."""
+    assert lessons_svc.sole_source([11]) == 11
+
+
+@pytest.mark.asyncio
+async def test_the_duplicate_gate_runs_before_anything_is_created():
+    """A near-match returns the existing id and writes nothing: two lessons
+    about one failure class want to be one lesson."""
+    _user_id_ctx.set(7)
+    hit = SimpleNamespace(id=99, title="already recorded", similarity=0.97,
+                          reason="semantic")
+    created = AsyncMock()
+    with patch("scribe.mcp.tools.lessons.dedup_svc.find_duplicate_note",
+               AsyncMock(return_value=hit)), \
+         patch.object(lessons_svc, "create_lesson", created):
+        out = await create_lesson(what=SUBJECT, when_to_apply=TRIGGER)
+
+    assert created.await_count == 0
+    assert out["duplicate"] is True
+    assert out["existing_id"] == 99
+    # The hint names the lesson's own updater, so the caller is pointed at a
+    # tool that exists rather than at update_note.
+    assert "update_lesson" in out["message"]
+
+
+@pytest.mark.asyncio
+async def test_the_gate_compares_the_composed_document_not_the_raw_fields():
+    """What reaches the gate is the title and body a lesson will actually be
+    stored as. Comparing `what` alone would miss that the trigger is half the
+    document, and would judge two lessons alike that rank nothing alike."""
+    _user_id_ctx.set(7)
+    gate = AsyncMock(return_value=None)
+    with patch("scribe.mcp.tools.lessons.dedup_svc.find_duplicate_note", gate), \
+         patch.object(lessons_svc, "create_lesson",
+                      AsyncMock(return_value=_stub_note())), \
+         patch("scribe.mcp.tools.lessons.systems_tools.attach_systems", AsyncMock()):
+        await create_lesson(what=SUBJECT, when_to_apply=TRIGGER, insight="Look.")
+
+    title, body = gate.await_args.args[1], gate.await_args.args[2]
+    assert title == f"{SUBJECT} — {TRIGGER}"
+    assert body.startswith(f"**When to apply:** {TRIGGER}")
+    assert gate.await_args.kwargs["note_type"] == "lesson"
+
+
+def test_a_lesson_is_judged_at_the_trigger_dominated_bar():
+    """#2518 measured deliberately-parallel siblings at 0.92 on a document that
+    is mostly prose ABOUT the thing. A lesson's document is that shape, so the
+    bar sits above that band — otherwise two different lessons about one area
+    block each other, which is the failure step 4 asked to check for."""
+    from scribe.services.dedup import (
+        _LESSON_SEMANTIC_THRESHOLD,
+        _SEMANTIC_THRESHOLD,
+        _semantic_threshold,
+    )
+
+    assert _semantic_threshold("lesson") == _LESSON_SEMANTIC_THRESHOLD
+    assert _LESSON_SEMANTIC_THRESHOLD > 0.92, (
+        "below the observed sibling band, so two genuinely different lessons "
+        "about one area would refuse each other"
+    )
+    assert _LESSON_SEMANTIC_THRESHOLD > _SEMANTIC_THRESHOLD
+    # An ordinary note is untouched — the carve-out is per kind, not a
+    # loosening of the gate.
+    assert _semantic_threshold("note") == _SEMANTIC_THRESHOLD
+
+
+@pytest.mark.asyncio
+async def test_an_update_recomposes_both_halves_of_the_document():
+    """A new trigger has to reach the title AND the head of the body. Patching
+    one would leave a lesson that reads correctly and ranks on the old
+    situation — the failure mode with no symptom."""
+    _user_id_ctx.set(7)
+    stored = _stub_note(
+        title=f"{SUBJECT} — {TRIGGER}",
+        body=f"**When to apply:** {TRIGGER}\n\nLook at the guard.",
+        data={"what": SUBJECT, "when_to_apply": TRIGGER},
+    )
+    updated = AsyncMock(return_value=_stub_note())
+    with patch.object(lessons_svc, "get_lesson", AsyncMock(return_value=stored)), \
+         patch("scribe.services.notes.update_note", updated):
+        await lessons_svc.update_lesson(7, 1, when_to_apply="a guard goes red")
+
+    fields = updated.await_args.kwargs
+    assert fields["title"] == f"{SUBJECT} — a guard goes red"
+    assert fields["body"].startswith("**When to apply:** a guard goes red")
+    assert fields["data"]["when_to_apply"] == "a guard goes red"
+
+
+@pytest.mark.asyncio
+async def test_an_update_does_not_stack_the_composed_lines():
+    """The insight is handed back to `compose_body`, which re-adds the trigger
+    and provenance lines. Without stripping them first they accumulate a copy
+    per edit — and because the trigger line is half of what makes the document
+    rank, the duplicates would look like the shape working."""
+    _user_id_ctx.set(7)
+    stored = _stub_note(
+        body=f"**When to apply:** {TRIGGER}\n\nLook at the guard."
+             "\n\n**Learned from:** #5",
+        data={"what": SUBJECT, "when_to_apply": TRIGGER, "taught_by": [5]},
+    )
+    updated = AsyncMock(return_value=_stub_note())
+    with patch.object(lessons_svc, "get_lesson", AsyncMock(return_value=stored)), \
+         patch("scribe.services.notes.update_note", updated):
+        await lessons_svc.update_lesson(7, 1, what="Suspect the guard")
+
+    body = updated.await_args.kwargs["body"]
+    assert body.count("**When to apply:**") == 1
+    assert body.count("**Learned from:**") == 1
+    assert "Look at the guard." in body