diff --git a/src/scribe/models/note.py b/src/scribe/models/note.py index f9d51b3..0133276 100644 --- a/src/scribe/models/note.py +++ b/src/scribe/models/note.py @@ -76,8 +76,27 @@ class Note(Base, TimestampMixin, SoftDeleteMixin): recurrence_next_spawn_at: Mapped[datetime | None] = mapped_column( DateTime(timezone=True), nullable=True ) - # Note type — 'note' (default) or 'process' (a stored process). Task-ness is - # tracked by `status`, not here. (person/place/list entity types removed 2026-07.) + # WHAT KIND of record this is, on the note/entity axis. Task-ness is tracked + # by `status`, not here (person/place/list entity types removed 2026-07): + # note (default) — authored prose, findable by what it is ABOUT + # process — a stored procedure, synced to a client as a skill + # snippet — a reusable shape, with its structured fields mirrored in + # `data` and its trigger composed into the title (0070) + # lesson — a transferable insight, findable by WHEN IT APPLIES rather + # than by topic (milestone 385). Same mirror discipline as a + # snippet: the trigger lives in `data.when_to_apply` and is + # composed into the title and the head of the body, which is + # what puts it in the embedded document. A lesson is never a + # task — see `status` — because a lesson that acquired one + # would start appearing in open-work listings. + # + # DELIBERATELY UNGATED. Unlike `task_kind` there is no CHECK on this column: + # migration 0036 added it as plain Text with a server default and nothing + # has constrained it since, so rule 36 has no whitelist to expand when a + # kind is added. The vocabulary that actually decides what a reader can + # reach is `services.knowledge._FACETS`; an unrecognised value there + # resolves to a filter matching nothing, which is the intended answer to a + # typo. note_type: Mapped[str] = mapped_column(Text, default="note", server_default="note") # Task sub-kind — what KIND of work this is, not how it is going: # work (default) — ships a change diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 2dcbbe7..7dc11fe 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -203,6 +203,33 @@ def embedding_text(title: str | None, body: str | None) -> str: return f"{title}\n{body}".strip() if body else title +def trigger_title(subject: str | None, trigger: str | None) -> str: + """`{subject} — {trigger}` — the title half of a situation-keyed document. + + ONE definition, because this join had three. `rule_document` built it for + rules, `snippets.compose_title` for snippets, and milestone 385 needed a + fourth for lessons — the shape #3207 records, where a fix or an improvement + then has to be found in N places by someone who does not know N. + + WHY THE JOIN MATTERS AT ALL, measured in note #2485: the snippet was the + only sharp record in the corpus — a 0.153 top-to-second gap against + 0.010–0.023 for everything else — and the cause was this title plus the + same trigger repeated in the body, so purpose appears twice in a short + document and dominates the vector. Every kind that must be findable by WHEN + IT APPLIES rather than what it is about is built on this line. + + Either side alone is returned as-is: a record with no trigger yet degrades + to its subject and still embeds, just less sharply — which is an argument + for backfilling triggers, not for padding the title with whatever text is + to hand. + """ + subject = (subject or "").strip() + trigger = (trigger or "").strip() + if subject and trigger: + return f"{subject} — {trigger}" + return subject or trigger + + # --- chunking (#280): the document shape ------------------------------------ # # bge-small reads at most 512 tokens and fastembed silently truncates the rest, @@ -785,7 +812,7 @@ def rule_document( if not trigger: return name or None, body or None return ( - f"{name} — {trigger}" if name else trigger, + trigger_title(name, trigger), f"When to apply: {trigger}\n\n{body}" if body else f"When to apply: {trigger}", ) diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index a5506ad..285dfc0 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -292,6 +292,13 @@ _FACETS: dict[str, tuple[bool, str | None]] = { "note": (False, "note"), "process": (False, "process"), "snippet": (False, "snippet"), + # A transferable insight, keyed by the situation it applies to rather than + # by its topic (milestone 385). It reaches the browse surface through this + # one entry: the door's validation, the counts and both dialects of the + # type filter are all generated from this table, which is the property + # #3161 asked for so that adding a kind is a single edit rather than four + # that must agree. + "lesson": (False, "lesson"), } # The non-task record types, for the counts query. Derived so it cannot drift diff --git a/src/scribe/services/lessons.py b/src/scribe/services/lessons.py new file mode 100644 index 0000000..cf4fd6c --- /dev/null +++ b/src/scribe/services/lessons.py @@ -0,0 +1,138 @@ +"""Lesson service — a transferable insight, retrievable by situation. + +A *lesson* is a Note with ``note_type='lesson'``: a better way to think about a +problem, or a solution that transfers, recorded so a later session meets it at +the moment it applies — and **without binding the reader**. + +WHY THE KIND EXISTS (milestone 385, from note #3727) + +Rules were the only surface that is global AND situation-keyed, so an agent +holding a transferable insight had one door, and that door binds. The observed +symptom was sessions offering rule proposals for things that should not be +rules. + +The gap is a document-shape fact, not a threshold: + + - a note is embedded as ``title\\nbody`` and is findable by **what it is + about**; + - a rule is embedded as ``{title} — {trigger}`` with ``When to apply:`` + repeated at the head of the body, so the trigger appears twice in a short + document and dominates the vector — findable by **when it applies**. + +No tuning reaches across that: the field the query would match on is simply not +in a note's document. So a lesson carries a trigger and is embedded like a rule, +while staying a note in every other respect. + +WHERE THE TRIGGER LIVES (decision #4157, milestone 385 step 1) + +In ``notes.data`` under ``when_to_apply``, written through a named parameter and +mirrored into the title and the head of the body — the shape snippets already +use for ``when_to_use``. Not a column on ``notes``. + +That decision was measured rather than assumed. The whole snippet corpus — +164 of 164 — carries a ``when_to_use`` with **no guard anywhere**, which refutes +the premise that an unenforced field gets skipped. What it does NOT show is that +an agent types a title convention correctly: ``compose_title`` builds the title +from the parameter, so what is at 100% is a named structured field. A column +would have bought enforceability at the price of deciding, for every note kind +at once, a question nothing had measured. + +The mirror is what makes the vector sharp, and it is why nothing re-embeds: +``chunk_document`` is untouched, so ``CHUNKER_VERSION`` does not move. The +trigger reaches the document by being in the text, exactly as a snippet's is. + +WHAT A LESSON INHERITS, AND THE CELLS LEFT EMPTY ON PURPOSE (#3163) + +A new kind inherits the note machinery wholesale, and #3163 asks which parts it +should NOT get — so that an empty cell is a decision rather than an oversight. + +Inherited, all deliberately: + + - **versions** — a lesson is reworded as understanding improves, and what it + used to say is worth as much as any note's history. + - **supersession** — the event this most needs. A lesson replaced by a better + lesson is precisely what ``note_supersessions`` models, and the demotion + penalty already exists. + - **trash**, **the share ACL**, **tags**, **project and System tagging**, + **chunked embeddings**, **the near-duplicate gate**. + +NOT inherited, and each for a stated reason: + + - **status / task_kind / milestone_id** — a lesson is not work. ``is_task`` is + ``status is not None``, so a lesson that acquired a status would become a + task and appear in open-work listings. This is the one cell where filling it + in by accident silently changes what the record IS. + - **recurrence** — task-only, and a lesson does not recur. + - **verify_with / expires_when** — available, because they are generic note + fields, but not part of a lesson's contract and not asked for on create. The + milestone-312 distinction is why: those mark a record that asserts a FACT + about someone else's software and can go false unwatched. A lesson is closer + to a norm — "a better way to think about this" has no truth value that rots + on its own. A lesson that does assert such a fact can still carry them. + +WHY THERE IS NO MIGRATION + +``note_type`` carries **no CHECK constraint** — only ``task_kind`` does +(``notes_task_kind_check``, migrations 0056 / 0065). Migration 0036 added +``note_type`` as plain ``Text`` with a server default and nothing has gated it +since. So rule 36 has nothing to expand here, and the failure it guards against +— a value the database refuses on an instance predating its migration — cannot +arise for this column. + +The real vocabulary is ``services.knowledge._FACETS``, which is where a kind +becomes reachable on the browse surface and validated at the door. That is one +table feeding both dialects of the type filter, so adding a kind there is a +single edit — a property #3161 recommended and that landed before this. +""" +from __future__ import annotations + +import re + +LESSON_NOTE_TYPE = "lesson" + +# The key in `notes.data`. Named for the field it mirrors on `rules`, because it +# answers the same question and a reader who knows one should not have to learn +# a second word for it. +TRIGGER_KEY = "when_to_apply" + +# The body's trigger line, and the pattern that reads it back. The body is the +# readable form and the thing that gets embedded; `data` is the queryable +# mirror. Reads prefer the mirror and fall back to this, which is the discipline +# `snippet_fields` follows and the reason a row written before the mirror +# existed is still readable. +_BODY_TRIGGER_RE = re.compile(r"^\*\*When to apply:\*\*\s*(.+?)\s*$", re.M) + + +def lesson_trigger(note) -> str: + """When this lesson applies, or "" — the mirror first, then the body. + + Prefers `data` for the same reason every snippet read does: it is indexed, + and parsing a body to answer a question the database can answer is how a + hot path ends up regexing markdown. The fallback is not dead code — it is + what makes a lesson readable if the mirror is ever absent, and an absent + mirror must degrade to the right answer rather than to silence. + """ + data = getattr(note, "data", None) or {} + from_mirror = (data.get(TRIGGER_KEY) or "").strip() if isinstance(data, dict) else "" + if from_mirror: + return from_mirror + match = _BODY_TRIGGER_RE.search(getattr(note, "body", None) or "") + return match.group(1).strip() if match else "" + + +def compose_title(what: str, when_to_apply: str = "") -> str: + """`{what} — {when it applies}`, the half of the document that ranks. + + Built HERE rather than asked of the caller, and that distinction is the + whole evidence base for this design: the snippet corpus is at 100% on its + trigger because a service composes the title from a named parameter, not + because agents type separators reliably. A caller made to spell the + convention is the option milestone 385 step 1 rejected. + + The join is `embeddings.trigger_title` — shared with rules and snippets, so + the three kinds that rank on a trigger cannot drift apart in how they say + so. + """ + from scribe.services.embeddings import trigger_title + + return trigger_title(what, when_to_apply) diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 8e6fdb9..c36067b 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -55,10 +55,15 @@ UNSET: object = object() # --- serialize: structured fields -> note (title/body/tags) ------------------ def compose_title(name: str, when_to_use: str = "") -> str: - """`name — when to use` (or just `name` when no usage note is given).""" - name = (name or "").strip() - when = (when_to_use or "").strip() - return f"{name} — {when}" if when else name + """`name — when to use` (or just `name` when no usage note is given). + + The join itself lives in `embeddings.trigger_title`, which rules and + lessons build their titles from too. Kept as a named function here because + it is this module's public vocabulary and callers say `compose_title`. + """ + from scribe.services.embeddings import trigger_title + + return trigger_title(name, when_to_use) def compose_tags(language: str = "", tags: list[str] | None = None) -> list[str]: diff --git a/tests/test_integration_lesson_kind.py b/tests/test_integration_lesson_kind.py new file mode 100644 index 0000000..ef2162f --- /dev/null +++ b/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 diff --git a/tests/test_lesson_kind.py b/tests/test_lesson_kind.py new file mode 100644 index 0000000..6d1e46f --- /dev/null +++ b/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 diff --git a/tests/test_services_knowledge_counts.py b/tests/test_services_knowledge_counts.py index efe6516..4164504 100644 --- a/tests/test_services_knowledge_counts.py +++ b/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