From 2e4c2d949338d2550a86ae374cdd068ef5f87e2b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 1 Oct 2026 10:47:03 -0400 Subject: [PATCH 1/5] =?UTF-8?q?feat(usage):=20count=20what=20happened=20aw?= =?UTF-8?q?ay=20from=20a=20record's=20own=20project=20=E2=80=94=20the=20re?= =?UTF-8?q?adout=20#3735=20needs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone 385 step 8 (#3735 "a lesson is recalled on a project it was not written on") is defined by opened-on-another-project. The reader's project has been recorded on every usage event since 0c8e109, but nothing compared it with the record's own, so the criterion was still unreadable. - note_usage.usage_for_notes: a second aggregate in the same session joins notes and counts surfaced_away_count / pulled_away_count. Counted only where both projects are known and differ; ranked surfacings only (#2477). The first aggregate is untouched, so events on deleted notes still count. - empty_usage carries both keys zero-filled; every door that attaches usage (list_lessons, get_lesson, snippets, knowledge) gets them through attach_usage. - UsageBadge tooltip says "On other projects: surfaced N×, opened M×" when it happened, and nothing when it did not. - Tests: the mocked split test feeds both aggregates; a unit test for the away counters; a real-Postgres test that home, unreported and ambient events are all left out. Co-Authored-By: Claude Opus 5.5 --- frontend/src/components/UsageBadge.vue | 8 ++- frontend/src/types/usage.ts | 5 ++ src/scribe/services/note_usage.py | 42 ++++++++++++ tests/test_note_usage.py | 89 +++++++++++++++++++++++++- 4 files changed, 140 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/UsageBadge.vue b/frontend/src/components/UsageBadge.vue index 47fabbb..68452e4 100644 --- a/frontend/src/components/UsageBadge.vue +++ b/frontend/src/components/UsageBadge.vue @@ -42,9 +42,15 @@ const title = () => { ? `Last opened ${new Date(u.last_pulled_at).toLocaleDateString()}.` : "Never opened."; const verdict = isDeadWeight() ? ` ${props.deadWeightAdvice}` : ""; + // Said only when it happened: "0× elsewhere" on every row would read as a + // finding about records that simply have not been near another project. + const awaySurfaced = u.surfaced_away_count ?? 0; + const away = awaySurfaced + ? ` On other projects: surfaced ${awaySurfaced}×, opened ${u.pulled_away_count ?? 0}×.` + : ""; return ( `Surfaced to an agent ${u.surfaced_count}×, opened in full ` + - `${u.pull_count}×. ${last}${verdict}` + `${u.pull_count}×.${away} ${last}${verdict}` ); }; diff --git a/frontend/src/types/usage.ts b/frontend/src/types/usage.ts index 42d4c8b..3dadafd 100644 --- a/frontend/src/types/usage.ts +++ b/frontend/src/types/usage.ts @@ -20,4 +20,9 @@ export interface RecordUsage { pull_count: number; last_surfaced_at: string | null; last_pulled_at: string | null; + /** The subsets that happened on a project other than the record's own — + * the evidence it transferred (milestone 385). Notes and snippets carry + * them; rules are global, so their readout does not. */ + surfaced_away_count?: number; + pulled_away_count?: number; } diff --git a/src/scribe/services/note_usage.py b/src/scribe/services/note_usage.py index cdada2b..3416bd3 100644 --- a/src/scribe/services/note_usage.py +++ b/src/scribe/services/note_usage.py @@ -35,6 +35,7 @@ from collections.abc import Sequence from sqlalchemy import case, func, select from scribe.models import async_session +from scribe.models.note import Note from scribe.models.note_usage import PULLED, SURFACED, NoteUsageEvent from scribe.models.base import iso from scribe.services.background import report_telemetry_failure @@ -193,6 +194,13 @@ def empty_usage() -> dict: record. `ambient_count` is the rest (see AMBIENT_SOURCES). The split is the readout half of #2477: the "high surfaced, zero pulls → dead weight" reading is only valid over surfacings that were choices. + + `surfaced_away_count` / `pulled_away_count` are the subsets that happened + on a project other than the one the record was written on — the evidence + that a record TRANSFERRED, which is the claim the lesson kind rests on + (milestone 385, #3735). Counted only where both projects are known: an + event with no reader project, or a record with no project of its own, + cannot speak to "away" and is left out rather than guessed. """ return { "surfaced_count": 0, @@ -200,6 +208,8 @@ def empty_usage() -> dict: "pull_count": 0, "last_surfaced_at": None, "last_pulled_at": None, + "surfaced_away_count": 0, + "pulled_away_count": 0, } @@ -247,6 +257,30 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: ) ) ).all() + # Away from home: the reader's project against the record's own. + # A second aggregate rather than a column on the first, because + # it needs the join to `notes` and the first must keep counting + # events whose note has since been deleted (the table is FK-free + # so that evidence outlives the row). Ranked surfacings only, for + # the same reason surfaced_count is (#2477). + away_rows = ( + await session.execute( + select( + NoteUsageEvent.note_id, + NoteUsageEvent.event, + func.count().label("n"), + ) + .join(Note, Note.id == NoteUsageEvent.note_id) + .where( + NoteUsageEvent.note_id.in_(ids), + NoteUsageEvent.project_id.is_not(None), + Note.project_id.is_not(None), + NoteUsageEvent.project_id != Note.project_id, + NoteUsageEvent.source.not_in(AMBIENT_SOURCES), + ) + .group_by(NoteUsageEvent.note_id, NoteUsageEvent.event) + ) + ).all() except Exception: # A telemetry readout must not be able to break the list it decorates — # but it must say it failed, or a broken readout is indistinguishable @@ -271,6 +305,14 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: latest = iso(last_at) if latest and (slot["last_pulled_at"] or "") < latest: slot["last_pulled_at"] = latest + for note_id, event, n in away_rows: + slot = out.get(int(note_id)) + if slot is None: + continue + if event == SURFACED: + slot["surfaced_away_count"] = int(n) + elif event == PULLED: + slot["pulled_away_count"] = int(n) return out diff --git a/tests/test_note_usage.py b/tests/test_note_usage.py index 26ba708..4bcc517 100644 --- a/tests/test_note_usage.py +++ b/tests/test_note_usage.py @@ -98,9 +98,12 @@ async def test_usage_for_notes_splits_counts_by_event(): (3, "pulled", 2, ts, False), ] session = MagicMock() - session.execute = AsyncMock( - return_value=MagicMock(all=MagicMock(return_value=rows)) - ) + # Two aggregates in one session: every event, then the away-from-home + # subset (note_id, event, count) — none here. + session.execute = AsyncMock(side_effect=[ + MagicMock(all=MagicMock(return_value=rows)), + MagicMock(all=MagicMock(return_value=[])), + ]) ctx = MagicMock() ctx.__aenter__ = AsyncMock(return_value=session) ctx.__aexit__ = AsyncMock(return_value=False) @@ -115,6 +118,36 @@ async def test_usage_for_notes_splits_counts_by_event(): assert out[3]["last_pulled_at"] == ts.isoformat() +async def test_usage_for_notes_counts_what_happened_away_from_home(): + """Surfaced or opened on a project other than the record's own is the + evidence a record transferred (#3735). It is a SUBSET of the totals, read + from the second aggregate, and it must land on the right counter.""" + from datetime import datetime, timezone + + ts = datetime(2026, 10, 1, tzinfo=timezone.utc) + session = MagicMock() + session.execute = AsyncMock(side_effect=[ + MagicMock(all=MagicMock(return_value=[ + (3, "surfaced", 9, ts, False), + (3, "pulled", 4, ts, False), + (4, "surfaced", 2, ts, False), + ])), + MagicMock(all=MagicMock(return_value=[ + (3, "surfaced", 5), + (3, "pulled", 1), + ])), + ]) + ctx = MagicMock() + ctx.__aenter__ = AsyncMock(return_value=session) + ctx.__aexit__ = AsyncMock(return_value=False) + with patch.object(note_usage, "async_session", return_value=ctx): + out = await usage_for_notes([3, 4]) + assert (out[3]["surfaced_away_count"], out[3]["pulled_away_count"]) == (5, 1) + assert (out[3]["surfaced_count"], out[3]["pull_count"]) == (9, 4) + # Used only at home: the away counters stay zero, not missing. + assert (out[4]["surfaced_away_count"], out[4]["pulled_away_count"]) == (0, 0) + + async def test_usage_readout_failure_degrades_to_zeroes(): """A telemetry readout must not be able to break the list it decorates.""" with patch.object(note_usage, "async_session", side_effect=RuntimeError("boom")): @@ -303,3 +336,53 @@ async def test_record_pulled_lands_end_to_end_from_a_running_loop(_dispose_engin assert out[nid]["pull_count"] == 1 finally: await _purge(nid) + + +@pytest.mark.integration +async def test_away_counts_compare_the_readers_project_with_the_records(_dispose_engine): + """The real join: an event counts as away only when the reader's project is + known and differs from the record's own. Home, unreported and ambient + events are all left out — each would otherwise read as transfer.""" + from sqlalchemy import delete + + from scribe.models import async_session + from scribe.models.note import Note + from scribe.models.project import Project + from scribe.services.note_usage import _insert_events + from tests.helpers import ensure_user + + async with async_session() as s: + owner = await ensure_user(s, "usage_away_owner") + home = Project(user_id=owner.id, title="usage home") + away = Project(user_id=owner.id, title="usage away") + s.add_all([home, away]) + await s.flush() + lesson = Note(user_id=owner.id, project_id=home.id, title="a lesson", + note_type="lesson") + s.add(lesson) + await s.flush() + nid, home_id, away_id, uid = lesson.id, home.id, away.id, owner.id + await s.commit() + try: + def ev(event, source, pid): + return {"user_id": uid, "note_id": nid, "event": event, + "source": source, "project_id": pid} + await _insert_events([ + ev("surfaced", "lesson_slot", away_id), # away: counts + ev("surfaced", "lesson_slot", home_id), # home + ev("surfaced", "lesson_slot", None), # unreported + ev("surfaced", "enter_project", away_id), # ambient + ev("pulled", "mcp_get_lesson", away_id), # away: counts + ev("pulled", "mcp_get_lesson", None), # unreported + ]) + out = await usage_for_notes([nid]) + assert out[nid]["surfaced_away_count"] == 1 + assert out[nid]["pulled_away_count"] == 1 + assert out[nid]["surfaced_count"] == 3 + assert out[nid]["pull_count"] == 2 + finally: + await _purge(nid) + async with async_session() as s: + await s.execute(delete(Note).where(Note.id == nid)) + await s.execute(delete(Project).where(Project.id.in_([home_id, away_id]))) + await s.commit() From 41e4fbaba13f3a35ca952dc00e549b427a27ae38 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 1 Oct 2026 12:38:02 -0400 Subject: [PATCH 2/5] =?UTF-8?q?feat(lessons):=20a=20lesson=20names=20the?= =?UTF-8?q?=20rule=20it=20is=20an=20instance=20of=20=E2=80=94=20lesson=5Fr?= =?UTF-8?q?ule=5Flinks=20(milestone=20440=20step=201,=20#4630)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The link between a lesson (one concrete situation) and the rule that governs it, with the operator's soft-then-hard design built into its state: suggested while evidence accumulates, confirmed or rejected once judged. Only confirmed will carry a rule in retrieval (#4633); rejected is kept so the pair is never proposed again. - models/lesson_rule_link.py + migration 0111: one row per (lesson, rule), CASCADE on both ends, indexed both ways, CHECK on state (rule 36), evidence JSONB and judged_at. - services/lesson_rules.py: require_rules (validated before any write, so a bad id leaves nothing half-linked), set_lesson_rules (set-semantics; a dropped rule becomes rejected, not forgotten), judge_link, and the two reads. ACL: write on the lesson (share-aware), ownership of the rule; a reader sees only rules they own. Decorations are fail-open (#4286). - MCP: create_lesson / update_lesson take rule_ids; get/create/update return `rules`; new judge_lesson_link tool. REST: the same on /api/lessons plus PUT /api/lessons//rules/. Rules: rule_detail carries `lessons`. - Backup v18: export (full and user-scoped, both ends in scope), builder, importer; both column guards register the table. - Tests: integration (states, set-semantics, judge, ACL all-or-nothing, cascade both ways, CHECK, one row per pair); unit (door wiring, judge registered, migration/model state agreement, backup skip and unjudged stays unjudged). conftest stubs the decorations for unit tests. Co-Authored-By: Claude Opus 5.5 --- alembic/versions/0111_lesson_rule_links.py | 54 +++++ src/scribe/mcp/tools/lessons.py | 51 +++- src/scribe/models/__init__.py | 2 + src/scribe/models/lesson_rule_link.py | 77 ++++++ src/scribe/routes/lessons.py | 41 ++++ src/scribe/services/backup.py | 70 +++++- src/scribe/services/lesson_rules.py | 244 ++++++++++++++++++++ src/scribe/services/rulebooks.py | 5 + tests/conftest.py | 21 ++ tests/test_integration_lesson_rule_links.py | 162 +++++++++++++ tests/test_lesson_rule_link_doors.py | 145 ++++++++++++ tests/test_services_backup.py | 9 +- 12 files changed, 875 insertions(+), 6 deletions(-) create mode 100644 alembic/versions/0111_lesson_rule_links.py create mode 100644 src/scribe/models/lesson_rule_link.py create mode 100644 src/scribe/services/lesson_rules.py create mode 100644 tests/test_integration_lesson_rule_links.py create mode 100644 tests/test_lesson_rule_link_doors.py diff --git a/alembic/versions/0111_lesson_rule_links.py b/alembic/versions/0111_lesson_rule_links.py new file mode 100644 index 0000000..96a2b35 --- /dev/null +++ b/alembic/versions/0111_lesson_rule_links.py @@ -0,0 +1,54 @@ +"""lesson_rule_links — a lesson points at the rule it is an instance of +(milestone 440 step 1, #4196) + +Revision ID: 0111 +Revises: 0110 +Create Date: 2026-10-01 + +One row per (lesson, rule) pair with a state: `suggested` while evidence +accumulates, `confirmed` or `rejected` once a judgment is made. Only a +confirmed link changes what surfaces; a rejected one is kept so the pair is not +proposed again. CASCADE on both ends — a link to a record that no longer exists +says nothing. No backfill: no lesson has ever been linked, and inventing a link +would assert a judgment nobody made. +""" +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql +from alembic import op + +revision = "0111" +down_revision = "0110" +branch_labels = None +depends_on = None + +# One place, so the CHECK and the model's LINK_STATES cannot drift (rule 36: +# a new value later means DROP + ADD CONSTRAINT in the same migration). +_STATES = ("suggested", "confirmed", "rejected") + + +def upgrade() -> None: + op.create_table( + "lesson_rule_links", + sa.Column("id", sa.BigInteger(), primary_key=True), + sa.Column("lesson_id", sa.Integer(), sa.ForeignKey("notes.id", ondelete="CASCADE"), nullable=False), + sa.Column("rule_id", sa.BigInteger(), sa.ForeignKey("rules.id", ondelete="CASCADE"), nullable=False), + sa.Column("state", sa.Text(), nullable=False, server_default="suggested"), + sa.Column("note", sa.Text(), nullable=True), + sa.Column("evidence", postgresql.JSONB(), nullable=True), + sa.Column("judged_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False, server_default=sa.text("now()")), + sa.UniqueConstraint("lesson_id", "rule_id", name="uq_lesson_rule_links_pair"), + ) + op.create_check_constraint( + "ck_lesson_rule_links_state", "lesson_rule_links", + "state IN (" + ", ".join(f"'{s}'" for s in _STATES) + ")", + ) + op.create_index("ix_lesson_rule_links_lesson_id", "lesson_rule_links", ["lesson_id"]) + op.create_index("ix_lesson_rule_links_rule_id", "lesson_rule_links", ["rule_id"]) + + +def downgrade() -> None: + op.drop_index("ix_lesson_rule_links_rule_id", table_name="lesson_rule_links") + op.drop_index("ix_lesson_rule_links_lesson_id", table_name="lesson_rule_links") + op.drop_constraint("ck_lesson_rule_links_state", "lesson_rule_links", type_="check") + op.drop_table("lesson_rule_links") diff --git a/src/scribe/mcp/tools/lessons.py b/src/scribe/mcp/tools/lessons.py index 0a35b1f..9e11293 100644 --- a/src/scribe/mcp/tools/lessons.py +++ b/src/scribe/mcp/tools/lessons.py @@ -13,6 +13,7 @@ from scribe.mcp._context import current_user_id from scribe.services import access as access_svc from scribe.services import dedup as dedup_svc from scribe.services import knowledge as knowledge_svc +from scribe.services import lesson_rules as lesson_rules_svc from scribe.services import lessons as lessons_svc from scribe.services import systems as systems_svc from scribe.services import trash as trash_svc @@ -89,6 +90,7 @@ async def create_lesson( tags: list[str] | None = None, project_id: int = 0, system_ids: list[int] | None = None, + rule_ids: list[int] | None = None, force: bool = False, ) -> dict: """Record something you LEARNED, so a later session meets it at the moment @@ -145,6 +147,12 @@ async def create_lesson( reach: a lesson is retrievable from every project (that is the point of the kind). 0 = none. system_ids: Systems (subsystems/areas) to file it under. + rule_ids: The rule(s) or preference(s) this lesson is an instance of — + the binding choice its situation falls under. A lesson never + becomes a rule; it points at the one that governs it, and the rule + is then reachable through the situation the lesson describes + (milestone 440). Naming a rule here confirms the link. Leave it + empty when no rule governs this situation. force: Create even if a near-duplicate exists. Returns the created lesson. On a near-duplicate, returns the existing id @@ -161,6 +169,9 @@ async def create_lesson( ) sources = lessons_svc.normalize_sources(learned_from) + # Validated before anything is written, so a lesson naming a rule the + # caller cannot read fails whole rather than saving half-linked. + linked = await lesson_rules_svc.require_rules(uid, rule_ids) title, body = lessons_svc.lesson_document( what, when_to_apply, insight, sources, ) @@ -179,8 +190,11 @@ async def create_lesson( ) if system_ids: await systems_svc.set_record_systems(uid, note.id, system_ids) + if linked: + await lesson_rules_svc.set_lesson_rules(uid, note.id, linked) data = _to_dict(note) await systems_tools.attach_systems(uid, uid, data, note.id, project_id or None) + await lesson_rules_svc.attach_lesson_rules(uid, [data]) return data @@ -214,6 +228,7 @@ async def get_lesson(lesson_id: int, project_id: int = 0) -> dict: # one that was true when it asked — otherwise every first read of a lesson # reports a pull that is its own. await attach_usage([out]) + await lesson_rules_svc.attach_lesson_rules(uid, [out]) record_pulled( user_id=uid, note_id=int(note.id), source="mcp_get_lesson", project_id=project_id, @@ -229,6 +244,7 @@ async def update_lesson( learned_from: list[int] | None = None, tags: list[str] | None = None, system_ids: list[int] | None = None, + rule_ids: list[int] | None = None, ) -> dict: """Update a lesson. Empty fields are left unchanged. @@ -258,8 +274,16 @@ async def update_lesson( when that argument gets dropped (#4249). A System tag is how `list_system_records` gathers an area's pile, so an untagged lesson is reachable by search and by nothing else. + rule_ids: Replace the rule(s) this lesson is an instance of. None + leaves unchanged; pass the FULL list. A rule that was linked and + is left out is recorded as REJECTED — "not an instance of this + one" — so the pair is not proposed again; `[]` rejects them all. """ uid = current_user_id() + linked = ( + await lesson_rules_svc.require_rules(uid, rule_ids) + if rule_ids is not None else None + ) note = await lessons_svc.update_lesson( uid, lesson_id, what=what or None, @@ -276,7 +300,30 @@ async def update_lesson( if system_ids is not None: await systems_svc.set_record_systems(uid, lesson_id, system_ids) note = await lessons_svc.get_lesson(uid, lesson_id) or note - return _to_dict(note) + if linked is not None: + await lesson_rules_svc.set_lesson_rules(uid, lesson_id, linked) + out = _to_dict(note) + await lesson_rules_svc.attach_lesson_rules(uid, [out]) + return out + + +async def judge_lesson_link( + lesson_id: int, rule_id: int, verdict: str, note: str = "", +) -> dict: + """Say whether a lesson is an instance of a rule: `verdict` is "confirm" + or "reject". + + Reach for this when Scribe proposes a pair — a lesson and a rule that keep + arriving together in different situations — or whenever you are reading a + lesson and recognise the rule it falls under. Confirming makes the rule + reachable through the situation the lesson describes; rejecting records + that it is not, so the pair is not proposed again. `note` is the why, and + the next reader judges the link by it. + + Returns the link: {lesson_id, rule_id, state, note, evidence, judged_at}. + """ + uid = current_user_id() + return await lesson_rules_svc.judge_link(uid, lesson_id, rule_id, verdict, note) async def delete_lesson(lesson_id: int) -> dict: @@ -312,5 +359,5 @@ async def delete_lesson(lesson_id: int) -> dict: def register(mcp) -> None: for fn in (list_lessons, create_lesson, get_lesson, update_lesson, - delete_lesson): + delete_lesson, judge_lesson_link): mcp.tool(name=fn.__name__)(fn) diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index abdcec7..4af89b9 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -77,6 +77,8 @@ from scribe.models.canonical_system import CanonicalSystem # noqa: E402, F401 from scribe.models.rulebook import ( # noqa: E402, F401 Rulebook, RulebookTopic, Rule, RuleRelation, rule_systems, ) +# After notes and rules: it foreign-keys both (milestone 440). +from scribe.models.lesson_rule_link import LessonRuleLink # noqa: E402, F401 from scribe.models.repo_binding import RepoBinding # noqa: E402, F401 from scribe.models.forge_connection import ForgeConnection # noqa: E402, F401 from scribe.models.code_shape import CodeShape, CodeShapeConsumer, CodeShapeEvent, CodeShapeUse # noqa: E402, F401 diff --git a/src/scribe/models/lesson_rule_link.py b/src/scribe/models/lesson_rule_link.py new file mode 100644 index 0000000..f9177d7 --- /dev/null +++ b/src/scribe/models/lesson_rule_link.py @@ -0,0 +1,77 @@ +from datetime import datetime + +from sqlalchemy import BigInteger, DateTime, ForeignKey, Integer, Text, UniqueConstraint +from sqlalchemy.dialects.postgresql import JSONB +from sqlalchemy.orm import Mapped, mapped_column + +from scribe.models import Base +from scribe.models.base import CreatedAtMixin, iso + +SUGGESTED = "suggested" +CONFIRMED = "confirmed" +REJECTED = "rejected" +# CHECK ck_lesson_rule_links_state (migration 0111, rule 36). +LINK_STATES = (SUGGESTED, CONFIRMED, REJECTED) + + +class LessonRuleLink(Base, CreatedAtMixin): + """A lesson says it is an instance of a rule (milestone 440, #4196). + + A lesson records one concrete situation; a rule records the binding choice + for a class of them. The link lets the rule be reached through the + situations that keep proving it, and lets a situation with lessons and no + rule be noticed. A lesson never BECOMES a rule — it points at one. + + ONE ROW PER PAIR, WITH A STATE, because the operator's design forms a link + in two stages ("a sort of soft and then hard link once it's been proven"): + + - ``suggested`` — evidence is accumulating that the two belong together + (they keep arriving in the same request, in distinct situations). It + carries nothing in retrieval: a suggested link that brought its rule + along would manufacture the co-surfacing it counts, and prove itself. + - ``confirmed`` — a judgment said the lesson is an instance of the rule. + The only state that changes what surfaces. + - ``rejected`` — a judgment said it is not. Kept, evidence and all, so the + pair is never proposed again and the reason stays readable. + + A table rather than a list on the lesson's `data`: the link needs a foreign + key on both ends (a deleted rule must not leave a lesson pointing at + nothing), a reverse index (a rule lists its lessons), and an id remap at + restore — none of which a JSON list gives. + """ + + __tablename__ = "lesson_rule_links" + + id: Mapped[int] = mapped_column(BigInteger, primary_key=True) + lesson_id: Mapped[int] = mapped_column( + Integer, ForeignKey("notes.id", ondelete="CASCADE"), index=True, + ) + rule_id: Mapped[int] = mapped_column( + BigInteger, ForeignKey("rules.id", ondelete="CASCADE"), index=True, + ) + state: Mapped[str] = mapped_column(Text, default=SUGGESTED, server_default=SUGGESTED) + # Why it was confirmed or rejected — the reasoning a later reader needs to + # decide whether it still holds, as a rule relation's `note` is. + note: Mapped[str | None] = mapped_column(Text, nullable=True) + # What the suggestion rests on (filled by the co-surfacing recorder, #4637). + # Kept after a judgment so a confirmation can be read beside its evidence. + evidence: Mapped[dict | None] = mapped_column(JSONB, nullable=True) + # When a judgment moved it out of `suggested`. Null while suggested. + judged_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True, + ) + + __table_args__ = ( + UniqueConstraint("lesson_id", "rule_id", name="uq_lesson_rule_links_pair"), + ) + + def to_dict(self) -> dict: + return { + "lesson_id": self.lesson_id, + "rule_id": self.rule_id, + "state": self.state, + "note": self.note or "", + "evidence": self.evidence or {}, + "judged_at": iso(self.judged_at), + "created_at": iso(self.created_at), + } diff --git a/src/scribe/routes/lessons.py b/src/scribe/routes/lessons.py index c9241a3..0335b07 100644 --- a/src/scribe/routes/lessons.py +++ b/src/scribe/routes/lessons.py @@ -28,6 +28,7 @@ from scribe.auth import get_current_user_id, login_required from scribe.routes.utils import not_found, parse_pagination from scribe.services import dedup as dedup_svc from scribe.services import knowledge as knowledge_svc +from scribe.services import lesson_rules as lesson_rules_svc from scribe.services import lessons as lessons_svc from scribe.services import systems as systems_svc from scribe.services import trash as trash_svc @@ -136,6 +137,12 @@ async def create_lesson_route(): project_id = data.get("project_id") or None learned_from = data.get("learned_from") or [] + # The rules this lesson is an instance of (milestone 440). Validated before + # anything is written, as the MCP door does, so a bad id saves nothing. + try: + linked = await lesson_rules_svc.require_rules(uid, data.get("rule_ids") or []) + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 # The same near-duplicate gate the MCP create path applies. Two lessons # under one trigger compete in a single ranked list for one reserved slot, @@ -165,10 +172,13 @@ async def create_lesson_route(): ) if data.get("system_ids") is not None: await systems_svc.set_record_systems(uid, note.id, data["system_ids"]) + if linked: + await lesson_rules_svc.set_lesson_rules(uid, note.id, linked) out = lessons_svc.lesson_to_dict(note) out["systems"] = [ s.to_dict() for s in await systems_svc.list_record_systems(uid, note.id) ] + await lesson_rules_svc.attach_lesson_rules(uid, [out]) return jsonify(out), 201 @@ -194,6 +204,7 @@ async def get_lesson_route(lesson_id: int): ) out.update(await describe_provenance(uid, note)) await attach_usage([out]) + await lesson_rules_svc.attach_lesson_rules(uid, [out]) # Opening the detail view IS a pull — the operator chose to look. Tagged # apart from the MCP sources so "an agent was handed it" and "a human read # it" stay distinguishable; they mean different things for pruning (#2085). @@ -233,9 +244,20 @@ async def update_lesson_route(lesson_id: int): ), }), 400 + linked = None + if data.get("rule_ids") is not None: + try: + linked = await lesson_rules_svc.require_rules(uid, data["rule_ids"]) + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 + updated = await lessons_svc.update_lesson(owner_uid, lesson_id, **kwargs) if updated is None: return not_found("Lesson") + if linked is not None: + # The CALLER, for the reason system_ids below uses it: the rules named + # must be ones the person making the edit can read. + await lesson_rules_svc.set_lesson_rules(uid, lesson_id, linked) if data.get("system_ids") is not None: # The CALLER, not owner_uid (#4249). `set_record_systems` runs its own # `can_write_note` and links only Systems the acting user can read; @@ -249,9 +271,28 @@ async def update_lesson_route(lesson_id: int): s.to_dict() for s in await systems_svc.list_record_systems(owner_uid, lesson_id) ] + await lesson_rules_svc.attach_lesson_rules(uid, [out]) return jsonify(out) +@lessons_bp.route("//rules/", methods=["PUT"]) +@login_required +async def judge_lesson_link_route(lesson_id: int, rule_id: int): + """Confirm or reject one lesson→rule link — the REST twin of the MCP + `judge_lesson_link`. Body: {"verdict": "confirm" | "reject", "note": "…"}.""" + uid = get_current_user_id() + data = await request.get_json() or {} + try: + link = await lesson_rules_svc.judge_link( + uid, lesson_id, rule_id, data.get("verdict", ""), data.get("note", ""), + ) + except PermissionError: + return jsonify({"error": "Permission denied"}), 403 + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 + return jsonify(link) + + @lessons_bp.route("/", methods=["DELETE"]) @login_required async def delete_lesson_route(lesson_id: int): diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 81e470c..335327e 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -16,6 +16,7 @@ from scribe.models.rule_usage import RuleUsageEvent from scribe.models.retrieval_tuning import RetrievalTuningEvent from scribe.models.canonical_system import CanonicalSystem from scribe.models.rulebook import RuleRelation, rule_systems as rule_systems_t +from scribe.models.lesson_rule_link import LessonRuleLink from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse from scribe.models.project import Project from scribe.models.repo_binding import RepoBinding @@ -80,8 +81,12 @@ logger = logging.getLogger(__name__) # say whether it still measures anything. Both travel NULLABLE and unfilled — # a row written before the stamp existed restores unstamped, because inventing # the model it was measured under would turn "unknown" into a stated fact. +# v18 (2026-10) added lesson_rule_links (milestone 440): which rule each lesson +# is an instance of, and the judgments that confirmed or rejected each pair. A +# confirmed link is a judgment nothing else records, and a rejected one is +# what stops the pair being proposed again — losing either undoes work. # Bump when the serialized schema changes. -BACKUP_VERSION = 17 +BACKUP_VERSION = 18 # Every table this backup carries, by its REAL name. Paired with _NOT_INCLUDED # below, these two lists must together account for the entire schema — which is @@ -119,6 +124,8 @@ _BACKED_UP = [ # on the operator's behalf, a restore that kept the numbers and dropped the # reasons would leave an install tuned by nobody it can name. "retrieval_tuning_events", + # v18 (2026-10): lesson → rule links and their judgments (milestone 440). + "lesson_rule_links", ] # Tables intentionally NOT in the backup, surfaced in the payload so the gap is @@ -212,6 +219,8 @@ _COLUMN_EXCLUSIONS: dict[str, set[str]] = { "record_systems": {"id", "created_at"}, "note_supersessions": {"id", "created_at"}, "rule_relations": {"id", "created_at"}, + # The pair is the row; everything else is the judgment and its evidence. + "lesson_rule_links": {"id"}, "note_usage_events": {"id"}, # Same as the note twin: the surrogate key is re-issued on insert. "rule_usage_events": {"id"}, @@ -299,6 +308,7 @@ _IMPORT_COLUMN_EXCLUSIONS: dict[str, set[str]] = { "record_systems": {"id", "created_at"}, "note_supersessions": {"id", "created_at"}, "rule_relations": {"id", "created_at"}, + "lesson_rule_links": {"id"}, "note_usage_events": {"id"}, "rule_usage_events": {"id"}, "retrieval_tuning_events": {"id"}, @@ -692,6 +702,21 @@ def _rule_relation_rows(rows) -> list[dict]: ] +def _lesson_rule_link_rows(rows) -> list[dict]: + """Which rule each lesson is an instance of, with the judgment's state, + reason and evidence (milestone 440). Ids are SOURCE ids, remapped through + the note and rule maps at restore.""" + return [ + { + "lesson_id": r.lesson_id, "rule_id": r.rule_id, "state": r.state, + "note": r.note, "evidence": r.evidence, + "judged_at": r.judged_at.isoformat() if r.judged_at else None, + "created_at": r.created_at.isoformat() if r.created_at else None, + } + for r in rows + ] + + def _rule_rows(rows) -> list[dict]: return [ { @@ -739,6 +764,7 @@ async def export_full_backup() -> dict: .join(CanonicalSystem, CanonicalSystem.id == rule_systems_t.c.canonical_id) )).all() rule_relations = (await session.execute(select(RuleRelation))).scalars().all() + lesson_rule_links = (await session.execute(select(LessonRuleLink))).scalars().all() record_systems = (await session.execute(select(RecordSystem))).scalars().all() supersessions = ( await session.execute(select(NoteSupersession)) @@ -797,6 +823,7 @@ async def export_full_backup() -> dict: "canonical_systems": _canonical_system_rows(canonical_systems), "rule_systems": _rule_system_rows(rule_system_rows), "rule_relations": _rule_relation_rows(rule_relations), + "lesson_rule_links": _lesson_rule_link_rows(lesson_rule_links), "systems": _system_rows( systems, {c.id: c.slug for c in canonical_systems} ), @@ -957,6 +984,14 @@ async def export_user_backup(user_id: int) -> dict: RuleRelation.to_rule_id.in_(_rule_ids), ) )).scalars().all() if _rule_ids else [] + # Both ends in THIS user's export, for the supersession reason: a link + # to a lesson or rule the import will not create restores as nothing. + lesson_rule_links = (await session.execute( + select(LessonRuleLink).where( + LessonRuleLink.lesson_id.in_(note_ids), + LessonRuleLink.rule_id.in_(_rule_ids), + ) + )).scalars().all() if (_rule_ids and note_ids) else [] return { "version": BACKUP_VERSION, @@ -984,6 +1019,7 @@ async def export_user_backup(user_id: int) -> dict: "canonical_systems": _canonical_system_rows(canonical_systems), "rule_systems": _rule_system_rows(rule_system_rows), "rule_relations": _rule_relation_rows(rule_relations), + "lesson_rule_links": _lesson_rule_link_rows(lesson_rule_links), "systems": _system_rows( systems, {c.id: c.slug for c in canonical_systems} ), @@ -1329,6 +1365,26 @@ def _build_rule_relation(row: dict, maps: _Maps) -> RuleRelation | None: ) +def _build_lesson_rule_link(row: dict, maps: _Maps) -> LessonRuleLink | None: + """Both ends must map: a link whose lesson or rule did not restore points + at whatever took that number in the destination.""" + lesson = maps.notes.get(row.get("lesson_id", 0)) + rule = maps.rules.get(row.get("rule_id", 0)) + if lesson is None or rule is None: + return None + return LessonRuleLink( + lesson_id=lesson, + rule_id=rule, + state=row.get("state") or "suggested", + note=row.get("note") or None, + evidence=row.get("evidence"), + # Kept absent when absent: a suggested link was never judged, and + # stamping it with the restore time would say it was. + judged_at=_dt_or_none(row.get("judged_at")), + created_at=_dt(row.get("created_at")), + ) + + def _build_rule_version(row: dict, maps: _Maps) -> RuleVersion | None: rid = maps.rules.get(row.get("rule_id", 0)) if rid is None: @@ -1725,7 +1781,7 @@ async def _restore_v2(data: dict) -> dict: "note_supersessions": 0, "code_shapes": 0, "code_shape_events": 0, "code_shape_uses": 0, "canonical_systems": 0, "rule_systems": 0, "rule_relations": 0, "rule_versions": 0, - "retrieval_tuning_events": 0, + "retrieval_tuning_events": 0, "lesson_rule_links": 0, } async with async_session() as session: @@ -1920,6 +1976,16 @@ async def _restore_v2(data: dict) -> dict: session.add(relation) stats["rule_relations"] += 1 + # Lesson → rule links (milestone 440): after both notes and rules are + # mapped, which is why they sit beside the rule edges. Archives before + # v18 carry no section and restore with none. + for lr in data.get("lesson_rule_links", []): + link = _build_lesson_rule_link(lr, maps) + if link is None: + continue + session.add(link) + stats["lesson_rule_links"] += 1 + # A rule's edit history (milestone 323). Must come after the rules # themselves — the rule map is only populated above — and both ids are # ids in the SOURCE database, which is #3182's arose_from_id trap. diff --git a/src/scribe/services/lesson_rules.py b/src/scribe/services/lesson_rules.py new file mode 100644 index 0000000..63c27ad --- /dev/null +++ b/src/scribe/services/lesson_rules.py @@ -0,0 +1,244 @@ +"""Lessons point at rules (milestone 440, #4196). + +A lesson is a non-binding record of one situation; a rule is the binding +choice for a class of them. This service owns the link between the two — which +rule a lesson is an instance of — and both directions of reading it. + +THE STATES (models/lesson_rule_link.py says why each exists): +`suggested` while evidence accumulates, `confirmed` or `rejected` once a +judgment is made. Every write here is a JUDGMENT, so every write lands as +confirmed or rejected; `suggested` rows come from the co-surfacing recorder +(#4637), never from a caller naming a rule. + +ACL (rule 78). Linking changes what a lesson says about itself, so it needs +WRITE on the lesson (`access.can_write_note`, share-aware). It names a rule, so +it needs the rule to be one the caller may read — rules are owner-scoped, and +`rulebooks._fetch_owned_rule` / `_owned_rules_clause` are that check's two +forms. A read shows only the rules the READER owns: a lesson shared with +someone must not hand them the titles of its owner's private rules. +""" +from __future__ import annotations + +import logging +from datetime import datetime, timezone + +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.lesson_rule_link import CONFIRMED, REJECTED, SUGGESTED, LessonRuleLink +from scribe.models.note import Note +from scribe.models.rulebook import Rule + +logger = logging.getLogger(__name__) + +# What a caller says to judge one pair. Words rather than the stored states, +# because "confirm" and "reject" are acts and the states are their results. +VERDICTS = {"confirm": CONFIRMED, "reject": REJECTED} + +# Recorded on a link that set-semantics removed, so a reader of the rejected +# row can tell an explicit "not this rule" from a link dropped by a rewrite. +_REMOVED_NOTE = "removed from the lesson's rules by an update" + + +def _ids(values) -> list[int]: + """Positive ints, de-duplicated, in the order given.""" + out: list[int] = [] + for v in values or []: + try: + i = int(v) + except (TypeError, ValueError): + raise ValueError(f"rule id {v!r} is not an integer") + if i > 0 and i not in out: + out.append(i) + return out + + +async def _require_lesson_writable(user_id: int, lesson_id: int) -> None: + from scribe.services import access + from scribe.services import lessons as lessons_svc + + note = await lessons_svc.get_lesson(user_id, lesson_id) + if note is None: + raise ValueError(f"lesson {lesson_id} not found") + if not await access.can_write_note(user_id, lesson_id): + raise PermissionError(f"lesson {lesson_id} is not yours to change") + + +async def require_rules(user_id: int, rule_ids) -> list[int]: + """The ids, validated as rules the caller owns — all of them or none. + + Checked BEFORE anything is written, by every caller, so a lesson create + that names a rule it cannot see fails without leaving a half-linked + lesson behind. + """ + from scribe.services import rulebooks as rulebooks_svc + + wanted = _ids(rule_ids) + missing = [rid for rid in wanted if await rulebooks_svc.get_rule(rid, user_id) is None] + if missing: + raise ValueError( + f"rule(s) {missing} not found — a lesson can point only at a rule " + "you can read. Nothing was linked." + ) + return wanted + + +async def _upsert(session, lesson_id: int, rule_id: int, state: str, note: str) -> None: + now = datetime.now(timezone.utc) + row = (await session.execute( + select(LessonRuleLink).where( + LessonRuleLink.lesson_id == lesson_id, + LessonRuleLink.rule_id == rule_id, + ) + )).scalar_one_or_none() + if row is None: + session.add(LessonRuleLink( + lesson_id=lesson_id, rule_id=rule_id, state=state, + note=note or None, judged_at=now, + )) + return + # Evidence is kept across a judgment: a confirmation reads best beside + # what it rested on. + row.state = state + row.note = note or row.note + row.judged_at = now + + +async def set_lesson_rules( + user_id: int, lesson_id: int, rule_ids, *, note: str = "", +) -> None: + """Make the lesson's CONFIRMED rules exactly `rule_ids` (set-semantics). + + A rule named here is confirmed, whatever state it was in: the writer of a + lesson saying "this is an instance of rule N" is the judgment the + suggested state waits for, so it needs no evidence bar. + + A rule that WAS confirmed and is no longer named becomes `rejected`, not + deleted. Dropping it is a judgment that the lesson is not an instance of + that rule, and a deleted row would let the co-surfacing recorder propose + the same pair again. + """ + await _require_lesson_writable(user_id, lesson_id) + wanted = await require_rules(user_id, rule_ids) + async with async_session() as session: + current = (await session.execute( + select(LessonRuleLink).where( + LessonRuleLink.lesson_id == lesson_id, + LessonRuleLink.state == CONFIRMED, + ) + )).scalars().all() + for row in current: + if row.rule_id not in wanted: + row.state = REJECTED + row.note = _REMOVED_NOTE + row.judged_at = datetime.now(timezone.utc) + for rid in wanted: + await _upsert(session, lesson_id, rid, CONFIRMED, note) + await session.commit() + + +async def judge_link( + user_id: int, lesson_id: int, rule_id: int, verdict: str, note: str = "", +) -> dict: + """Confirm or reject one (lesson, rule) pair, suggested or not.""" + state = VERDICTS.get((verdict or "").strip().lower()) + if state is None: + raise ValueError(f"verdict must be one of {sorted(VERDICTS)}, got {verdict!r}") + await _require_lesson_writable(user_id, lesson_id) + [rid] = await require_rules(user_id, [rule_id]) + async with async_session() as session: + await _upsert(session, lesson_id, rid, state, note) + await session.commit() + row = (await session.execute( + select(LessonRuleLink).where( + LessonRuleLink.lesson_id == lesson_id, + LessonRuleLink.rule_id == rid, + ) + )).scalar_one() + return row.to_dict() + + +async def rules_for_lessons(user_id: int, lesson_ids) -> dict[int, list[dict]]: + """{lesson_id: [{id, title, kind, state, note}]}, for rules the READER owns. + + One query for the whole set — a lesson list would otherwise be N+1. + Confirmed first, then suggested, then rejected: the order a reader cares + about them in. + """ + from scribe.services.rulebooks import _owned_rules_clause + + ids = [int(i) for i in lesson_ids or []] + out: dict[int, list[dict]] = {i: [] for i in ids} + if not ids: + return out + order = {s: n for n, s in enumerate((CONFIRMED, SUGGESTED, REJECTED))} + async with async_session() as session: + rows = (await session.execute( + select(LessonRuleLink, Rule) + .join(Rule, Rule.id == LessonRuleLink.rule_id) + .where(LessonRuleLink.lesson_id.in_(ids)) + .where(_owned_rules_clause(user_id)) + )).all() + for link, rule in sorted(rows, key=lambda r: (order.get(r[0].state, 9), r[1].id)): + out[link.lesson_id].append({ + "id": rule.id, "title": rule.title, "kind": rule.kind, + "state": link.state, "note": link.note or "", + }) + return out + + +async def lessons_for_rule(user_id: int, rule_id: int) -> list[dict]: + """The lessons that point at one rule, readable by the caller (share-aware). + + The reverse direction, and the one a rule's page needs: the concrete + situations that have been judged instances of it. + """ + from scribe.services.access import readable_notes_clause + + async with async_session() as session: + rows = (await session.execute( + select(LessonRuleLink, Note) + .join(Note, Note.id == LessonRuleLink.lesson_id) + .where(LessonRuleLink.rule_id == int(rule_id)) + .where(Note.deleted_at.is_(None)) + .where(readable_notes_clause(user_id)) + )).all() + order = {s: n for n, s in enumerate((CONFIRMED, SUGGESTED, REJECTED))} + return [ + {"id": note.id, "title": note.title, "state": link.state, "note": link.note or ""} + for link, note in sorted(rows, key=lambda r: (order.get(r[0].state, 9), r[1].id)) + ] + + +async def attach_lesson_rules(user_id: int, rows: list[dict], *, key: str = "id") -> None: + """Add `rules` to each lesson payload row, in place — one query per page. + + Fail-open (snippet #4286): this decorates a lesson the caller already has, + so a failed lookup leaves the key off and logs, rather than refusing the + lesson. An absent key reads as "not attached", never as "no rule". + """ + ids = [int(r[key]) for r in rows if isinstance(r.get(key), int)] + try: + found = await rules_for_lessons(user_id, ids) + except Exception: + logger.warning("lesson→rule links could not be read", exc_info=True) + return + for r in rows: + if isinstance(r.get(key), int): + r["rules"] = found.get(r[key], []) + + +async def attach_rule_lessons(user_id: int, data: dict, rule_id: int) -> None: + """Add `lessons` to a rule payload, in place, when there are any. + + Present-only, as a rule's `systems` and `relations` are (#2483), and + fail-open for the reason `attach_lesson_rules` gives. + """ + try: + lessons = await lessons_for_rule(user_id, rule_id) + except Exception: + logger.warning("rule→lesson links could not be read", exc_info=True) + return + if lessons: + data["lessons"] = lessons + diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index 57b37c1..02557a4 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -455,6 +455,11 @@ async def rule_detail(user_id: int, rule: Rule, system_ids: list[int] | None = N data["systems"] = systems if relations: data["relations"] = relations + # The concrete situations judged (or proposed) to be instances of this + # rule — milestone 440. Same present-only convention as the two above. + from scribe.services.lesson_rules import attach_rule_lessons + + await attach_rule_lessons(user_id, data, rule.id) return data diff --git a/tests/conftest.py b/tests/conftest.py index c5ce140..b64e527 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -142,6 +142,27 @@ def _no_task_log_arm(): yield +@pytest.fixture(autouse=True) +def _no_lesson_rule_links(request): + """Stub the lesson↔rule link decorations (milestone 440). + + Every door that returns a lesson or a rule now attaches its links, and the + read is a real database call — so every unit test that opens either would + otherwise reach for the fake DATABASE_URL to learn that nothing is linked. + Both decorations are fail-open, so the cost would be a slow failed connect + per test rather than a failure, which is worse: it would never be noticed. + + Skipped for integration tests, which exercise the real links against + Postgres (tests/test_integration_lesson_rule_links.py). + """ + if request.node.get_closest_marker("integration"): + yield + return + with patch("scribe.services.lesson_rules.attach_lesson_rules", AsyncMock()), \ + patch("scribe.services.lesson_rules.attach_rule_lessons", AsyncMock()): + yield + + @pytest.fixture(autouse=True) def _no_rule_arm(): """Stub the write-path hint's standing-RULES arm (milestone 307). diff --git a/tests/test_integration_lesson_rule_links.py b/tests/test_integration_lesson_rule_links.py new file mode 100644 index 0000000..5156bcb --- /dev/null +++ b/tests/test_integration_lesson_rule_links.py @@ -0,0 +1,162 @@ +"""Real-Postgres tests for lesson → rule links (milestone 440 step 1, #4630). + +What a mock cannot show: that the CHECK holds the three states, that deleting +either end takes the link with it, that a rule the caller does not own cannot +be linked (and nothing is half-written when one id is bad), and that each side +reads only what its reader may see. +""" +import uuid +from unittest.mock import MagicMock, patch + +import pytest +import pytest_asyncio +from sqlalchemy import delete, select +from sqlalchemy.exc import IntegrityError + +from scribe.models import async_session +from scribe.models.lesson_rule_link import LessonRuleLink +from scribe.models.note import Note +from scribe.models.project import Project +from scribe.models.rulebook import Rule +from scribe.services import lesson_rules as links_svc +from scribe.services import lessons as lessons_svc +from scribe.services import rulebooks as rulebooks_svc +from tests.helpers import ensure_user + +pytestmark = [ + pytest.mark.integration, + pytest.mark.usefixtures("_dispose_engine", "_no_embedding"), +] + + +@pytest.fixture(autouse=True) +def _no_reindex(): + """Rule writes detach an embedding refresh that outlives the test's loop.""" + with patch("scribe.services.rulebooks._refresh_rule_embedding", MagicMock()): + yield + + +@pytest_asyncio.fixture +async def world(): + """An owner with a lesson and two rules; a stranger with a rule of their own.""" + tag = uuid.uuid4().hex[:8] + async with async_session() as s: + owner = await ensure_user(s, f"lrl_owner_{tag}") + stranger = await ensure_user(s, f"lrl_stranger_{tag}") + mine = Project(user_id=owner.id, title="Mine") + theirs = Project(user_id=stranger.id, title="Theirs") + s.add_all([mine, theirs]) + await s.flush() + ids = {"owner": owner.id, "stranger": stranger.id, + "mine": mine.id, "theirs": theirs.id} + await s.commit() + + owner = ids["owner"] + r1 = await rulebooks_svc.create_project_rule( + ids["mine"], owner, "Read the job log first", "Before waiting longer.", + when_to_apply="a CI run has overrun its usual duration", + ) + r2 = await rulebooks_svc.create_project_rule( + ids["mine"], owner, "Probe the system itself", "Not a proxy for it.", + when_to_apply="about to state what version is deployed", + ) + foreign = await rulebooks_svc.create_project_rule( + ids["theirs"], ids["stranger"], "Their rule", "Not yours.", + when_to_apply="something only they do", + ) + lesson = await lessons_svc.create_lesson( + owner, what="An overrun run usually failed early", + when_to_apply="a CI run is still in_progress far past its usual time", + insight="Read the log; the failure is often minutes old.", + project_id=ids["mine"], + ) + ids.update(r1=r1.id, r2=r2.id, foreign=foreign.id, lesson=lesson.id) + return ids + + +async def _states(lesson_id: int) -> dict[int, str]: + async with async_session() as s: + rows = (await s.execute( + select(LessonRuleLink).where(LessonRuleLink.lesson_id == lesson_id) + )).scalars().all() + return {r.rule_id: r.state for r in rows} + + +async def test_naming_rules_confirms_them_and_both_sides_read_the_link(world): + owner = world["owner"] + await links_svc.set_lesson_rules(owner, world["lesson"], [world["r1"], world["r2"]]) + + assert await _states(world["lesson"]) == {world["r1"]: "confirmed", world["r2"]: "confirmed"} + rules = (await links_svc.rules_for_lessons(owner, [world["lesson"]]))[world["lesson"]] + assert {r["id"] for r in rules} == {world["r1"], world["r2"]} + lessons = await links_svc.lessons_for_rule(owner, world["r1"]) + assert [(l["id"], l["state"]) for l in lessons] == [(world["lesson"], "confirmed")] + + +async def test_a_rule_left_out_of_the_set_is_rejected_not_forgotten(world): + owner = world["owner"] + await links_svc.set_lesson_rules(owner, world["lesson"], [world["r1"], world["r2"]]) + await links_svc.set_lesson_rules(owner, world["lesson"], [world["r1"]]) + + assert await _states(world["lesson"]) == {world["r1"]: "confirmed", world["r2"]: "rejected"} + rules = (await links_svc.rules_for_lessons(owner, [world["lesson"]]))[world["lesson"]] + # Confirmed reads first; the rejection keeps its reason. + assert [r["state"] for r in rules] == ["confirmed", "rejected"] + assert rules[1]["note"] == links_svc._REMOVED_NOTE + + +async def test_judge_moves_a_pair_both_ways_and_refuses_a_nonsense_verdict(world): + owner = world["owner"] + out = await links_svc.judge_link(owner, world["lesson"], world["r1"], "reject", "different failure") + assert (out["state"], out["note"]) == ("rejected", "different failure") + assert out["judged_at"] is not None + out = await links_svc.judge_link(owner, world["lesson"], world["r1"], "confirm") + assert out["state"] == "confirmed" + with pytest.raises(ValueError): + await links_svc.judge_link(owner, world["lesson"], world["r1"], "maybe") + + +async def test_a_rule_the_caller_cannot_read_links_nothing_at_all(world): + """All-or-nothing: one unreadable id refuses the whole set, so the readable + one beside it is not linked either.""" + with pytest.raises(ValueError): + await links_svc.set_lesson_rules( + world["owner"], world["lesson"], [world["r1"], world["foreign"]], + ) + assert await _states(world["lesson"]) == {} + + +async def test_a_stranger_cannot_link_someone_elses_lesson(world): + with pytest.raises((ValueError, PermissionError)): + await links_svc.set_lesson_rules(world["stranger"], world["lesson"], [world["foreign"]]) + assert await _states(world["lesson"]) == {} + + +async def test_deleting_either_end_takes_the_link_with_it(world): + owner = world["owner"] + await links_svc.set_lesson_rules(owner, world["lesson"], [world["r1"], world["r2"]]) + async with async_session() as s: + await s.execute(delete(Rule).where(Rule.id == world["r1"])) + await s.commit() + assert await _states(world["lesson"]) == {world["r2"]: "confirmed"} + async with async_session() as s: + await s.execute(delete(Note).where(Note.id == world["lesson"])) + await s.commit() + assert await _states(world["lesson"]) == {} + + +async def test_the_check_holds_the_three_states(world): + async with async_session() as s: + s.add(LessonRuleLink(lesson_id=world["lesson"], rule_id=world["r1"], state="maybe")) + with pytest.raises(IntegrityError): + await s.commit() + + +async def test_one_row_per_pair(world): + async with async_session() as s: + s.add_all([ + LessonRuleLink(lesson_id=world["lesson"], rule_id=world["r1"], state="suggested"), + LessonRuleLink(lesson_id=world["lesson"], rule_id=world["r1"], state="confirmed"), + ]) + with pytest.raises(IntegrityError): + await s.commit() diff --git a/tests/test_lesson_rule_link_doors.py b/tests/test_lesson_rule_link_doors.py new file mode 100644 index 0000000..8715fad --- /dev/null +++ b/tests/test_lesson_rule_link_doors.py @@ -0,0 +1,145 @@ +"""The lesson → rule link at its doors (milestone 440 step 1, #4630). + +The link's own behaviour — states, cascade, ACL — is tested against Postgres in +tests/test_integration_lesson_rule_links.py. These pin the wiring a mock CAN +see: that a bad rule id stops a lesson create before anything is written, that +the doors pass the set through with the semantics their docstrings promise, +that the judge tool is registered, that backup skips a link it cannot map, and +that the migration and the model agree about the states. +""" +from __future__ import annotations + +import importlib.util +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.mcp._context import _user_id_ctx +from scribe.mcp.tools import lessons as lesson_tools +from scribe.models.lesson_rule_link import LINK_STATES +from scribe.services import backup +from scribe.services import lesson_rules as links_svc +from scribe.services import lessons as lessons_svc + +TRIGGER = "a CI run is still in_progress far past its usual time" + + +def _stub_note(**kw): + base = dict( + id=41, 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) + + +def _create_patches(created): + return ( + 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()), + ) + + +@pytest.mark.asyncio +async def test_an_unreadable_rule_stops_the_lesson_before_it_is_written(): + """All-or-nothing at the door: the rule ids are validated BEFORE the + lesson exists, so a bad id cannot leave a lesson saved and unlinked.""" + _user_id_ctx.set(7) + created = AsyncMock(return_value=_stub_note()) + p1, p2, p3 = _create_patches(created) + with p1, p2, p3, patch.object( + links_svc, "require_rules", AsyncMock(side_effect=ValueError("rule(s) [9] not found")), + ): + with pytest.raises(ValueError): + await lesson_tools.create_lesson(what="x", when_to_apply=TRIGGER, rule_ids=[9]) + created.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_create_links_the_named_rules_to_the_new_lesson(): + _user_id_ctx.set(7) + created = AsyncMock(return_value=_stub_note(id=41)) + linked = AsyncMock() + p1, p2, p3 = _create_patches(created) + with p1, p2, p3, \ + patch.object(links_svc, "require_rules", AsyncMock(return_value=[5, 6])), \ + patch.object(links_svc, "set_lesson_rules", linked): + await lesson_tools.create_lesson(what="x", when_to_apply=TRIGGER, rule_ids=[5, 6]) + linked.assert_awaited_once() + assert linked.await_args.args[1:] == (41, [5, 6]) + + +@pytest.mark.asyncio +async def test_create_without_rules_writes_no_link(): + _user_id_ctx.set(7) + linked = AsyncMock() + p1, p2, p3 = _create_patches(AsyncMock(return_value=_stub_note())) + with p1, p2, p3, patch.object(links_svc, "set_lesson_rules", linked): + await lesson_tools.create_lesson(what="x", when_to_apply=TRIGGER) + linked.assert_not_awaited() + + +@pytest.mark.parametrize("rule_ids, expect_call", [(None, False), ([], True), ([5], True)]) +@pytest.mark.asyncio +async def test_update_leaves_links_alone_on_none_and_replaces_on_a_list(rule_ids, expect_call): + """None is "unchanged"; a list — including [] — is the full new set, which + the service turns into confirmations and rejections.""" + _user_id_ctx.set(7) + linked = AsyncMock() + with patch.object(lessons_svc, "update_lesson", AsyncMock(return_value=_stub_note())), \ + patch.object(links_svc, "require_rules", AsyncMock(side_effect=lambda uid, ids: list(ids))), \ + patch.object(links_svc, "set_lesson_rules", linked): + await lesson_tools.update_lesson(lesson_id=41, rule_ids=rule_ids) + assert linked.await_count == (1 if expect_call else 0) + if expect_call: + assert linked.await_args.args[1:] == (41, rule_ids) + + +def test_the_judge_tool_is_registered(): + names = [] + fake = SimpleNamespace(tool=lambda name: (lambda fn: names.append(name) or fn)) + lesson_tools.register(fake) + assert "judge_lesson_link" in names + + +def test_verdicts_name_real_states(): + assert set(links_svc.VERDICTS.values()) <= set(LINK_STATES) + + +def test_the_migration_check_and_the_model_agree_on_the_states(): + """Rule 36's drift, guarded: the CHECK is written from the migration's + tuple and the code from the model's, so they must be the same tuple.""" + path = Path(__file__).resolve().parents[1] / "alembic" / "versions" / "0111_lesson_rule_links.py" + spec = importlib.util.spec_from_file_location("m0111", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + assert tuple(module._STATES) == tuple(LINK_STATES) + + +@pytest.mark.parametrize("lesson_mapped, rule_mapped", [(False, True), (True, False)]) +def test_backup_skips_a_link_whose_end_did_not_restore(lesson_mapped, rule_mapped): + maps = backup._Maps() + if lesson_mapped: + maps.notes[10] = 110 + if rule_mapped: + maps.rules[20] = 120 + row = {"lesson_id": 10, "rule_id": 20, "state": "confirmed"} + assert backup._build_lesson_rule_link(row, maps) is None + + +def test_backup_keeps_an_unjudged_link_unjudged(): + """A suggested link was never judged; restoring it with the restore's time + in judged_at would say it was.""" + maps = backup._Maps() + maps.notes[10] = 110 + maps.rules[20] = 120 + built = backup._build_lesson_rule_link( + {"lesson_id": 10, "rule_id": 20, "state": "suggested", "judged_at": None}, maps, + ) + assert (built.lesson_id, built.rule_id, built.state) == (110, 120, "suggested") + assert built.judged_at is None diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index cc45bae..56a6978 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -27,7 +27,7 @@ def test_backup_version_is_current(): (Named for the number it asserted until v10, which is exactly the drift a name-carrying-a-value invites; it now says what it checks.)""" - assert backup.BACKUP_VERSION == 17 + assert backup.BACKUP_VERSION == 18 def _exportable_note(**over): @@ -131,6 +131,7 @@ def test_a_repo_binding_carries_the_branch_its_ledger_follows(): def _column_guard_targets(): from scribe.models.canonical_system import CanonicalSystem from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse + from scribe.models.lesson_rule_link import LessonRuleLink from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.milestone import Milestone from scribe.models.note import Note @@ -167,6 +168,7 @@ def _column_guard_targets(): "record_systems": (RecordSystem, backup._record_system_rows), "note_supersessions": (NoteSupersession, backup._note_supersession_rows), "rule_relations": (RuleRelation, backup._rule_relation_rows), + "lesson_rule_links": (LessonRuleLink, backup._lesson_rule_link_rows), "note_usage_events": (NoteUsageEvent, backup._usage_event_rows), "rule_usage_events": (RuleUsageEvent, backup._rule_usage_event_rows), "retrieval_tuning_events": ( @@ -283,6 +285,7 @@ def _import_guard_targets(): "record_systems": backup._build_record_system, "note_supersessions": backup._build_note_supersession, "rule_relations": backup._build_rule_relation, + "lesson_rule_links": backup._build_lesson_rule_link, "note_usage_events": backup._build_usage_event, "rule_usage_events": backup._build_rule_usage_event, "retrieval_tuning_events": backup._build_retrieval_tuning_event, @@ -534,7 +537,9 @@ async def test_export_full_backup_contains_every_declared_section(): "note_supersessions", "code_shapes", "code_shape_events", "code_shape_uses", # v16: the reasons beside the settings they explain. - "retrieval_tuning_events"): + "retrieval_tuning_events", + # v18: which rule each lesson is an instance of. + "lesson_rule_links"): assert key in out, f"missing export section: {key}" assert out[key] == [] From c8393975c3d4bf62716318d537a2b1e481569f5e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 1 Oct 2026 12:44:06 -0400 Subject: [PATCH 3/5] fix(mcp): classify judge_lesson_link as a write tool (#4630) The tool-classification guard (test_mcp_auth) failed CI run 705: the new judge_lesson_link tool sat in no set, so a read key would have been silently denied it. It changes a link's state, so it belongs in _WRITE_TOOLS. Co-Authored-By: Claude Opus 5.5 --- src/scribe/mcp/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 6957397..8bdb3ba 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -161,7 +161,7 @@ _READ_ONLY_TOOLS = frozenset({ _WRITE_TOOLS = frozenset({ # notes, tasks, planning "create_note", "update_note", "delete_note", - "create_lesson", "update_lesson", "delete_lesson", + "create_lesson", "update_lesson", "delete_lesson", "judge_lesson_link", "create_task", "update_task", "delete_task", "add_task_log", "create_records", "start_planning", "create_milestone", "update_milestone", "delete_milestone", From f7d8dc2e55b41742c27529b1a15d479854d57d52 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 1 Oct 2026 13:25:27 -0400 Subject: [PATCH 4/5] =?UTF-8?q?feat(lessons):=20judged=20when=20written=20?= =?UTF-8?q?=E2=80=94=20a=20new=20lesson=20is=20offered=20its=20rules,=20an?= =?UTF-8?q?d=20"no=20rule=20fits"=20is=20an=20answer=20(milestone=20440=20?= =?UTF-8?q?step=202,=20#4631)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Which rule is this lesson an instance of?" now has three recorded answers: a rule named (a confirmed link, #4630), no rule fits (new), or unjudged. - Model + migration 0112: lesson_no_rule (lesson_id PK, CASCADE from the note; why; judged_at). A table rather than a key in notes.data, because that mirror is re-composed from the body on every edit and would erase it. - Service (lesson_rules): set_no_rule rejects any confirmed link with the reason; a confirmation (set_lesson_rules or judge_link) deletes the answer; require_one_answer refuses both answers in one call before any write; judgments_for_lessons + attach_lesson_rules add rule_judgment (and no_rule) to every lesson payload; list_unjudged lists the open ones; rule_candidates searches rules with the lesson's claim + trigger at the explicit-search bar, None when the search could not run. - MCP: create_lesson/update_lesson take no_rule; an unanswered create returns rule_candidates, rule_judgment and a rule_hint; list_lessons(unjudged=true). - REST: the same on POST/PATCH /api/lessons and GET ?unjudged=1; create returns rule_candidates. - Backup v19: a lesson_no_rule section, export (full and per-user) and import. - Guidance: create_lesson docstring, writing-records.md in using-scribe (owner, pinned in test_guidance_ownership), create_rule docstring on linking the lessons a new rule governs. Plugin version minted. - Tests: door units, integration for the three states, the rejection reason, scoping, cascade; backup registries. Co-Authored-By: Claude Opus 5.5 --- alembic/versions/0112_lesson_no_rule.py | 33 +++ plugin/.claude-plugin/plugin.json | 2 +- plugin/skills/using-scribe/writing-records.md | 24 ++ src/scribe/mcp/tools/lessons.py | 89 +++++++- src/scribe/mcp/tools/rulebooks.py | 8 + src/scribe/models/__init__.py | 2 +- src/scribe/models/lesson_rule_link.py | 37 ++- src/scribe/routes/lessons.py | 53 +++-- src/scribe/services/backup.py | 48 +++- src/scribe/services/lesson_rules.py | 210 +++++++++++++++++- tests/conftest.py | 7 +- tests/test_guidance_ownership.py | 9 + tests/test_integration_lesson_rule_links.py | 81 +++++++ tests/test_lesson_rule_link_doors.py | 120 ++++++++++ tests/test_services_backup.py | 10 +- 15 files changed, 696 insertions(+), 37 deletions(-) create mode 100644 alembic/versions/0112_lesson_no_rule.py diff --git a/alembic/versions/0112_lesson_no_rule.py b/alembic/versions/0112_lesson_no_rule.py new file mode 100644 index 0000000..da806af --- /dev/null +++ b/alembic/versions/0112_lesson_no_rule.py @@ -0,0 +1,33 @@ +"""lesson_no_rule — "no rule fits" is a recorded answer (milestone 440 step 2, +#4631) + +Revision ID: 0112 +Revises: 0111 +Create Date: 2026-10-01 + +One row per lesson that was judged to fall under no rule, with the reason. A +lesson with neither a confirmed link nor this row is UNJUDGED, which is what +`list_lessons(unjudged=true)` lists. CASCADE from the lesson. No backfill: no +lesson has been judged yet, and writing "no rule fits" for one nobody looked +at would assert a judgment nobody made. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0112" +down_revision = "0111" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "lesson_no_rule", + sa.Column("lesson_id", sa.Integer(), sa.ForeignKey("notes.id", ondelete="CASCADE"), primary_key=True), + sa.Column("why", sa.Text(), nullable=False), + sa.Column("judged_at", sa.DateTime(timezone=True), nullable=False, server_default=sa.text("now()")), + ) + + +def downgrade() -> None: + op.drop_table("lesson_no_rule") diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 45eb2b3..a6ac874 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe for Claude Code: connects the scribe MCP server, adds the hooks that deliver live project state and relevant records at the right moment, ships the shared client-neutral Scribe skills (using-scribe, writing-plans, reporting-back, systematic-debugging, verification, brainstorming, reusing-code, shape-accounting), and syncs your saved Scribe Processes as skills (/scribe:sync).", - "version": "2026.10.01.1246", + "version": "2026.10.01.1724", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/using-scribe/writing-records.md b/plugin/skills/using-scribe/writing-records.md index 9876571..0c86018 100644 --- a/plugin/skills/using-scribe/writing-records.md +++ b/plugin/skills/using-scribe/writing-records.md @@ -8,6 +8,7 @@ arrives that names the situation you are actually in; and before filling ## Contents - Where a new rule goes — its home, its trigger, what already covers the moment - A lesson grows each time it proves itself +- A lesson names the rule it is an instance of - A note that asserts a fact can carry its own check ## Where a new rule goes @@ -76,6 +77,29 @@ waiting in a situation nobody is in. One claim that has met the same failure four times is worth more than four claims that each met it once, so when a near-duplicate create hands back an existing id, that is the record to grow. +## A lesson names the rule it is an instance of + +A lesson records one situation; a rule records the binding choice for a class +of them. Linking the two is what lets a rule be reached through the situations +that keep proving it — moments that resemble why it was written more closely +than its own wording does. A lesson never becomes a rule. It points at one. + +The writer is best placed to say which rule, so answer while writing, in +`create_lesson` itself or in the `update_lesson` right after it: + +- **`rule_ids=[…]`** — the rule or preference whose situation this is an + instance of. +- **`no_rule="why"`** — no rule governs it, in a line. That is a full answer, + not a gap: lessons that stand alone and keep landing in one situation are + what a missing rule looks like, and the reason is what lets the answer be + re-judged once a rule exists. + +A lesson created with neither comes back with `rule_candidates`, the rules it +most resembles, each with its trigger. Read them against the situation the +lesson describes, not its topic — a rule about the same subsystem that governs +a different moment is not its rule — and give one of the two answers. +`list_lessons(unjudged=true)` gathers the ones still open. + ## A note that asserts a fact can carry its own check **A few notes assert a FACT, and those can carry their own check.** diff --git a/src/scribe/mcp/tools/lessons.py b/src/scribe/mcp/tools/lessons.py index 9e11293..3f116eb 100644 --- a/src/scribe/mcp/tools/lessons.py +++ b/src/scribe/mcp/tools/lessons.py @@ -29,7 +29,7 @@ _to_dict = lessons_svc.lesson_to_dict async def list_lessons( q: str = "", tag: str = "", limit: int = 50, offset: int = 0, - project_id: int = 0, + project_id: int = 0, unjudged: bool = False, ) -> dict: """List lessons — the kind enumerated, rather than only what a query resembles. @@ -51,16 +51,32 @@ async def list_lessons( project_id: Narrow to where a lesson was WRITTEN. 0 = every project. A lesson is retrievable from anywhere regardless (step 3); this filters the listing, not the reach. + unjudged: List only the lessons nobody has answered "which rule is + this an instance of?" for — no rule named, and no "no rule fits" + recorded. Work down this list with `update_lesson(rule_ids=…)` or + `update_lesson(no_rule="why")`. A listing rather than a search, so + it takes `tag` and `project_id` but not `q`. Returns {"lessons": [{id, title, when_to_apply, tags, preview}], "total"}. """ uid = current_user_id() - items, total = await knowledge_svc.query_knowledge( - user_id=uid, note_type=lessons_svc.LESSON_NOTE_TYPE, - tags=[tag] if tag else [], sort="modified", q=q or None, - limit=max(1, min(limit, 100)), offset=max(0, offset), - project_id=project_id or None, - ) + if unjudged: + if q: + raise ValueError( + "unjudged lists every unjudged lesson rather than searching " + "them — call it with tag/project_id and without q." + ) + items, total = await lesson_rules_svc.list_unjudged( + uid, tag=tag, project_id=project_id or None, + limit=max(1, min(limit, 100)), offset=max(0, offset), + ) + else: + items, total = await knowledge_svc.query_knowledge( + user_id=uid, note_type=lessons_svc.LESSON_NOTE_TYPE, + tags=[tag] if tag else [], sort="modified", q=q or None, + limit=max(1, min(limit, 100)), offset=max(0, offset), + project_id=project_id or None, + ) labelled = await access_svc.label_shared_items(uid, items) # One aggregate for the page, like the snippet listing — surfaced-vs-opened # per lesson (#4196). An agent listing lessons can see which of its own @@ -91,11 +107,29 @@ async def create_lesson( project_id: int = 0, system_ids: list[int] | None = None, rule_ids: list[int] | None = None, + no_rule: str = "", force: bool = False, ) -> dict: """Record something you LEARNED, so a later session meets it at the moment it applies — on this project or any other. + A LESSON POINTS AT THE RULE IT IS AN INSTANCE OF. While you write it, you + know the situation better than anyone will again, so that is when to say + which binding choice it falls under. Answer one of two ways, in this call + or right after it with `update_lesson`: + + - `rule_ids=[…]` — the rule(s) or preference(s) it is an instance of. The + rule then becomes reachable through the situation the lesson describes, + which is closer to why it was written than its own wording often is. + - `no_rule="why"` — no rule governs this situation, in a line. That is a + real answer: a lesson that stands alone is what a rule nobody has + written yet is made from, and the reason lets it be re-judged later. + + A lesson created with neither comes back with `rule_candidates` — the + rules it most resembles, each with its trigger — and `rule_judgment: + "unjudged"`. Read them against the lesson and answer; `list_lessons( + unjudged=true)` gathers any left open. + A LESSON OR A RULE? The difference is FORCE, not importance. A rule is something that must be followed; a lesson is something worth knowing. If ignoring it would be a mistake, it is a rule (create_rule) and needs the @@ -151,11 +185,14 @@ async def create_lesson( the binding choice its situation falls under. A lesson never becomes a rule; it points at the one that governs it, and the rule is then reachable through the situation the lesson describes - (milestone 440). Naming a rule here confirms the link. Leave it - empty when no rule governs this situation. + (milestone 440). Naming a rule here confirms the link. + no_rule: The reason no rule governs this situation, in a line — the + other answer to "which rule?". Give one or the other, not both. force: Create even if a near-duplicate exists. - Returns the created lesson. On a near-duplicate, returns the existing id + Returns the created lesson with its `rules` and `rule_judgment`, plus + `rule_candidates` when it is unjudged (absent when the rule search could + not run). On a near-duplicate, returns the existing id instead of creating — two lessons about one failure class want to be one lesson, so update that one rather than adding a second. """ @@ -172,6 +209,7 @@ async def create_lesson( # Validated before anything is written, so a lesson naming a rule the # caller cannot read fails whole rather than saving half-linked. linked = await lesson_rules_svc.require_rules(uid, rule_ids) + lesson_rules_svc.require_one_answer(linked, no_rule) title, body = lessons_svc.lesson_document( what, when_to_apply, insight, sources, ) @@ -192,12 +230,34 @@ async def create_lesson( await systems_svc.set_record_systems(uid, note.id, system_ids) if linked: await lesson_rules_svc.set_lesson_rules(uid, note.id, linked) + elif no_rule.strip(): + await lesson_rules_svc.set_no_rule(uid, note.id, no_rule) data = _to_dict(note) await systems_tools.attach_systems(uid, uid, data, note.id, project_id or None) await lesson_rules_svc.attach_lesson_rules(uid, [data]) + if not linked and not no_rule.strip(): + await _offer_candidates(uid, data, what, when_to_apply, project_id) return data +async def _offer_candidates(uid: int, data: dict, what: str, trigger: str, project_id: int) -> None: + """Put the rules an unjudged lesson resembles in front of its writer. + + `rule_judgment` is set here too, because the attach above is fail-open and + may have left it off; a lesson just created with no answer IS unjudged. + """ + data["rule_judgment"] = lesson_rules_svc.UNJUDGED + candidates = await lesson_rules_svc.rule_candidates(uid, what, trigger, project_id or None) + if candidates is not None: + data["rule_candidates"] = candidates + data["rule_hint"] = ( + "Which rule is this lesson an instance of? If one of rule_candidates " + "governs its situation, update_lesson(lesson_id, rule_ids=[…]) links " + "it; if none does, update_lesson(lesson_id, no_rule=\"why\") records " + "that it stands alone." + ) + + async def get_lesson(lesson_id: int, project_id: int = 0) -> dict: """Fetch one lesson by id, with its trigger and sources read back out. @@ -245,6 +305,7 @@ async def update_lesson( tags: list[str] | None = None, system_ids: list[int] | None = None, rule_ids: list[int] | None = None, + no_rule: str = "", ) -> dict: """Update a lesson. Empty fields are left unchanged. @@ -278,12 +339,18 @@ async def update_lesson( leaves unchanged; pass the FULL list. A rule that was linked and is left out is recorded as REJECTED — "not an instance of this one" — so the pair is not proposed again; `[]` rejects them all. + Naming a rule replaces any "no rule fits" answer. + no_rule: Record that no rule governs this lesson's situation, with the + reason in a line. Any rule still linked is rejected with that + reason. Empty leaves the answer unchanged; give this or a + non-empty `rule_ids`, not both. """ uid = current_user_id() linked = ( await lesson_rules_svc.require_rules(uid, rule_ids) if rule_ids is not None else None ) + lesson_rules_svc.require_one_answer(linked, no_rule) note = await lessons_svc.update_lesson( uid, lesson_id, what=what or None, @@ -302,6 +369,8 @@ async def update_lesson( note = await lessons_svc.get_lesson(uid, lesson_id) or note if linked is not None: await lesson_rules_svc.set_lesson_rules(uid, lesson_id, linked) + if no_rule.strip(): + await lesson_rules_svc.set_no_rule(uid, lesson_id, no_rule) out = _to_dict(note) await lesson_rules_svc.attach_lesson_rules(uid, [out]) return out diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 7d44473..d6b4562 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -393,6 +393,14 @@ async def create_rule( observations belong, and the reason the kind question is worth asking out loud rather than settled silently before the proposal. + A RULE WRITTEN FOR A SITUATION LESSONS ALREADY RECORD is linked from + them once it exists: `update_lesson(lesson_id, rule_ids=[…, ])` + for each lesson it governs. Those lessons are the concrete situations + that proved it, and linked, they are how the rule is reached in moments + closer to why it was written than its own wording. A lesson that + answered "no rule fits" before this rule existed is the first to + re-judge; `get_lesson` shows the answer and its reason. + Where the interface offers structured choices, ask it that way — a question with named options is answered in a click, while the same question inside a paragraph is answered by scrolling past. Where it does diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index 4af89b9..178fb84 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -78,7 +78,7 @@ from scribe.models.rulebook import ( # noqa: E402, F401 Rulebook, RulebookTopic, Rule, RuleRelation, rule_systems, ) # After notes and rules: it foreign-keys both (milestone 440). -from scribe.models.lesson_rule_link import LessonRuleLink # noqa: E402, F401 +from scribe.models.lesson_rule_link import LessonNoRule, LessonRuleLink # noqa: E402, F401 from scribe.models.repo_binding import RepoBinding # noqa: E402, F401 from scribe.models.forge_connection import ForgeConnection # noqa: E402, F401 from scribe.models.code_shape import CodeShape, CodeShapeConsumer, CodeShapeEvent, CodeShapeUse # noqa: E402, F401 diff --git a/src/scribe/models/lesson_rule_link.py b/src/scribe/models/lesson_rule_link.py index f9177d7..ea08374 100644 --- a/src/scribe/models/lesson_rule_link.py +++ b/src/scribe/models/lesson_rule_link.py @@ -1,6 +1,6 @@ from datetime import datetime -from sqlalchemy import BigInteger, DateTime, ForeignKey, Integer, Text, UniqueConstraint +from sqlalchemy import BigInteger, DateTime, ForeignKey, Integer, Text, UniqueConstraint, func from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import Mapped, mapped_column @@ -75,3 +75,38 @@ class LessonRuleLink(Base, CreatedAtMixin): "judged_at": iso(self.judged_at), "created_at": iso(self.created_at), } + + +class LessonNoRule(Base): + """A judgment that no rule governs a lesson's situation (#4631). + + The third answer to "which rule is this an instance of?", beside naming + one and leaving it open. It is what lets an UNJUDGED lesson be told apart + from one that was looked at and found to stand alone — and a lesson with + no rule is the raw material #4634 reads for convergence, so the + difference is the whole signal. + + A row of its own rather than a key in the lesson's `data`: that column is + a mirror re-composed from the body on every edit (services/lessons.py), + so anything kept there that the body does not render is erased by the + next rewording — and the lesson would silently read as unjudged again. + + One row per lesson. Naming a rule later deletes it: "no rule fits" and + "this rule fits" cannot both be the current answer. + """ + + __tablename__ = "lesson_no_rule" + + lesson_id: Mapped[int] = mapped_column( + Integer, ForeignKey("notes.id", ondelete="CASCADE"), primary_key=True, + ) + # Why none fits — required, because "no rule" with no reason cannot be + # re-judged when a rule is later written for the situation. + why: Mapped[str] = mapped_column(Text) + judged_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), server_default=func.now(), + ) + + def to_dict(self) -> dict: + return {"why": self.why, "judged_at": iso(self.judged_at)} + diff --git a/src/scribe/routes/lessons.py b/src/scribe/routes/lessons.py index 0335b07..a68ced5 100644 --- a/src/scribe/routes/lessons.py +++ b/src/scribe/routes/lessons.py @@ -63,16 +63,25 @@ async def list_lessons_route(): project_id = None limit, offset = parse_pagination(default_limit=24, max_limit=100) - items, total = await knowledge_svc.query_knowledge( - user_id=uid, - note_type=lessons_svc.LESSON_NOTE_TYPE, - tags=[tag] if tag else [], - sort="modified", - q=q, - limit=limit, - offset=offset, - project_id=project_id, - ) + # The lessons nobody has answered "which rule?" for (#4631). A listing, + # so it does not combine with a search — the MCP door says the same. + if request.args.get("unjudged") in ("1", "true"): + if q: + return jsonify({"error": "unjudged is a listing; drop q"}), 400 + items, total = await lesson_rules_svc.list_unjudged( + uid, tag=tag, project_id=project_id, limit=limit, offset=offset, + ) + else: + items, total = await knowledge_svc.query_knowledge( + user_id=uid, + note_type=lessons_svc.LESSON_NOTE_TYPE, + tags=[tag] if tag else [], + sort="modified", + q=q, + limit=limit, + offset=offset, + project_id=project_id, + ) items = await label_shared_items(uid, items) # One aggregate for the whole page — a per-row lookup would be N+1 by # construction. Every row gets the key, zero-filled, so the UI renders @@ -139,8 +148,10 @@ async def create_lesson_route(): learned_from = data.get("learned_from") or [] # The rules this lesson is an instance of (milestone 440). Validated before # anything is written, as the MCP door does, so a bad id saves nothing. + no_rule = (data.get("no_rule") or "").strip() try: linked = await lesson_rules_svc.require_rules(uid, data.get("rule_ids") or []) + lesson_rules_svc.require_one_answer(linked, no_rule) except ValueError as exc: return jsonify({"error": str(exc)}), 400 @@ -174,11 +185,21 @@ async def create_lesson_route(): await systems_svc.set_record_systems(uid, note.id, data["system_ids"]) if linked: await lesson_rules_svc.set_lesson_rules(uid, note.id, linked) + elif no_rule: + await lesson_rules_svc.set_no_rule(uid, note.id, no_rule) out = lessons_svc.lesson_to_dict(note) out["systems"] = [ s.to_dict() for s in await systems_svc.list_record_systems(uid, note.id) ] await lesson_rules_svc.attach_lesson_rules(uid, [out]) + # The same offer the MCP door makes, so the form can show the rules to + # judge against while the writer still has the situation in mind. + if not linked and not no_rule: + candidates = await lesson_rules_svc.rule_candidates( + uid, what, when_to_apply, project_id, + ) + if candidates is not None: + out["rule_candidates"] = candidates return jsonify(out), 201 @@ -245,11 +266,13 @@ async def update_lesson_route(lesson_id: int): }), 400 linked = None - if data.get("rule_ids") is not None: - try: + no_rule = (data.get("no_rule") or "").strip() + try: + if data.get("rule_ids") is not None: linked = await lesson_rules_svc.require_rules(uid, data["rule_ids"]) - except ValueError as exc: - return jsonify({"error": str(exc)}), 400 + lesson_rules_svc.require_one_answer(linked, no_rule) + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 updated = await lessons_svc.update_lesson(owner_uid, lesson_id, **kwargs) if updated is None: @@ -258,6 +281,8 @@ async def update_lesson_route(lesson_id: int): # The CALLER, for the reason system_ids below uses it: the rules named # must be ones the person making the edit can read. await lesson_rules_svc.set_lesson_rules(uid, lesson_id, linked) + if no_rule: + await lesson_rules_svc.set_no_rule(uid, lesson_id, no_rule) if data.get("system_ids") is not None: # The CALLER, not owner_uid (#4249). `set_record_systems` runs its own # `can_write_note` and links only Systems the acting user can read; diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 335327e..c472d94 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -16,7 +16,7 @@ from scribe.models.rule_usage import RuleUsageEvent from scribe.models.retrieval_tuning import RetrievalTuningEvent from scribe.models.canonical_system import CanonicalSystem from scribe.models.rulebook import RuleRelation, rule_systems as rule_systems_t -from scribe.models.lesson_rule_link import LessonRuleLink +from scribe.models.lesson_rule_link import LessonNoRule, LessonRuleLink from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse from scribe.models.project import Project from scribe.models.repo_binding import RepoBinding @@ -85,8 +85,11 @@ logger = logging.getLogger(__name__) # is an instance of, and the judgments that confirmed or rejected each pair. A # confirmed link is a judgment nothing else records, and a rejected one is # what stops the pair being proposed again — losing either undoes work. +# v19 (2026-10) added lesson_no_rule (#4631): the "no rule fits" answer and its +# reason. Without it a restored lesson that was judged to stand alone reads as +# never judged, and lands back on the unjudged list. # Bump when the serialized schema changes. -BACKUP_VERSION = 18 +BACKUP_VERSION = 19 # Every table this backup carries, by its REAL name. Paired with _NOT_INCLUDED # below, these two lists must together account for the entire schema — which is @@ -126,6 +129,8 @@ _BACKED_UP = [ "retrieval_tuning_events", # v18 (2026-10): lesson → rule links and their judgments (milestone 440). "lesson_rule_links", + # v19 (2026-10): "no rule fits" answers (#4631). + "lesson_no_rule", ] # Tables intentionally NOT in the backup, surfaced in the payload so the gap is @@ -221,6 +226,8 @@ _COLUMN_EXCLUSIONS: dict[str, set[str]] = { "rule_relations": {"id", "created_at"}, # The pair is the row; everything else is the judgment and its evidence. "lesson_rule_links": {"id"}, + # Keyed by the lesson itself; nothing to exclude, every column travels. + "lesson_no_rule": set(), "note_usage_events": {"id"}, # Same as the note twin: the surrogate key is re-issued on insert. "rule_usage_events": {"id"}, @@ -309,6 +316,7 @@ _IMPORT_COLUMN_EXCLUSIONS: dict[str, set[str]] = { "note_supersessions": {"id", "created_at"}, "rule_relations": {"id", "created_at"}, "lesson_rule_links": {"id"}, + "lesson_no_rule": set(), "note_usage_events": {"id"}, "rule_usage_events": {"id"}, "retrieval_tuning_events": {"id"}, @@ -717,6 +725,17 @@ def _lesson_rule_link_rows(rows) -> list[dict]: ] +def _lesson_no_rule_rows(rows) -> list[dict]: + """The "no rule fits" answers, by SOURCE lesson id (#4631).""" + return [ + { + "lesson_id": r.lesson_id, "why": r.why, + "judged_at": r.judged_at.isoformat() if r.judged_at else None, + } + for r in rows + ] + + def _rule_rows(rows) -> list[dict]: return [ { @@ -765,6 +784,7 @@ async def export_full_backup() -> dict: )).all() rule_relations = (await session.execute(select(RuleRelation))).scalars().all() lesson_rule_links = (await session.execute(select(LessonRuleLink))).scalars().all() + lesson_no_rule = (await session.execute(select(LessonNoRule))).scalars().all() record_systems = (await session.execute(select(RecordSystem))).scalars().all() supersessions = ( await session.execute(select(NoteSupersession)) @@ -824,6 +844,7 @@ async def export_full_backup() -> dict: "rule_systems": _rule_system_rows(rule_system_rows), "rule_relations": _rule_relation_rows(rule_relations), "lesson_rule_links": _lesson_rule_link_rows(lesson_rule_links), + "lesson_no_rule": _lesson_no_rule_rows(lesson_no_rule), "systems": _system_rows( systems, {c.id: c.slug for c in canonical_systems} ), @@ -992,6 +1013,9 @@ async def export_user_backup(user_id: int) -> dict: LessonRuleLink.rule_id.in_(_rule_ids), ) )).scalars().all() if (_rule_ids and note_ids) else [] + lesson_no_rule = (await session.execute( + select(LessonNoRule).where(LessonNoRule.lesson_id.in_(note_ids)) + )).scalars().all() if note_ids else [] return { "version": BACKUP_VERSION, @@ -1020,6 +1044,7 @@ async def export_user_backup(user_id: int) -> dict: "rule_systems": _rule_system_rows(rule_system_rows), "rule_relations": _rule_relation_rows(rule_relations), "lesson_rule_links": _lesson_rule_link_rows(lesson_rule_links), + "lesson_no_rule": _lesson_no_rule_rows(lesson_no_rule), "systems": _system_rows( systems, {c.id: c.slug for c in canonical_systems} ), @@ -1365,6 +1390,17 @@ def _build_rule_relation(row: dict, maps: _Maps) -> RuleRelation | None: ) +def _build_lesson_no_rule(row: dict, maps: _Maps) -> LessonNoRule | None: + """Skipped when its lesson did not restore — the answer is about that + lesson and no other.""" + lesson = maps.notes.get(row.get("lesson_id", 0)) + if lesson is None or not (row.get("why") or "").strip(): + return None + return LessonNoRule( + lesson_id=lesson, why=row["why"], judged_at=_dt(row.get("judged_at")), + ) + + def _build_lesson_rule_link(row: dict, maps: _Maps) -> LessonRuleLink | None: """Both ends must map: a link whose lesson or rule did not restore points at whatever took that number in the destination.""" @@ -1782,6 +1818,7 @@ async def _restore_v2(data: dict) -> dict: "code_shape_uses": 0, "canonical_systems": 0, "rule_systems": 0, "rule_relations": 0, "rule_versions": 0, "retrieval_tuning_events": 0, "lesson_rule_links": 0, + "lesson_no_rule": 0, } async with async_session() as session: @@ -1985,6 +2022,13 @@ async def _restore_v2(data: dict) -> dict: continue session.add(link) stats["lesson_rule_links"] += 1 + # "No rule fits" answers (#4631); archives before v19 carry none. + for nr in data.get("lesson_no_rule", []): + answer = _build_lesson_no_rule(nr, maps) + if answer is None: + continue + session.add(answer) + stats["lesson_no_rule"] += 1 # A rule's edit history (milestone 323). Must come after the rules # themselves — the rule map is only populated above — and both ids are diff --git a/src/scribe/services/lesson_rules.py b/src/scribe/services/lesson_rules.py index 63c27ad..f8e6424 100644 --- a/src/scribe/services/lesson_rules.py +++ b/src/scribe/services/lesson_rules.py @@ -10,6 +10,13 @@ judgment is made. Every write here is a JUDGMENT, so every write lands as confirmed or rejected; `suggested` rows come from the co-surfacing recorder (#4637), never from a caller naming a rule. +THE THIRD ANSWER (#4631). "Which rule is this an instance of?" has three +answers, and only two of them are links: a rule named (confirmed), or NO RULE +FITS — a `lesson_no_rule` row carrying the reason. A lesson with neither is +UNJUDGED, and `list_unjudged` lists those. The three are kept apart because a +lesson that stands alone is the raw material for a rule nobody has written yet +(#4634), and that is unreadable if "nobody looked" means the same thing. + ACL (rule 78). Linking changes what a lesson says about itself, so it needs WRITE on the lesson (`access.can_write_note`, share-aware). It names a rule, so it needs the rule to be one the caller may read — rules are owner-scoped, and @@ -22,10 +29,12 @@ from __future__ import annotations import logging from datetime import datetime, timezone -from sqlalchemy import select +from sqlalchemy import and_, delete, exists, func, not_, select from scribe.models import async_session -from scribe.models.lesson_rule_link import CONFIRMED, REJECTED, SUGGESTED, LessonRuleLink +from scribe.models.lesson_rule_link import ( + CONFIRMED, REJECTED, SUGGESTED, LessonNoRule, LessonRuleLink, +) from scribe.models.note import Note from scribe.models.rulebook import Rule @@ -39,6 +48,18 @@ VERDICTS = {"confirm": CONFIRMED, "reject": REJECTED} # row can tell an explicit "not this rule" from a link dropped by a rewrite. _REMOVED_NOTE = "removed from the lesson's rules by an update" +# Recorded on a confirmed link that a "no rule fits" answer overturned, ahead +# of that answer's reason. +_NO_RULE_NOTE = "no rule fits: " + +# What `rule_judgment` reads on a lesson payload — the three answers. +LINKED, NO_RULE, UNJUDGED = "linked", "no_rule", "unjudged" + +# How many rules a new lesson is offered to judge against. A handful, not the +# fifty `what_might_apply` returns: this is read in the create response, at +# the moment of writing, and a long tail there buries the one that fits. +CANDIDATE_LIMIT = 5 + def _ids(values) -> list[int]: """Positive ints, de-duplicated, in the order given.""" @@ -83,6 +104,16 @@ async def require_rules(user_id: int, rule_ids) -> list[int]: return wanted +def require_one_answer(linked, no_rule: str) -> None: + """Refuse "these rules fit" and "no rule fits" together, before any write + — they contradict, and either order of applying them loses one.""" + if linked and (no_rule or "").strip(): + raise ValueError( + "rule_ids and no_rule are the two answers to \"which rule is this " + "an instance of?\" — give the one that holds. Nothing was written." + ) + + async def _upsert(session, lesson_id: int, rule_id: int, state: str, note: str) -> None: now = datetime.now(timezone.utc) row = (await session.execute( @@ -104,6 +135,11 @@ async def _upsert(session, lesson_id: int, rule_id: int, state: str, note: str) row.judged_at = now +async def _clear_no_rule(session, lesson_id: int) -> None: + """A confirmed rule overturns "no rule fits" — both cannot be the answer.""" + await session.execute(delete(LessonNoRule).where(LessonNoRule.lesson_id == lesson_id)) + + async def set_lesson_rules( user_id: int, lesson_id: int, rule_ids, *, note: str = "", ) -> None: @@ -134,9 +170,48 @@ async def set_lesson_rules( row.judged_at = datetime.now(timezone.utc) for rid in wanted: await _upsert(session, lesson_id, rid, CONFIRMED, note) + if wanted: + await _clear_no_rule(session, lesson_id) await session.commit() +async def set_no_rule(user_id: int, lesson_id: int, why: str) -> dict: + """Record that no rule governs this lesson's situation, and why. + + A confirmed link the lesson still carries becomes `rejected`, with the + reason: saying none fits is saying the linked one does not either. A + second answer replaces the first, so the reason on file is the latest. + """ + why = (why or "").strip() + if not why: + raise ValueError( + "no_rule takes the reason no rule fits, in a line — it is what " + "lets the answer be re-judged when a rule is later written for " + "this situation." + ) + await _require_lesson_writable(user_id, lesson_id) + now = datetime.now(timezone.utc) + async with async_session() as session: + confirmed = (await session.execute( + select(LessonRuleLink).where( + LessonRuleLink.lesson_id == lesson_id, + LessonRuleLink.state == CONFIRMED, + ) + )).scalars().all() + for row in confirmed: + row.state = REJECTED + row.note = _NO_RULE_NOTE + why + row.judged_at = now + answer = await session.get(LessonNoRule, lesson_id) + if answer is None: + session.add(LessonNoRule(lesson_id=lesson_id, why=why, judged_at=now)) + else: + answer.why = why + answer.judged_at = now + await session.commit() + return {"why": why, "judged_at": now.isoformat()} + + async def judge_link( user_id: int, lesson_id: int, rule_id: int, verdict: str, note: str = "", ) -> dict: @@ -148,6 +223,8 @@ async def judge_link( [rid] = await require_rules(user_id, [rule_id]) async with async_session() as session: await _upsert(session, lesson_id, rid, state, note) + if state == CONFIRMED: + await _clear_no_rule(session, lesson_id) await session.commit() row = (await session.execute( select(LessonRuleLink).where( @@ -210,22 +287,147 @@ async def lessons_for_rule(user_id: int, rule_id: int) -> list[dict]: ] +async def judgments_for_lessons(lesson_ids) -> dict[int, dict]: + """{lesson_id: {"rule_judgment": linked|no_rule|unjudged, "no_rule"?}}. + + Read from the link and answer tables directly, NOT from the reader's view + of the rules: a lesson shared with someone who cannot see its owner's rule + is still a judged lesson, and calling it unjudged would invite them to + judge it again. + """ + ids = [int(i) for i in lesson_ids or []] + if not ids: + return {} + async with async_session() as session: + linked = set((await session.execute( + select(LessonRuleLink.lesson_id).where( + LessonRuleLink.lesson_id.in_(ids), + LessonRuleLink.state == CONFIRMED, + ) + )).scalars().all()) + answers = { + a.lesson_id: a for a in (await session.execute( + select(LessonNoRule).where(LessonNoRule.lesson_id.in_(ids)) + )).scalars().all() + } + out: dict[int, dict] = {} + for i in ids: + if i in linked: + out[i] = {"rule_judgment": LINKED} + elif i in answers: + out[i] = {"rule_judgment": NO_RULE, "no_rule": answers[i].to_dict()} + else: + out[i] = {"rule_judgment": UNJUDGED} + return out + + async def attach_lesson_rules(user_id: int, rows: list[dict], *, key: str = "id") -> None: - """Add `rules` to each lesson payload row, in place — one query per page. + """Add `rules` and `rule_judgment` (plus `no_rule` when that is the answer) + to each lesson payload row, in place — a fixed number of queries per page. Fail-open (snippet #4286): this decorates a lesson the caller already has, - so a failed lookup leaves the key off and logs, rather than refusing the + so a failed lookup leaves the keys off and logs, rather than refusing the lesson. An absent key reads as "not attached", never as "no rule". """ ids = [int(r[key]) for r in rows if isinstance(r.get(key), int)] try: found = await rules_for_lessons(user_id, ids) + judged = await judgments_for_lessons(ids) except Exception: logger.warning("lesson→rule links could not be read", exc_info=True) return for r in rows: if isinstance(r.get(key), int): r["rules"] = found.get(r[key], []) + r.update(judged.get(r[key], {})) + + +def unjudged_clause(): + """SQL: a lesson (a `notes` row) with no confirmed rule and no "no rule + fits" answer. A rejected link alone leaves it unjudged — "not that rule" + does not say whether another one fits.""" + from scribe.models.note import Note + + return and_( + not_(exists().where(and_( + LessonRuleLink.lesson_id == Note.id, + LessonRuleLink.state == CONFIRMED, + ))), + not_(exists().where(LessonNoRule.lesson_id == Note.id)), + ) + + +async def list_unjudged( + user_id: int, *, tag: str = "", project_id: int | None = None, + limit: int = 50, offset: int = 0, +) -> tuple[list[dict], int]: + """The lessons nobody has answered "which rule?" for, newest edit first. + + The browse scope (`browsable_notes_clause`), the list it sits beside uses: + a lesson shared directly with the caller is search-only and stays out of + an ambient list. Rows come back in the knowledge list's item shape. + """ + from scribe.models.note import Note + from scribe.services.access import browsable_notes_clause + from scribe.services.knowledge import _note_to_item + from scribe.services.lessons import LESSON_NOTE_TYPE + + base = ( + select(Note) + .where(browsable_notes_clause(user_id)) + .where(Note.note_type == LESSON_NOTE_TYPE) + .where(Note.deleted_at.is_(None)) + .where(unjudged_clause()) + ) + if tag: + base = base.where(Note.tags.contains([tag])) + if project_id is not None: + base = base.where(Note.project_id == project_id) + async with async_session() as session: + total = (await session.execute( + select(func.count()).select_from(base.subquery()) + )).scalar_one() + rows = (await session.execute( + base.order_by(Note.updated_at.desc()).limit(limit).offset(offset) + )).scalars().all() + return [_note_to_item(n) for n in rows], int(total) + + +async def rule_candidates( + user_id: int, what: str, when_to_apply: str, project_id: int | None = None, +) -> list[dict] | None: + """The rules a lesson most resembles, for the writer to judge against. + + Searched with the lesson's claim and its trigger — the same two halves a + rule's own document leads with (`{title} — {trigger}`), so like is + compared with like; the insight is the story and would dilute the vector. + Scoped as any project read is: global rules plus the lesson's project's + own. The bar is the explicit rule search's, since this is an explicit + question rather than an injection. + + None when the search could not run (no embedder, a failed query), so a + caller can say "candidates unavailable" rather than "nothing resembles". + """ + from scribe.services.embeddings import ( + DEFAULT_SIMILARITY_THRESHOLD, semantic_search_rules, + ) + + query = " — ".join(p for p in ((what or "").strip(), (when_to_apply or "").strip()) if p) + report: dict = {} + found = await semantic_search_rules( + user_id, query, limit=CANDIDATE_LIMIT, + threshold=DEFAULT_SIMILARITY_THRESHOLD, report=report, + project_id=project_id or None, + ) + if not report.get("searched"): + return None + return [ + { + "id": rule.id, "title": rule.title, "kind": rule.kind, + "when_to_apply": rule.when_to_apply or "", "score": round(score, 3), + } + for score, rule in found + ] async def attach_rule_lessons(user_id: int, data: dict, rule_id: int) -> None: diff --git a/tests/conftest.py b/tests/conftest.py index b64e527..489d199 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -152,6 +152,10 @@ def _no_lesson_rule_links(request): Both decorations are fail-open, so the cost would be a slow failed connect per test rather than a failure, which is worse: it would never be noticed. + The candidate search a new lesson runs (#4631) is stubbed to "nothing + resembles it" for the same reason: it embeds, and the model load would + otherwise ride along with every unit test that creates a lesson. + Skipped for integration tests, which exercise the real links against Postgres (tests/test_integration_lesson_rule_links.py). """ @@ -159,7 +163,8 @@ def _no_lesson_rule_links(request): yield return with patch("scribe.services.lesson_rules.attach_lesson_rules", AsyncMock()), \ - patch("scribe.services.lesson_rules.attach_rule_lessons", AsyncMock()): + patch("scribe.services.lesson_rules.attach_rule_lessons", AsyncMock()), \ + patch("scribe.services.lesson_rules.rule_candidates", AsyncMock(return_value=[])): yield diff --git a/tests/test_guidance_ownership.py b/tests/test_guidance_ownership.py index a03fc21..ccffb91 100644 --- a/tests/test_guidance_ownership.py +++ b/tests/test_guidance_ownership.py @@ -145,6 +145,15 @@ TOPICS: tuple[Topic, ...] = ( # one fires mid-work, the moment a lesson arrives in a situation it # actually names. `lessons inform` already points at the kind. shared_with=("docstrings",)), + Topic("a lesson names the rule it is an instance of", U, + ("rule_ids", "no_rule", "unjudged=true"), + "a rule about the same subsystem that governs a different moment " + "is not its rule", + # Mid-work, like the topic above: it fires when a lesson is written, + # so no index marker. create_lesson's docstring states the two + # answers at the moment they are given, and may — tool contracts + # are not copy-scanned. + shared_with=("docstrings",)), Topic("preferences shape how work is done, never what is recorded", U, ("never what gets recorded",), "a preference never makes a task into a note"), diff --git a/tests/test_integration_lesson_rule_links.py b/tests/test_integration_lesson_rule_links.py index 5156bcb..539a8c5 100644 --- a/tests/test_integration_lesson_rule_links.py +++ b/tests/test_integration_lesson_rule_links.py @@ -160,3 +160,84 @@ async def test_one_row_per_pair(world): ]) with pytest.raises(IntegrityError): await s.commit() + + +# ── #4631: "no rule fits" and the unjudged list ───────────────────────────── + + +async def _judgment(lesson_id: int) -> dict: + return (await links_svc.judgments_for_lessons([lesson_id]))[lesson_id] + + +async def _unjudged_ids(user_id: int) -> set[int]: + items, _total = await links_svc.list_unjudged(user_id, limit=100) + return {i["id"] for i in items} + + +async def test_a_new_lesson_is_unjudged_until_it_is_answered_either_way(world): + owner, lesson = world["owner"], world["lesson"] + assert (await _judgment(lesson))["rule_judgment"] == "unjudged" + assert lesson in await _unjudged_ids(owner) + + await links_svc.set_no_rule(owner, lesson, "specific to this CI host") + judged = await _judgment(lesson) + assert judged["rule_judgment"] == "no_rule" + assert judged["no_rule"]["why"] == "specific to this CI host" + assert lesson not in await _unjudged_ids(owner) + + # Naming a rule overturns the answer — both cannot stand. + await links_svc.set_lesson_rules(owner, lesson, [world["r1"]]) + judged = await _judgment(lesson) + assert judged == {"rule_judgment": "linked"} + assert lesson not in await _unjudged_ids(owner) + + +async def test_no_rule_rejects_a_confirmed_link_with_its_reason(world): + owner, lesson = world["owner"], world["lesson"] + await links_svc.set_lesson_rules(owner, lesson, [world["r1"]]) + await links_svc.set_no_rule(owner, lesson, "the log was not the issue") + + assert await _states(lesson) == {world["r1"]: "rejected"} + rules = (await links_svc.rules_for_lessons(owner, [lesson]))[lesson] + assert rules[0]["note"].endswith("the log was not the issue") + assert (await _judgment(lesson))["rule_judgment"] == "no_rule" + + +async def test_a_confirmation_by_judgment_also_clears_no_rule(world): + owner, lesson = world["owner"], world["lesson"] + await links_svc.set_no_rule(owner, lesson, "nothing yet") + await links_svc.judge_link(owner, lesson, world["r2"], "confirm") + assert (await _judgment(lesson))["rule_judgment"] == "linked" + + +async def test_a_rejection_alone_leaves_the_lesson_unjudged(world): + """"Not that rule" says nothing about whether another one fits.""" + owner, lesson = world["owner"], world["lesson"] + await links_svc.judge_link(owner, lesson, world["r1"], "reject", "different failure") + assert (await _judgment(lesson))["rule_judgment"] == "unjudged" + assert lesson in await _unjudged_ids(owner) + + +async def test_no_rule_needs_a_reason_and_a_writer(world): + with pytest.raises(ValueError): + await links_svc.set_no_rule(world["owner"], world["lesson"], " ") + with pytest.raises((ValueError, PermissionError)): + await links_svc.set_no_rule(world["stranger"], world["lesson"], "mine now") + assert (await _judgment(world["lesson"]))["rule_judgment"] == "unjudged" + + +async def test_the_unjudged_list_is_scoped_to_the_reader(world): + assert world["lesson"] not in await _unjudged_ids(world["stranger"]) + + +async def test_deleting_the_lesson_takes_its_answer_with_it(world): + from scribe.models.lesson_rule_link import LessonNoRule + + await links_svc.set_no_rule(world["owner"], world["lesson"], "alone") + async with async_session() as s: + await s.execute(delete(Note).where(Note.id == world["lesson"])) + await s.commit() + left = (await s.execute( + select(LessonNoRule).where(LessonNoRule.lesson_id == world["lesson"]) + )).scalars().all() + assert left == [] diff --git a/tests/test_lesson_rule_link_doors.py b/tests/test_lesson_rule_link_doors.py index 8715fad..96ec27a 100644 --- a/tests/test_lesson_rule_link_doors.py +++ b/tests/test_lesson_rule_link_doors.py @@ -143,3 +143,123 @@ def test_backup_keeps_an_unjudged_link_unjudged(): ) assert (built.lesson_id, built.rule_id, built.state) == (110, 120, "suggested") assert built.judged_at is None + + +# ── #4631: judged when written ─────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_an_unanswered_lesson_comes_back_with_rules_to_judge_against(): + """Neither answer given: the create offers the rules the lesson resembles, + searched with its claim and trigger and scoped to its project.""" + _user_id_ctx.set(7) + offered = [{"id": 5, "title": "Read the log", "kind": "rule", + "when_to_apply": "a run overran", "score": 0.61}] + search = AsyncMock(return_value=offered) + p1, p2, p3 = _create_patches(AsyncMock(return_value=_stub_note())) + with p1, p2, p3, patch.object(links_svc, "rule_candidates", search): + out = await lesson_tools.create_lesson(what="x", when_to_apply=TRIGGER, project_id=2) + assert search.await_args.args == (7, "x", TRIGGER, 2) + assert out["rule_candidates"] == offered + assert out["rule_judgment"] == links_svc.UNJUDGED + assert "no_rule" in out["rule_hint"] and "rule_ids" in out["rule_hint"] + + +@pytest.mark.asyncio +async def test_a_search_that_could_not_run_offers_no_list_rather_than_an_empty_one(): + """None means unavailable; an empty list would claim nothing resembles it.""" + _user_id_ctx.set(7) + p1, p2, p3 = _create_patches(AsyncMock(return_value=_stub_note())) + with p1, p2, p3, patch.object(links_svc, "rule_candidates", AsyncMock(return_value=None)): + out = await lesson_tools.create_lesson(what="x", when_to_apply=TRIGGER) + assert "rule_candidates" not in out + assert out["rule_judgment"] == links_svc.UNJUDGED + + +@pytest.mark.asyncio +async def test_no_rule_on_create_records_the_answer_and_offers_nothing(): + _user_id_ctx.set(7) + answered = AsyncMock() + search = AsyncMock(return_value=[]) + p1, p2, p3 = _create_patches(AsyncMock(return_value=_stub_note(id=41))) + with p1, p2, p3, patch.object(links_svc, "set_no_rule", answered), \ + patch.object(links_svc, "rule_candidates", search): + out = await lesson_tools.create_lesson( + what="x", when_to_apply=TRIGGER, no_rule="a one-off of this CI host", + ) + assert answered.await_args.args[1:] == (41, "a one-off of this CI host") + search.assert_not_awaited() + assert "rule_candidates" not in out + + +@pytest.mark.asyncio +async def test_rules_and_no_rule_together_write_nothing(): + _user_id_ctx.set(7) + created = AsyncMock(return_value=_stub_note()) + p1, p2, p3 = _create_patches(created) + with p1, p2, p3, patch.object(links_svc, "require_rules", AsyncMock(return_value=[5])): + with pytest.raises(ValueError, match="no_rule"): + await lesson_tools.create_lesson( + what="x", when_to_apply=TRIGGER, rule_ids=[5], no_rule="none fits", + ) + created.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_update_records_no_rule_and_refuses_it_beside_named_rules(): + _user_id_ctx.set(7) + answered = AsyncMock() + updated = AsyncMock(return_value=_stub_note()) + with patch.object(lessons_svc, "update_lesson", updated), \ + patch.object(links_svc, "set_no_rule", answered): + await lesson_tools.update_lesson(lesson_id=41, no_rule="stands alone") + assert answered.await_args.args[1:] == (41, "stands alone") + + updated.reset_mock() + with patch.object(lessons_svc, "update_lesson", updated), \ + patch.object(links_svc, "require_rules", AsyncMock(return_value=[5])): + with pytest.raises(ValueError): + await lesson_tools.update_lesson(lesson_id=41, rule_ids=[5], no_rule="none") + updated.assert_not_awaited() + + +def test_an_empty_rule_list_beside_no_rule_is_one_answer_not_two(): + """`rule_ids=[]` clears; it does not name a rule, so it cannot contradict.""" + links_svc.require_one_answer([], "none fits") + links_svc.require_one_answer(None, "none fits") + links_svc.require_one_answer([5], "") + with pytest.raises(ValueError): + links_svc.require_one_answer([5], "none fits") + + +@pytest.mark.asyncio +async def test_list_unjudged_is_a_listing_and_routes_to_its_own_query(): + _user_id_ctx.set(7) + listed = AsyncMock(return_value=([], 0)) + with patch.object(links_svc, "list_unjudged", listed), \ + patch("scribe.mcp.tools.lessons.access_svc.label_shared_items", AsyncMock(return_value=[])), \ + patch("scribe.mcp.tools.lessons.attach_usage", AsyncMock()): + out = await lesson_tools.list_lessons(unjudged=True, tag="ci", project_id=2) + assert out == {"lessons": [], "total": 0} + assert listed.await_args.kwargs["tag"] == "ci" + assert listed.await_args.kwargs["project_id"] == 2 + with pytest.raises(ValueError): + await lesson_tools.list_lessons(unjudged=True, q="log") + + +def test_backup_skips_a_no_rule_answer_whose_lesson_did_not_restore(): + maps = backup._Maps() + assert backup._build_lesson_no_rule({"lesson_id": 10, "why": "alone"}, maps) is None + maps.notes[10] = 110 + built = backup._build_lesson_no_rule({"lesson_id": 10, "why": "alone"}, maps) + assert (built.lesson_id, built.why) == (110, "alone") + # An answer with no reason is not an answer; it restores as unjudged. + assert backup._build_lesson_no_rule({"lesson_id": 10, "why": " "}, maps) is None + + +def test_the_no_rule_migration_follows_the_link_migration(): + path = Path(__file__).resolve().parents[1] / "alembic" / "versions" / "0112_lesson_no_rule.py" + spec = importlib.util.spec_from_file_location("m0112", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + assert (module.revision, module.down_revision) == ("0112", "0111") diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index 56a6978..a7ee02e 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -27,7 +27,7 @@ def test_backup_version_is_current(): (Named for the number it asserted until v10, which is exactly the drift a name-carrying-a-value invites; it now says what it checks.)""" - assert backup.BACKUP_VERSION == 18 + assert backup.BACKUP_VERSION == 19 def _exportable_note(**over): @@ -131,7 +131,7 @@ def test_a_repo_binding_carries_the_branch_its_ledger_follows(): def _column_guard_targets(): from scribe.models.canonical_system import CanonicalSystem from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse - from scribe.models.lesson_rule_link import LessonRuleLink + from scribe.models.lesson_rule_link import LessonNoRule, LessonRuleLink from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.milestone import Milestone from scribe.models.note import Note @@ -169,6 +169,7 @@ def _column_guard_targets(): "note_supersessions": (NoteSupersession, backup._note_supersession_rows), "rule_relations": (RuleRelation, backup._rule_relation_rows), "lesson_rule_links": (LessonRuleLink, backup._lesson_rule_link_rows), + "lesson_no_rule": (LessonNoRule, backup._lesson_no_rule_rows), "note_usage_events": (NoteUsageEvent, backup._usage_event_rows), "rule_usage_events": (RuleUsageEvent, backup._rule_usage_event_rows), "retrieval_tuning_events": ( @@ -286,6 +287,7 @@ def _import_guard_targets(): "note_supersessions": backup._build_note_supersession, "rule_relations": backup._build_rule_relation, "lesson_rule_links": backup._build_lesson_rule_link, + "lesson_no_rule": backup._build_lesson_no_rule, "note_usage_events": backup._build_usage_event, "rule_usage_events": backup._build_rule_usage_event, "retrieval_tuning_events": backup._build_retrieval_tuning_event, @@ -539,7 +541,9 @@ async def test_export_full_backup_contains_every_declared_section(): # v16: the reasons beside the settings they explain. "retrieval_tuning_events", # v18: which rule each lesson is an instance of. - "lesson_rule_links"): + "lesson_rule_links", + # v19: the lessons judged to fall under no rule. + "lesson_no_rule"): assert key in out, f"missing export section: {key}" assert out[key] == [] From 95be2512abf7b0a0487c883d1a7cde9f640afe81 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 1 Oct 2026 13:34:08 -0400 Subject: [PATCH 5/5] fix(lessons): build the rule-candidate query with trigger_title (#4631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI run 707 failed test_the_trigger_separator_is_spelled_in_exactly_one_place: rule_candidates joined the lesson's claim and trigger with an inline " — ", a second spelling of embeddings.TRIGGER_SEP (#3207). It now calls trigger_title — the same join a rule's own document title is built with, which was the point of the query shape in the first place. Co-Authored-By: Claude Opus 5.5 --- src/scribe/services/lesson_rules.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/scribe/services/lesson_rules.py b/src/scribe/services/lesson_rules.py index f8e6424..9364581 100644 --- a/src/scribe/services/lesson_rules.py +++ b/src/scribe/services/lesson_rules.py @@ -399,7 +399,7 @@ async def rule_candidates( """The rules a lesson most resembles, for the writer to judge against. Searched with the lesson's claim and its trigger — the same two halves a - rule's own document leads with (`{title} — {trigger}`), so like is + rule's own document leads with (`trigger_title`), so like is compared with like; the insight is the story and would dilute the vector. Scoped as any project read is: global rules plus the lesson's project's own. The bar is the explicit rule search's, since this is an explicit @@ -409,10 +409,11 @@ async def rule_candidates( caller can say "candidates unavailable" rather than "nothing resembles". """ from scribe.services.embeddings import ( - DEFAULT_SIMILARITY_THRESHOLD, semantic_search_rules, + DEFAULT_SIMILARITY_THRESHOLD, semantic_search_rules, trigger_title, ) - query = " — ".join(p for p in ((what or "").strip(), (when_to_apply or "").strip()) if p) + # The rule document's own title join, so like is compared with like. + query = trigger_title(what, when_to_apply) report: dict = {} found = await semantic_search_rules( user_id, query, limit=CANDIDATE_LIMIT,