Lessons reach the session that needs them, every kind is a full citizen, and a slow disk no longer takes the instance down #167
@@ -106,7 +106,7 @@ _READ_ONLY_TOOLS = frozenset({
|
||||
# free-text records and withholds the structured ones (#2496).
|
||||
"get_snippet", "list_snippets",
|
||||
"get_process", "list_processes",
|
||||
"get_lesson",
|
||||
"get_lesson", "list_lessons",
|
||||
# Design systems: read, resolve (inheritance + mode), render, and compare
|
||||
# against recorded snippets. All four compute from stored records and write
|
||||
# nothing — the drift report is a report, and applying it is a separate
|
||||
@@ -159,7 +159,7 @@ _READ_ONLY_TOOLS = frozenset({
|
||||
_WRITE_TOOLS = frozenset({
|
||||
# notes, tasks, planning
|
||||
"create_note", "update_note", "delete_note",
|
||||
"create_lesson", "update_lesson",
|
||||
"create_lesson", "update_lesson", "delete_lesson",
|
||||
"create_task", "update_task", "delete_task", "add_task_log",
|
||||
"create_records", "start_planning",
|
||||
"create_milestone", "update_milestone", "delete_milestone",
|
||||
|
||||
@@ -12,8 +12,10 @@ from __future__ import annotations
|
||||
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 lessons as lessons_svc
|
||||
from scribe.services import systems as systems_svc
|
||||
from scribe.services import trash as trash_svc
|
||||
from scribe.mcp.tools import systems as systems_tools
|
||||
from scribe.services.note_usage import record_pulled
|
||||
|
||||
@@ -35,6 +37,55 @@ def _to_dict(note) -> dict:
|
||||
}
|
||||
|
||||
|
||||
async def list_lessons(
|
||||
q: str = "", tag: str = "", limit: int = 50, offset: int = 0,
|
||||
project_id: int = 0,
|
||||
) -> dict:
|
||||
"""List lessons — the kind enumerated, rather than only what a query
|
||||
resembles.
|
||||
|
||||
Semantic search is how a lesson REACHES a session; this is how a person or
|
||||
an agent sees what exists. The two answer different questions, and without
|
||||
this one there is no way to ask "what has been learned here at all".
|
||||
|
||||
Each entry carries its trigger, because a list of lessons sorted by title
|
||||
is a list of claims with the situation — the half that says when each one
|
||||
matters — left off.
|
||||
|
||||
Args:
|
||||
q: Free-text search across title + body (optional).
|
||||
tag: Filter to a single tag (optional).
|
||||
limit: Max results (1-100).
|
||||
offset: Skip this many before returning — page past the cap. `total`
|
||||
is the unpaged count, so it says whether more remains.
|
||||
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.
|
||||
|
||||
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,
|
||||
)
|
||||
labelled = await access_svc.label_shared_items(uid, items)
|
||||
rows = [
|
||||
{
|
||||
"id": it["id"], "title": it["title"], "tags": it.get("tags", []),
|
||||
"preview": it.get("snippet", ""),
|
||||
# Projected by `_note_to_item` straight off the `data` mirror —
|
||||
# absent when the row carries none, rather than an empty string.
|
||||
"when_to_apply": it.get("when_to_apply", ""),
|
||||
**({"shared": True, "owner": it.get("owner")} if it.get("shared") else {}),
|
||||
}
|
||||
for it in labelled
|
||||
]
|
||||
return {"lessons": rows, "total": total}
|
||||
|
||||
|
||||
async def create_lesson(
|
||||
what: str,
|
||||
when_to_apply: str,
|
||||
@@ -192,6 +243,38 @@ async def update_lesson(
|
||||
return _to_dict(note)
|
||||
|
||||
|
||||
async def delete_lesson(lesson_id: int) -> dict:
|
||||
"""Retire a lesson — it moves to the trash and is recoverable.
|
||||
|
||||
Reach for this when a lesson turned out to be wrong, or was superseded by
|
||||
a better one. A lesson that is merely NOT REACHING anyone is usually not a
|
||||
deletion: its trigger is keyed to a situation nobody is in, and rewording
|
||||
that with update_lesson keeps what was learned.
|
||||
|
||||
Deletion was always possible through `delete_note` — a lesson is a note and
|
||||
the trash is kind-agnostic — but nothing said so, and a kind whose own
|
||||
tools offer create/read/update reads as one you cannot retire (#2250, which
|
||||
recorded exactly this for processes).
|
||||
"""
|
||||
uid = current_user_id()
|
||||
note = await lessons_svc.get_lesson(uid, lesson_id)
|
||||
# The KIND is checked before deleting: this tool is reached for by name, so
|
||||
# letting it trash an ordinary note because the id happened to resolve
|
||||
# would be a destructive action taken on a mistyped argument.
|
||||
if note is None:
|
||||
raise ValueError(f"lesson {lesson_id} not found")
|
||||
batch = await trash_svc.delete(uid, "note", lesson_id)
|
||||
if batch is None:
|
||||
raise ValueError(f"lesson {lesson_id} not found")
|
||||
return {
|
||||
"deleted_batch_id": batch,
|
||||
"message": (
|
||||
f"Lesson {lesson_id} moved to trash. Restore with restore('{batch}')."
|
||||
),
|
||||
}
|
||||
|
||||
|
||||
def register(mcp) -> None:
|
||||
for fn in (create_lesson, get_lesson, update_lesson):
|
||||
for fn in (list_lessons, create_lesson, get_lesson, update_lesson,
|
||||
delete_lesson):
|
||||
mcp.tool(name=fn.__name__)(fn)
|
||||
|
||||
@@ -310,8 +310,14 @@ async def find_duplicate_records(kind: str = "note", threshold: float = 0.0) ->
|
||||
project's records, or when you suspect the same ground was covered twice.
|
||||
|
||||
Args:
|
||||
kind: "note" (documents) or "task". Snippets have their own report,
|
||||
kind: "note" (documents), "task", or "lesson". Each kind is compared
|
||||
only against its own — a to-do and a write-up are not alternatives
|
||||
to each other. Snippets have their own report,
|
||||
find_duplicate_snippets, whose groups propose a lossless merge.
|
||||
LESSONS are the kind most worth running this on: their create gate
|
||||
is deliberately permissive (it sits above the band where two
|
||||
genuinely different lessons about one area would block each other),
|
||||
so this report is where that tolerance gets reviewed.
|
||||
threshold: Similarity floor, 0-1. 0 uses the configured setting.
|
||||
|
||||
READ THE `suggestion` FIELD BEFORE ACTING, because the right fix differs by
|
||||
@@ -324,9 +330,15 @@ async def find_duplicate_records(kind: str = "note", threshold: float = 0.0) ->
|
||||
question.
|
||||
"""
|
||||
uid = current_user_id()
|
||||
if kind not in ("note", "task"):
|
||||
raise ValueError('kind must be "note" or "task" — snippets have their '
|
||||
"own report, find_duplicate_snippets")
|
||||
# Validated against the service's own list rather than a second copy here,
|
||||
# so a kind added there reaches this door instead of being refused by a
|
||||
# literal nobody remembered to widen.
|
||||
allowed = tuple(k for k in dedup_svc._REPORT_KINDS if k != "snippet")
|
||||
if kind not in allowed:
|
||||
raise ValueError(
|
||||
f"kind must be one of {', '.join(allowed)} — snippets have their "
|
||||
"own report, find_duplicate_snippets"
|
||||
)
|
||||
return await dedup_svc.find_duplicate_records(
|
||||
uid, kind=kind, threshold=threshold if threshold > 0 else None,
|
||||
)
|
||||
|
||||
@@ -374,8 +374,24 @@ DUPLICATE_THRESHOLD_KEYS = {
|
||||
"snippet": "kb_duplicate_threshold_snippet",
|
||||
"note": "kb_duplicate_threshold_note",
|
||||
"task": "kb_duplicate_threshold_task",
|
||||
"lesson": "kb_duplicate_threshold_lesson",
|
||||
"process": "kb_duplicate_threshold_process",
|
||||
}
|
||||
# The lesson default is the GENERAL semantic floor, and deliberately below its
|
||||
# own write-path bar. The gate sits at 0.96 so it does not refuse two genuinely
|
||||
# different lessons whose triggers read alike — and that tolerance is precisely
|
||||
# what wants reviewing later. So the report looks at the band the gate was told
|
||||
# to let through. Safe here and not at the gate, because a report proposes and
|
||||
# the operator picks, where the gate blocks a write outright.
|
||||
DUPLICATE_DEFAULT_THRESHOLDS = {
|
||||
"snippet": 0.82, "note": 0.93, "task": 0.93, "lesson": 0.90,
|
||||
# A process is prose like a note, and its gate is the general one, so it
|
||||
# takes the note's floor. It is here because every typed kind earns a
|
||||
# report — a kind with create/read/update/delete and no way to ask "did we
|
||||
# record this twice" is one whose duplicates are only ever found by
|
||||
# accident.
|
||||
"process": 0.93,
|
||||
}
|
||||
DUPLICATE_DEFAULT_THRESHOLDS = {"snippet": 0.82, "note": 0.93, "task": 0.93}
|
||||
# Hard cap on returned pairs. A pathologically uniform corpus is O(n²) pairs, and
|
||||
# a report nobody can read is not a report.
|
||||
_MAX_DUPLICATE_PAIRS = 200
|
||||
@@ -512,17 +528,23 @@ _KIND_SUGGESTION = {
|
||||
# kind → the Note-model predicate for BOTH sides of the self-join. Tasks are
|
||||
# notes with a status, not a note_type of their own — the same split every
|
||||
# list surface makes.
|
||||
_REPORT_KINDS = ("snippet", "note", "task")
|
||||
_REPORT_KINDS = ("snippet", "note", "task", "lesson", "process")
|
||||
|
||||
|
||||
def _kind_clauses(kind: str, note_alias):
|
||||
"""The WHERE terms that make an aliased Note row one `kind` of record."""
|
||||
if kind == "snippet":
|
||||
return (note_alias.note_type == SNIPPET_NOTE_TYPE,)
|
||||
if kind == "lesson":
|
||||
return (note_alias.note_type == LESSON_NOTE_TYPE,)
|
||||
if kind == "process":
|
||||
return (note_alias.note_type == "process",)
|
||||
if kind == "task":
|
||||
return (note_alias.note_type == "note", note_alias.status.isnot(None))
|
||||
# kind == "note": documents only — a task is a note with a status, and
|
||||
# mixing them would propose folding a to-do into a write-up.
|
||||
# kind == "note": documents only. A task is a note with a status, and
|
||||
# mixing them would propose folding a to-do into a write-up; the typed
|
||||
# kinds are excluded by the note_type equality for the same reason, so
|
||||
# each kind is only ever compared against its own.
|
||||
return (note_alias.note_type == "note", note_alias.status.is_(None))
|
||||
|
||||
|
||||
|
||||
@@ -240,6 +240,15 @@ def _note_to_item(note: Note) -> dict:
|
||||
if language:
|
||||
item["language"] = language
|
||||
|
||||
# WHEN a lesson applies, same reasoning as the language above: a plain
|
||||
# projection of the `data` mirror, no body parsing. A listing of lessons
|
||||
# without it is a list of claims with the situation — the half that says
|
||||
# when each one matters — left off, and the title's own copy cannot be
|
||||
# recovered by splitting on the em dash, because a claim may contain one.
|
||||
trigger = (note.data or {}).get("when_to_apply") if note.data else None
|
||||
if trigger:
|
||||
item["when_to_apply"] = trigger
|
||||
|
||||
verdict = (note.data or {}).get("verification") if note.data else None
|
||||
if verdict and verdict.get("status"):
|
||||
item["verification"] = {
|
||||
|
||||
@@ -0,0 +1,115 @@
|
||||
"""Every typed record kind gets the same doors (#4164).
|
||||
|
||||
WHY THIS IS ONE GUARD AND NOT FIFTEEN
|
||||
|
||||
Milestone 385 shipped `lesson` across four steps and it still arrived with
|
||||
three of five tools, no way to list it, and no duplicate report. Nothing was
|
||||
red, because every test asked "does what I built work" and none asked "is the
|
||||
kind finished". A per-tool test cannot catch a MISSING tool.
|
||||
|
||||
So this asserts the property directly — a kind with its own `note_type` has a
|
||||
full create/read/update/delete plus a listing, each one registered and each one
|
||||
classified for auth — and derives the expectation from the kinds themselves. A
|
||||
fourth kind added later inherits the bar without anyone remembering to.
|
||||
|
||||
#2250 is the same failure one kind earlier: processes could always be deleted
|
||||
through `delete_note`, but nothing said so, and "a kind whose own tools offer
|
||||
create/read/update reads as one you cannot retire".
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
|
||||
import pytest
|
||||
|
||||
from scribe.mcp.server import (
|
||||
_DELIBERATELY_WRITE_SCOPED,
|
||||
_READ_ONLY_TOOLS,
|
||||
_WRITE_TOOLS,
|
||||
)
|
||||
from scribe.services.dedup import _REPORT_KINDS
|
||||
|
||||
# singular -> the tools module, which is also the plural used by the listing.
|
||||
KINDS = {"snippet": "snippets", "process": "processes", "lesson": "lessons"}
|
||||
|
||||
|
||||
def _module(plural: str):
|
||||
return importlib.import_module(f"scribe.mcp.tools.{plural}")
|
||||
|
||||
|
||||
def _expected_tools(singular: str, plural: str) -> list[str]:
|
||||
return [
|
||||
f"list_{plural}",
|
||||
f"create_{singular}",
|
||||
f"get_{singular}",
|
||||
f"update_{singular}",
|
||||
f"delete_{singular}",
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("singular,plural", sorted(KINDS.items()))
|
||||
def test_a_kind_has_all_five_doors(singular, plural):
|
||||
"""THE guard. A kind that can be created and not listed, or updated and not
|
||||
retired, is one whose gaps are invisible until someone needs the missing
|
||||
half."""
|
||||
module = _module(plural)
|
||||
missing = [
|
||||
name for name in _expected_tools(singular, plural)
|
||||
if not callable(getattr(module, name, None))
|
||||
]
|
||||
assert not missing, f"{plural}.py is missing {missing}"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("singular,plural", sorted(KINDS.items()))
|
||||
def test_every_door_is_actually_registered(singular, plural):
|
||||
"""Defining the function is not offering it. `register(mcp)` is what puts a
|
||||
tool on the surface, and a handler left out of that list is dead code that
|
||||
every other test still exercises directly."""
|
||||
module = _module(plural)
|
||||
registered: list[str] = []
|
||||
|
||||
class _Recorder:
|
||||
def tool(self, name):
|
||||
registered.append(name)
|
||||
return lambda fn: fn
|
||||
|
||||
module.register(_Recorder())
|
||||
|
||||
missing = [n for n in _expected_tools(singular, plural) if n not in registered]
|
||||
assert not missing, f"{plural}.register() does not offer {missing}"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("singular,plural", sorted(KINDS.items()))
|
||||
def test_every_door_is_classified_for_auth(singular, plural):
|
||||
"""`test_mcp_auth` requires every registered tool to sit in exactly one of
|
||||
the three sets. Asserted per KIND as well, because that test enumerates
|
||||
what is registered — so a whole module wired up with none of its tools
|
||||
classified is caught here by the kind that owns them."""
|
||||
classified = _READ_ONLY_TOOLS | _WRITE_TOOLS | _DELIBERATELY_WRITE_SCOPED
|
||||
missing = [n for n in _expected_tools(singular, plural) if n not in classified]
|
||||
assert not missing, f"unclassified in server.py: {missing}"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("singular", sorted(KINDS))
|
||||
def test_every_kind_can_be_asked_whether_it_holds_duplicates(singular):
|
||||
"""A kind with no report is one whose duplicates are found by accident.
|
||||
|
||||
It matters most where the write-path gate is deliberately permissive: a
|
||||
lesson's bar sits above the band where two genuinely different lessons
|
||||
would block each other, and the report is where that tolerance is meant to
|
||||
be reviewed.
|
||||
"""
|
||||
assert singular in _REPORT_KINDS
|
||||
|
||||
|
||||
def test_the_surface_guards_can_fail():
|
||||
"""Rule 167: falsify the shape these assert against, so a guard that has
|
||||
quietly stopped describing anything cannot pass by describing nothing."""
|
||||
assert _expected_tools("lesson", "lessons") == [
|
||||
"list_lessons", "create_lesson", "get_lesson",
|
||||
"update_lesson", "delete_lesson",
|
||||
]
|
||||
# A kind that does not exist has no module — the lookup is real, not a
|
||||
# getattr that returns None for everything.
|
||||
with pytest.raises(ModuleNotFoundError):
|
||||
_module("widgets")
|
||||
Reference in New Issue
Block a user