diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index f5af39d..a7672df 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -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", diff --git a/src/scribe/mcp/tools/lessons.py b/src/scribe/mcp/tools/lessons.py index 32c9d2a..664b3f0 100644 --- a/src/scribe/mcp/tools/lessons.py +++ b/src/scribe/mcp/tools/lessons.py @@ -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) diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index d13b490..c72327e 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -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, ) diff --git a/src/scribe/services/dedup.py b/src/scribe/services/dedup.py index aa04f0f..7d5c24a 100644 --- a/src/scribe/services/dedup.py +++ b/src/scribe/services/dedup.py @@ -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)) diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index 285dfc0..dd7959d 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -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"] = { diff --git a/tests/test_record_kind_surface.py b/tests/test_record_kind_surface.py new file mode 100644 index 0000000..b8458a6 --- /dev/null +++ b/tests/test_record_kind_surface.py @@ -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")