diff --git a/frontend/src/api/lessons.ts b/frontend/src/api/lessons.ts index 684200d..4b66ee8 100644 --- a/frontend/src/api/lessons.ts +++ b/frontend/src/api/lessons.ts @@ -65,6 +65,10 @@ export interface LessonListRow { tags: string[]; when_to_apply?: string; snippet?: string; + /** Surfaced-vs-opened for this lesson. Always present on a listing from + * this door, zero-filled where nothing has been recorded — absent only + * when a row came from somewhere else. */ + usage?: RecordUsage; shared?: boolean; owner?: string | null; } diff --git a/frontend/src/views/LessonDetailView.vue b/frontend/src/views/LessonDetailView.vue index 53f46de..81c5617 100644 --- a/frontend/src/views/LessonDetailView.vue +++ b/frontend/src/views/LessonDetailView.vue @@ -24,6 +24,7 @@ import { apiErrorMessage } from "@/api/client"; import { deleteLesson, getLesson, type Lesson } from "@/api/lessons"; import ConfirmDialog from "@/components/ConfirmDialog.vue"; import TagPill from "@/components/TagPill.vue"; +import UsageBadge from "@/components/UsageBadge.vue"; import { useToastStore } from "@/stores/toast"; import { renderMarkdown } from "@/utils/markdown"; @@ -99,7 +100,19 @@ onMounted(load);

{{ lesson.when_to_apply }}

-

{{ lesson.what }}

+
+

{{ lesson.what }}

+ + +
dict: # retrieval as any other note, so a getter that records nothing would leave # the kind permanently at zero pulls — reading as dead weight beside kinds # that merely had a counter (#2476, the repeat of #2245). + # Read BEFORE the pull is recorded, so the number an agent is shown is the + # one that was true when it asked — otherwise every first read of a lesson + # reports a pull that is its own. + out["usage"] = (await usage_for_notes([int(note.id)])).get( + int(note.id), empty_usage() + ) record_pulled( user_id=uid, note_id=int(note.id), source="mcp_get_lesson", project_id=project_id, diff --git a/src/scribe/routes/lessons.py b/src/scribe/routes/lessons.py index a974467..cd384d1 100644 --- a/src/scribe/routes/lessons.py +++ b/src/scribe/routes/lessons.py @@ -72,10 +72,21 @@ async def list_lessons_route(): offset=offset, project_id=project_id, ) - return jsonify({ - "lessons": await label_shared_items(uid, items), - "total": total, - }) + 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 + # "never surfaced" rather than treating a missing field as a state. + # + # Lessons were the one kind collecting this and showing it nowhere (#4196). + # The counts matter more here than on a snippet: the promotion question a + # lesson eventually raises — does this bind? — is answered by repeatedly + # surfaced AND repeatedly opened, and the far commoner reading of the same + # row is that the trigger fires on the wrong situation, which `update_lesson` + # exists to fix. + usage = await usage_for_notes([int(it["id"]) for it in items]) + for it in items: + it["usage"] = usage.get(int(it["id"]), empty_usage()) + return jsonify({"lessons": items, "total": total}) @lessons_bp.route("/taught-by/", methods=["GET"]) diff --git a/tests/test_lesson_usage_surface.py b/tests/test_lesson_usage_surface.py new file mode 100644 index 0000000..2789442 --- /dev/null +++ b/tests/test_lesson_usage_surface.py @@ -0,0 +1,189 @@ +"""A lesson's surfaced-vs-opened counts reach a reader (#4196). + +The lesson slot has recorded usage since it shipped, and until this nothing +showed it. `get_lesson`'s REST door attached it; the list did not, neither MCP +door did, and no view rendered it — so the one kind whose central question is +"is this trigger firing on the right situation" was the one kind whose answer +was unreadable. + +WHY THE COUNTS MATTER MORE HERE THAN ON A SNIPPET. #4196 asks whether a lesson +repeatedly followed should become a rule, and names the trap: raw frequency +cannot tell "this should bind" from "this trigger is too broad", which is the +commoner reading. Surfaced-AND-opened can. Neither question is answerable by +anything that cannot see the numbers, which is why this is the first step and +not the threshold. + +NO THRESHOLD IS ASSERTED ANYWHERE HERE, deliberately. At the time of writing +the corpus is 10 lessons with 7 recorded surfacings and 3 opens; a promotion +rule fitted to that would be fitting noise, and `UsageBadge` already declines +to render a verdict on fewer than three surfacings for the same reason. +""" +import inspect +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.mcp.tools import lessons as lesson_tools +from scribe.services.note_usage import empty_usage + + +def _rows(): + return [ + {"id": 7, "title": "a", "tags": [], "snippet": "", "when_to_apply": "x"}, + {"id": 9, "title": "b", "tags": [], "snippet": "", "when_to_apply": "y"}, + ] + + +@pytest.mark.asyncio +async def test_the_mcp_listing_carries_usage_for_every_row(): + """Zero-filled, not omitted. A missing key would make the reader treat + "never surfaced" and "not reported" as the same thing, which is the + distinction the whole readout exists to keep.""" + used = {7: {**empty_usage(), "surfaced_count": 5, "pull_count": 2}} + with ( + patch.object(lesson_tools, "current_user_id", return_value=1), + patch.object(lesson_tools.knowledge_svc, "query_knowledge", + new=AsyncMock(return_value=(_rows(), 2))), + patch.object(lesson_tools.access_svc, "label_shared_items", + new=AsyncMock(side_effect=lambda _uid, items: items)), + patch.object(lesson_tools, "usage_for_notes", + new=AsyncMock(return_value=used)), + ): + out = await lesson_tools.list_lessons() + + by_id = {r["id"]: r for r in out["lessons"]} + assert by_id[7]["usage"]["surfaced_count"] == 5 + assert by_id[7]["usage"]["pull_count"] == 2 + # The row nobody has surfaced still carries the key. + assert by_id[9]["usage"] == empty_usage() + + +@pytest.mark.asyncio +async def test_the_listing_asks_for_usage_once_for_the_whole_page(): + """One aggregate, not one lookup per row — the snippet listing's own + comment calls a per-row read N+1 by construction, and a lesson list is the + same shape.""" + reader = AsyncMock(return_value={}) + with ( + patch.object(lesson_tools, "current_user_id", return_value=1), + patch.object(lesson_tools.knowledge_svc, "query_knowledge", + new=AsyncMock(return_value=(_rows(), 2))), + patch.object(lesson_tools.access_svc, "label_shared_items", + new=AsyncMock(side_effect=lambda _uid, items: items)), + patch.object(lesson_tools, "usage_for_notes", new=reader), + ): + await lesson_tools.list_lessons() + + assert reader.await_count == 1 + assert sorted(reader.await_args.args[0]) == [7, 9] + + +@pytest.mark.asyncio +async def test_get_lesson_reads_the_count_before_recording_its_own_pull(): + """Otherwise the first read of a lesson reports a pull that is its own. + + `get_lesson` records a pull on every open — it has to, or the kind sits + permanently at zero and reads as dead weight. That makes the ORDER load + bearing in a way it is not for kinds that only count. + """ + order: list[str] = [] + note = MagicMock(id=7, user_id=1) + + async def _usage_read(ids): + order.append("read") + return {7: {**empty_usage(), "surfaced_count": 4, "pull_count": 1}} + + with ( + patch.object(lesson_tools, "current_user_id", return_value=1), + patch.object(lesson_tools.lessons_svc, "get_lesson", + new=AsyncMock(return_value=note)), + patch.object(lesson_tools, "_to_dict", return_value={"id": 7}), + patch.object(lesson_tools.access_svc, "describe_provenance", + new=AsyncMock(return_value={})), + patch.object(lesson_tools, "usage_for_notes", new=_usage_read), + patch.object(lesson_tools, "record_pulled", + side_effect=lambda **_kw: order.append("pull")), + ): + out = await lesson_tools.get_lesson(7) + + assert order == ["read", "pull"], ( + "the pull was recorded before the count was read, so the number the " + "caller is shown includes the read that produced it" + ) + assert out["usage"]["pull_count"] == 1 + + +# ── the REST door, on structure (rule 167) ─────────────────────────────────── +# +# Its siblings in test_lesson_rest_door.py are source guards for the same +# reason: the route is decorated and returns a Quart response, so driving it +# means standing up the app. What matters here is reachable from the source +# and falsifiable from it — an aggregate rather than a per-row read, and the +# same read-then-record order the MCP door is pinned to above. + + +def _route_source(name: str) -> str: + from scribe.routes import lessons as routes + + src = inspect.getsource(routes) + start = src.index(f"async def {name}(") + nxt = src.find("\n@lessons_bp.route", start) + return src[start:nxt if nxt > 0 else len(src)] + + +def test_the_rest_listing_reads_usage_once_for_the_page(): + """A per-row lookup would be N+1 by construction — the listing's own + comment says so, and this is what makes that comment checkable.""" + src = _route_source("list_lessons_route") + assert src.count("usage_for_notes(") == 1 + # The one call is not inside the loop that assigns the rows. + call = src.index("usage_for_notes(") + assign = src.index('["usage"]') + assert call < assign + + +def test_every_rest_row_carries_the_key_even_at_zero(): + """"Never surfaced" is a state the UI renders; a missing field is not.""" + src = _route_source("list_lessons_route") + assert "empty_usage()" in src, ( + "a row with no recorded usage would come back without the key, and a " + "reader cannot tell that from a reporting failure" + ) + + +def test_the_rest_detail_door_also_reads_before_it_records(): + """Two doors that disagree about what the number counts are worse than + one door that is wrong, because only one of them looks wrong.""" + src = _route_source("get_lesson_route") + assert src.index("usage_for_notes(") < src.index("record_pulled(") + + +def test_these_guards_can_fail(): + """Rule 167: falsify the shape they assert against, so a guard that has + quietly stopped describing anything cannot pass by describing nothing.""" + src = _route_source("list_lessons_route") + assert "def list_lessons_route" in src + assert "def get_lesson_route" not in src, "the slice ran past its route" + + +def test_the_detail_view_renders_the_badge_rather_than_respelling_it(): + """Snippet #3460: reach for UsageBadge and pass the kind's own advice; a + view's scoped re-spelling of `.usage-tag` is what that component replaced. + + The advice is the kind-specific half. For a lesson it points at the + trigger, not at deletion — a lesson nobody opens is usually keyed to a + situation nobody is in, which is `update_lesson`'s own words and the + reading #4196 calls the commoner one. + """ + view = (Path(__file__).resolve().parents[1] + / "frontend/src/views/LessonDetailView.vue").read_text() + assert "UsageBadge" in view + assert "usage-tag" not in view, "re-spelled the chip instead of reusing it" + + key = 'dead-weight-advice="' + start = view.index(key) + len(key) + advice = view[start:view.index('"', start)] + assert "when_to_apply" in advice, ( + f"the dead-weight advice does not point at the trigger: {advice!r}" + )