feat(lessons): the kind whose whole question is "is this trigger right" was the one kind that could not see its own counts (#4196)
CI & Build / Python lint (push) Successful in 8s
CI & Build / Plugin hooks (push) Successful in 17s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 57s
CI & Build / Python tests (push) Successful in 1m41s
CI & Build / Build & push image (push) Successful in 36s
CI & Build / Python lint (push) Successful in 8s
CI & Build / Plugin hooks (push) Successful in 17s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 57s
CI & Build / Python tests (push) Successful in 1m41s
CI & Build / Build & push image (push) Successful in 36s
The lesson slot has recorded surfaced-vs-opened since it shipped. Nothing showed it. `get_lesson`'s REST door attached `usage` to the payload and no view rendered it; the listing did not attach it at all, and neither MCP door did. #4196 asks when a lesson that keeps getting followed should become a rule, and names the trap in the same breath: raw frequency cannot separate "this should bind" from "this trigger is too broad", and the second is the commoner reading by a wide margin. Surfaced-AND-opened can separate them. Neither question is answerable by a reader who cannot see the numbers, which is why this is the first step and not the threshold. NO THRESHOLD IS PROPOSED HERE, deliberately. The corpus today is 10 lessons with 7 recorded surfacings and 3 opens, over about fifteen hours of usage data. A promotion rule fitted to that would be fitting noise — #3311's failure, and the warning lesson #4228 was written to carry. `UsageBadge` already declines to render a verdict under three surfacings for the same reason. So #4196 stays open: its subject, the promotion path, is still unbuilt. What lands is the evidence it needs. - REST `GET /api/lessons` and MCP `list_lessons` attach `usage` to every row, from one aggregate per page rather than a per-row read, which would be N+1 by construction. Every row carries the key zero-filled, so "never surfaced" is a state a reader can see rather than a missing field they have to interpret. - MCP `get_lesson` attaches it too, and reads it BEFORE recording its own pull. That door records a pull on every open — it has to, or the kind sits permanently at zero — which makes the order load-bearing in a way it is not for a kind that only counts. The REST detail door already ordered it this way; the two now agree about what the number means. - `LessonDetailView` renders `UsageBadge` (snippet #3460) rather than re-spelling the chip, with the advice keyed to this kind: a lesson that is repeatedly offered and never opened is usually keyed to a situation nobody is in, so it points at re-keying `when_to_apply`, not at deleting the claim. Guards, in the two styles this pair of doors already uses: the MCP side driven behaviourally through mocks, including the call ORDER for `get_lesson`; the REST side on structure like its siblings in test_lesson_rest_door.py, because the route is decorated and returns a Quart response. Rule 167's falsifier is included. KNOWN GAP, not fixed here: `KnowledgeView` is the only lesson LIST in the UI and it reads `/knowledge`, not `/lessons` — so the REST listing change reaches `frontend/src/api/lessons.ts::listLessons`, which currently has no consumer. The agent-facing listing does reach a reader today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
<p class="ld-trigger-text">{{ lesson.when_to_apply }}</p>
|
||||
</section>
|
||||
|
||||
<h1 class="ld-what">{{ lesson.what }}</h1>
|
||||
<div class="ld-what-row">
|
||||
<h1 class="ld-what">{{ lesson.what }}</h1>
|
||||
<!-- Lessons collected surfaced-vs-opened from the day the slot shipped
|
||||
and showed it nowhere (#4196). It belongs here above anywhere
|
||||
else: the reading that matters is almost never "delete this", it
|
||||
is "the trigger is keyed to a situation nobody is in", and the
|
||||
place to act on that is the lesson you are already looking at. -->
|
||||
<UsageBadge
|
||||
:usage="lesson.usage"
|
||||
noun="lesson"
|
||||
dead-weight-advice="Repeatedly offered and never opened usually means the trigger fires on the wrong situation — re-key `when_to_apply` rather than deleting the claim."
|
||||
/>
|
||||
</div>
|
||||
|
||||
<div
|
||||
v-if="insightHtml"
|
||||
@@ -210,6 +223,13 @@ onMounted(load);
|
||||
}
|
||||
.ld-trigger-text { margin: 0; font-size: 0.98rem; line-height: 1.5; }
|
||||
|
||||
.ld-what-row {
|
||||
display: flex;
|
||||
align-items: baseline;
|
||||
gap: var(--fs-space-2);
|
||||
flex-wrap: wrap;
|
||||
}
|
||||
|
||||
.ld-what {
|
||||
margin: 0 0 1.25rem;
|
||||
font-size: 1.25rem;
|
||||
|
||||
@@ -17,7 +17,7 @@ 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
|
||||
from scribe.services.note_usage import empty_usage, record_pulled, usage_for_notes
|
||||
|
||||
|
||||
# The payload shape lives in the service (`lesson_to_dict`), shared with the
|
||||
@@ -61,6 +61,11 @@ async def list_lessons(
|
||||
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
|
||||
# triggers are firing and which are not, which is the reading that leads to
|
||||
# `update_lesson` rather than to a second lesson about the same failure.
|
||||
usage = await usage_for_notes([int(it["id"]) for it in labelled])
|
||||
rows = [
|
||||
{
|
||||
"id": it["id"], "title": it["title"], "tags": it.get("tags", []),
|
||||
@@ -68,6 +73,7 @@ async def list_lessons(
|
||||
# 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", ""),
|
||||
"usage": usage.get(int(it["id"]), empty_usage()),
|
||||
**({"shared": True, "owner": it.get("owner")} if it.get("shared") else {}),
|
||||
}
|
||||
for it in labelled
|
||||
@@ -203,6 +209,12 @@ async def get_lesson(lesson_id: int, project_id: int = 0) -> 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,
|
||||
|
||||
@@ -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/<int:record_id>", methods=["GET"])
|
||||
|
||||
@@ -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}"
|
||||
)
|
||||
Reference in New Issue
Block a user