feat(lessons): a lesson names the rule it is an instance of — lesson_rule_links (milestone 440 step 1, #4630)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 15s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 54s
CI & Build / Python tests (push) Failing after 1m13s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 15s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 54s
CI & Build / Python tests (push) Failing after 1m13s
CI & Build / Build & push image (push) Skipped
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/<id>/rules/<rule_id>. 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user