feat(lessons): the lesson kind, and one join for every trigger title (#3729)

Milestone 385 step 2, implementing decision #4157 from the step-1 spike.

The kind: `note_type='lesson'`, a note findable by WHEN IT APPLIES rather
than by what it is about. The trigger lives in `notes.data.when_to_apply`,
mirrored into the title and the head of the body — the shape snippets
already use, and the reason nothing re-embeds: chunk_document is untouched,
so CHUNKER_VERSION does not move.

NO MIGRATION, and the step assumed there would be one. `note_type` carries
no CHECK — only `task_kind` does (0056, 0065). Migration 0036 added it as
plain Text with a server default and nothing has gated it since, so rule 36
has no whitelist to expand and #3128's failure mode (a value the database
refuses) cannot arise for this column. The vocabulary that actually decides
what a reader can reach is services.knowledge._FACETS, which since #3161 is
one table feeding the door's validation, the counts and both dialects of
the type filter — so the kind lands there in a single edit.

ONE JOIN, not a fourth copy. `{subject} — {trigger}` had three
implementations: rule_document, snippets.compose_title, and this step
needed another. #3207 records what that costs, so the join moves to
embeddings.trigger_title beside embedding_text and all three delegate.
Behaviour is unchanged for rules and snippets; the guard calls each through
its own public name, so a re-implementation fails it.

The #3163 bill is stated in the service docstring rather than left to be
inferred: versions, supersession, trash, the share ACL, tags, project and
System tagging, chunked embeddings and the duplicate gate are all
inherited; status/task_kind/milestone_id and recurrence are not, and
verify_with/expires_when are available but outside the kind's contract.
The status cell is the one that matters — `is_task` IS `status is not
None`, so a lesson that acquired one would become a task.

The integration guard asserts the WRITE rather than the constraint: it
holds whether or not note_type is ever gated, and goes red only if it is
gated without this value.

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

tests/test_integration_lesson_kind.py

@@ -0,0 +1,112 @@
+"""A lesson is a row the database actually accepts (milestone 385 step 2).
+
+WHY THIS IS THE REAL GUARD, AND WHY IT IS HERE
+
+The step asked for "a guard that the CHECK actually accepts `lesson` and
+rejects a typo", citing #3128 — a `spike` kind that could not be written on an
+instance predating its migration. That failure mode belongs to `task_kind`,
+which IS gated (`notes_task_kind_check`, migrations 0056 and 0065).
+`note_type` is not gated at all: migration 0036 added it as plain Text with a
+server default and nothing has constrained it since.
+
+So there is no whitelist to expand and no rejection to assert. Writing the
+guard as "the constraint admits lesson" would have pinned a constraint that
+does not exist; writing it as "no constraint exists" would pin today's schema
+rather than the behaviour that matters, and would go red on a change that is
+perfectly fine.
+
+Asserting the WRITE covers both worlds. It passes today, it keeps passing if a
+CHECK is added that admits `lesson`, and it goes red the day one is added that
+does not — which is the only outcome anyone needs to be told about.
+
+The second test is the cell #3163 asks to be left empty on purpose: a lesson
+must not be a task. `is_task` is a read-only property over `status`, so this
+cannot be asserted by setting a flag — it has to be observed on a real row.
+"""
+import pytest
+import pytest_asyncio
+from sqlalchemy import select
+
+from scribe.models import async_session
+from scribe.models.note import Note
+from scribe.services import lessons as lessons_svc
+from scribe.services import notes as notes_svc
+from tests.helpers import ensure_user
+
+pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")]
+
+OWNER_USERNAME = "lesson_kind_owner"
+
+TRIGGER = "the operator pasted a stack trace and said it is still broken"
+SUBJECT = "Change one thing, then look"
+
+
+@pytest_asyncio.fixture
+async def owner_id():
+    async with async_session() as s:
+        owner = await ensure_user(s, OWNER_USERNAME)
+        await s.commit()
+        uid = owner.id
+        # Cleaned at SETUP rather than teardown: create_note fires a detached
+        # embedding refresh that opens its own connection and writes the row,
+        # and a teardown delete would race it. A fresh loop has already
+        # cancelled whatever the previous test left in flight.
+        for note in (await s.execute(
+            select(Note).where(
+                Note.user_id == uid,
+                Note.note_type == lessons_svc.LESSON_NOTE_TYPE,
+            )
+        )).scalars().all():
+            await s.delete(note)
+        await s.commit()
+    return uid
+
+
+async def test_a_lesson_is_a_row_the_database_accepts(owner_id):
+    """The whole point. Goes red if `note_type` is ever gated without this
+    value, and stays correct if it is gated with it."""
+    lesson = await notes_svc.create_note(
+        owner_id,
+        title=lessons_svc.compose_title(SUBJECT, TRIGGER),
+        body=f"**When to apply:** {TRIGGER}\n\nOne change at a time.",
+        note_type=lessons_svc.LESSON_NOTE_TYPE,
+        data={lessons_svc.TRIGGER_KEY: TRIGGER},
+    )
+
+    async with async_session() as s:
+        stored = (await s.execute(
+            select(Note).where(Note.id == lesson.id)
+        )).scalars().one()
+
+    assert stored.note_type == "lesson"
+    # The trigger survives the round trip on both halves — the indexed mirror
+    # and the readable body — because the vector is built from the text and
+    # the queries are built from the mirror.
+    assert lessons_svc.lesson_trigger(stored) == TRIGGER
+    assert stored.title == f"{SUBJECT} — {TRIGGER}"
+    assert "**When to apply:**" in (stored.body or "")
+
+
+async def test_a_lesson_is_not_a_task(owner_id):
+    """#3163's cell left empty on purpose.
+
+    `is_task` is `status is not None` and is read-only, so this is only
+    observable on a stored row. A lesson that arrived with a status would join
+    the open-work listings — the one way an unfilled field changes what the
+    record IS rather than what it says.
+    """
+    lesson = await notes_svc.create_note(
+        owner_id,
+        title=lessons_svc.compose_title(SUBJECT, TRIGGER),
+        body="One change at a time.",
+        note_type=lessons_svc.LESSON_NOTE_TYPE,
+    )
+
+    async with async_session() as s:
+        stored = (await s.execute(
+            select(Note).where(Note.id == lesson.id)
+        )).scalars().one()
+
+    assert stored.status is None
+    assert stored.is_task is False
+    assert stored.milestone_id is None

tests/test_lesson_kind.py

@@ -0,0 +1,114 @@
+"""The `lesson` kind — its trigger, its title, and its place in the vocabulary.
+
+WHY THIS EXISTS (milestone 385 step 2, #3729)
+
+A lesson is a note that must be findable by WHEN IT APPLIES rather than by what
+it is about. That is a document-shape fact: the trigger has to reach the
+embedded text, and decision #4157 put it in `notes.data` with a mirror in the
+title and the head of the body — the shape snippets already use.
+
+THE ONE THAT MATTERS MOST
+
+`test_one_join_builds_every_trigger_title`. Three kinds now rank on a
+`{subject} — {trigger}` title: rules, snippets and lessons. That join had three
+implementations before this step and would have had four; #3207 records what
+that costs. The guard is behavioural rather than `assert a is b`, because the
+three are reached through different public names and a test that compared
+identities would pass on a re-implementation that merely re-exported.
+
+WHAT IS DELIBERATELY NOT TESTED HERE
+
+That the database accepts `note_type='lesson'`. `note_type` carries no CHECK —
+only `task_kind` does — so there is nothing to assert against in a unit test,
+and asserting the absence would pin the schema's current shape rather than the
+behaviour that matters. The real guard is in the integration lane, where a
+lesson is written and read back: it holds whether or not a constraint exists,
+and goes red the day one is added without this value.
+"""
+from types import SimpleNamespace
+
+from scribe.services import knowledge as knowledge_svc
+from scribe.services import lessons as lessons_svc
+from scribe.services import snippets as snippets_svc
+from scribe.services.embeddings import trigger_title
+
+
+def _note(data=None, body=""):
+    return SimpleNamespace(data=data, body=body)
+
+
+def test_the_trigger_is_read_from_the_indexed_mirror():
+    note = _note(data={"when_to_apply": "a stack trace, and it is still broken"})
+    assert lessons_svc.lesson_trigger(note) == "a stack trace, and it is still broken"
+
+
+def test_the_body_answers_when_the_mirror_is_absent():
+    """Not dead code. A row written before the mirror existed is still readable,
+    and an absent mirror has to degrade to the right answer rather than to
+    silence — the discipline `snippet_fields` follows for the same reason."""
+    note = _note(body="**When to apply:** about to reach for a second hypothesis\n\nbody")
+    assert lessons_svc.lesson_trigger(note) == "about to reach for a second hypothesis"
+
+
+def test_the_mirror_wins_when_both_are_present():
+    note = _note(
+        data={"when_to_apply": "from the mirror"},
+        body="**When to apply:** from the body",
+    )
+    assert lessons_svc.lesson_trigger(note) == "from the mirror"
+
+
+def test_no_trigger_reads_as_empty_rather_than_raising():
+    """A lesson with no trigger is unreachable, not broken. Whatever refuses to
+    write one belongs on the write path; a reader's job is to say so plainly."""
+    assert lessons_svc.lesson_trigger(_note()) == ""
+    assert lessons_svc.lesson_trigger(_note(data={}, body="no trigger line here")) == ""
+
+
+def test_one_join_builds_every_trigger_title():
+    """THE GUARD. Rules, snippets and lessons rank on the same title shape.
+
+    Behavioural on purpose — see the module docstring. Each of the three is
+    called through the name its own callers use, so a fourth hand-rolled copy
+    fails here even though it would look correct in isolation.
+    """
+    subject, trigger = "Pace hard debugging", "a stack trace and it is still broken"
+    expected = f"{subject} — {trigger}"
+
+    assert trigger_title(subject, trigger) == expected
+    assert lessons_svc.compose_title(subject, trigger) == expected
+    assert snippets_svc.compose_title(subject, trigger) == expected
+
+
+def test_a_subject_with_no_trigger_degrades_to_the_subject():
+    """It still embeds, just less sharply — an argument for backfilling
+    triggers, not for padding the title with whatever text is to hand."""
+    assert trigger_title("debounce", "") == "debounce"
+    assert lessons_svc.compose_title("  debounce  ") == "debounce"
+    assert trigger_title("", "when it applies") == "when it applies"
+
+
+def test_the_kind_is_in_the_browse_vocabulary():
+    """A kind the browse surface does not know is a record nobody can filter to.
+
+    Asserted through the public table rather than a literal, because the door's
+    validation, the counts and both dialects of the type filter are all
+    generated from it (#3161) — so this is the one place that decides.
+    """
+    assert lessons_svc.LESSON_NOTE_TYPE in knowledge_svc.FACET_TYPES
+    assert lessons_svc.LESSON_NOTE_TYPE in knowledge_svc.NON_TASK_FACETS
+
+
+def test_a_lesson_is_not_a_task_on_either_arm_of_the_filter():
+    """The cell left empty on purpose (#3163).
+
+    `is_task` IS `status is not None`, so a lesson that acquired a status would
+    become a task and start appearing in open-work listings — the one way
+    filling a field in by accident changes what the record IS.
+    """
+    assert knowledge_svc.facet_is_task(lessons_svc.LESSON_NOTE_TYPE) is False
+    lesson = SimpleNamespace(note_type="lesson", is_task=False, task_kind="work")
+    assert knowledge_svc.matches_facet(lesson, "lesson") is True
+    assert knowledge_svc.matches_facet(lesson, "task") is False
+    # And it does not answer to another kind's facet.
+    assert knowledge_svc.matches_facet(lesson, "note") is False

tests/test_services_knowledge_counts.py

@@ -71,7 +71,10 @@ async def test_total_counts_snippets_and_counts_no_task_twice():
 async def test_absent_facets_report_zero_rather_than_missing():
     counts, _ = await _counts([("note", 1)], [])
     assert counts["note"] == 1
-    for key in ("process", "snippet", "task", "work", "issue", "spike", "plan"):
+    # `lesson` is here from milestone 385 step 2: a kind added to the facet
+    # table and not to the counts would show an empty chip beside a feed that
+    # has rows in it, which is defect 3b in #3161 repeating itself.
+    for key in ("process", "snippet", "lesson", "task", "work", "issue", "spike", "plan"):
         assert counts[key] == 0, key
     assert counts["total"] == 1