diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 381e67d..399bea4 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -28,6 +28,7 @@ from scribe.services import shape_ledger as shape_ledger_svc from scribe.services import snippets as snippets_svc from scribe.services.access import label_shared_items, owner_names_for from scribe.services.embeddings import semantic_search_notes, semantic_search_rules +from scribe.services.lessons import LESSON_NOTE_TYPE from scribe.services.note_usage import record_surfaced from scribe.services.rule_usage import record_rule_surfaced from scribe.services.supersession import superseded_ids @@ -759,6 +760,122 @@ async def _reserve_slot_for_reuse( return (kept + fresh)[:top_k] +async def _reserve_slot_for_lesson( + user_id: int, + query: str, + kept: list, + cfg: dict, + *, + project_id: int | None, + already: set[int], +) -> tuple[list, int | None]: + """Guarantee a lesson one slot, if one clears the bar (milestone 385 step 5). + + WHY A SLOT, AND WHAT IT ACTUALLY DISPLACES + + The step's own framing was that "a slot spent on a lesson is a slot not + spent on a rule that binds". That is not what happens here, and the + correction matters for judging the cost: the notes menu and the rule hints + are separate functions with separate budgets, composed by the caller + (`build_prompt_rule_hint` says why). A line reserved in THIS menu displaces + a note, a snippet or an issue — never a rule. + + THE ASYMMETRY, which is `preference_slot`'s argument on a different corpus: + + - a NOTE crowded out of this menu is a lost convenience. It stays + searchable, and the operator can ask for it. + - a RULE crowded out still fires at an act arm. The prompt hit is a + preview of a second chance. + - a LESSON crowded out is the feature failing. A lesson exists only to be + met at the moment it applies — nobody browses lessons looking for one — + so the arm that surfaces it IS its delivery, and the loss is total and + silent. Silent delivery failure is the exact shape #3727 recorded: an + insight with no home arrived as a rule proposal instead. + + And the ratio only moves one way. Lessons are by design rare and hard-won + while project records grow with the work, which is the 200:1 problem + `reuse_slot` was built for (#2246), before it has had a chance to be + measured here. + + THE SLOT BUYS POSITION, NOT A LOWER BAR. It reserves at the menu's own + threshold, so a weak lesson cannot buy the line and silence stays the + default — the discipline both existing slots keep. + + IT EXTENDS, IT NEVER DISPLACES, siding with `preference_slot` over + `reuse_slot`. Two reasons, and the second is the one that would be hard to + recover later: a displaced hit was returned by the general search and sits + in that call's `retrieval_logs` row, so evicting it makes the two tables + disagree about the same call for a reason nothing in the data explains + (#3668, and milestone #379 is what that costs). The first is voice — a + lesson does not bind, and a record that does not bind should not be able + to throw a better-scoring one off the menu. + + Returns the possibly-extended list and the id the slot spent. The caller + needs that id to keep each source's surfaced set matching its own log row: + this slot records its own surfacing under its own name, so counting it + again under `auto_inject` would double it. + """ + if any(_record_kind(n) == LESSON_NOTE_TYPE for _s, n in kept): + return kept, None # a lesson already placed on score + + _t0 = time.perf_counter() + _rep: dict = {} + # KIND-FILTERED, so the slot can only ever be spent on what it is for — + # `preference_slot`'s reasoning: verifying the kind after an open search + # would let a stray note buy the line, and that line would be + # indistinguishable from one that earned its place. + # + # `include_global_kinds` is the half that makes a lesson reachable at all + # from a project it was not written on, which is this kind's whole claim + # (#3730). Without it the slot would be a guarantee that silently only + # applies to lessons learned here. + found = await semantic_search_notes( + user_id, query, + limit=1, + threshold=cfg["threshold"], + project_id=project_id, + exclude_ids={int(n.id) for _s, n in kept}, + note_type=(LESSON_NOTE_TYPE,), + include_global_kinds=True, + scope="browse", + report=_rep, + ) + fresh = [(s, n) for s, n in found if int(n.id) not in already] + # ITS OWN SOURCE, from the first deploy. This slot is a claim that a kind + # deserves a guaranteed line, and a claim like that has to be falsifiable: + # `best_available_id` (#3807) names the lesson a bar refused, and the + # result count says how often the guarantee was actually spent. Without + # this row the question "does the lesson slot earn its line?" would have no + # data behind it in either direction — which is #2463's finding, recorded + # about the slot that shipped without one. + record_retrieval( + user_id=user_id, source="lesson_slot", query=query, + threshold=cfg["threshold"], limit=1, project_id=project_id, + is_task=None, results=fresh, + best_available=_rep.get("best_available_score"), + best_available_id=_rep.get("best_available_id"), + searched=bool(_rep.get("searched", True)), + suppressed=len(found) - len(fresh), + duration_ms=(time.perf_counter() - _t0) * 1000.0, + ) + kept_ids = {int(n.id) for _s, n in kept} + slot = [ + (s, n) for s, n in found + if _record_kind(n) == LESSON_NOTE_TYPE and int(n.id) not in kept_ids + ][:1] + if not slot: + return kept, None + + slot_id = int(slot[0][1].id) + # FRESH ONLY, matching the row above: a ledger repeat is rendered (#4101) + # but is not a new surfacing, so this source's two tables stay identical. + if slot_id not in already: + record_surfaced( + user_id=user_id, note_ids=[slot_id], source="lesson_slot", + ) + return kept + slot, slot_id + + async def build_autoinject_hint( user_id: int, query: str, @@ -811,6 +928,14 @@ async def build_autoinject_hint( limit=cfg["top_k"], threshold=cfg["threshold"], project_id=(project_id or None), + # LESSONS ARE PROJECT-INDEPENDENT (#3730), so they join this menu's + # candidate set from wherever they were learned. Widening it is what + # makes the reserved slot below falsifiable rather than decorative: if + # the slot were the only path a lesson had, the general contest would + # be permanently closed to the kind and "the slot earns its line" would + # be true by construction. The switch adds nothing else — it ORs in + # `GLOBAL_NOTE_TYPES` and no other kind is in it. + include_global_kinds=True, # Injection is the one retrieval nobody asked for, so it takes the BROWSE # scope: never a record shared one-to-one with the operator. What can # still appear is a collaborator's note inside a shared project — legible @@ -850,6 +975,14 @@ async def build_autoinject_hint( user_id, q, kept, cfg, project_id=(project_id or None), already=already, ) + # AFTER the reuse slot, because that one evicts the menu's weakest hit + # while this one extends: running them the other way round would let a + # reserved lesson be the line reuse throws off, and a slot that another + # slot can silently undo is not a guarantee. + kept, lesson_slot_id = await _reserve_slot_for_lesson( + user_id, q, kept, cfg, project_id=(project_id or None), + already=already, + ) # A collaborator's note can reach this menu via a shared project, and the # operator never asked for it — so say whose it is. Unattributed, it reads as @@ -867,10 +1000,34 @@ async def build_autoinject_hint( # once, rather than each repeated line having to explain itself. lines = [ "> Possibly relevant from your Scribe records — open any in full with " - "`get_note(id)`, or `get_snippet` / `get_process` for those kinds " - "(titles only; a line marked `seen` was surfaced earlier this session " - "and may no longer be in context):", + "`get_note(id)`, or `get_snippet` / `get_process` / `get_lesson` for " + "those kinds (titles only; a line marked `seen` was surfaced earlier " + "this session and may no longer be in context):", ] + # THE REGISTER, SAID ONCE AND ONLY WHEN IT APPLIES (milestone 385 step 5). + # + # The operator's requirement for this kind was "they don't always have to + # be followed", and the risk is not that a reader mistakes a lesson for a + # rule — this menu's voice is already the non-binding one, deliberately + # (see the `seen` marker below). The risk is the opposite: read as one more + # title in a list of MATERIAL, a lesson looks like something to open if + # curious, when it is advice someone paid for. The line has to say "weigh + # this" without acquiring the rule arms' "before deciding it does not + # apply", which binds. + # + # In the HEADER rather than on each line, for the reason the `seen` flag is + # a flag: the meaning is the same for every lesson on the menu, and a + # clause repeated per line would cost more than it says. Conditional + # because a menu with no lesson should not pay for the sentence, and + # because a header that explains an absent kind reads as boilerplate — + # which is how a reader learns to skip headers. + if any(_record_kind(n) == LESSON_NOTE_TYPE for _s, n in kept): + lines.append( + "> A line marked `lesson` is something an earlier session learned " + "the hard way, kept because it should transfer. Weigh it against " + "what you are doing and use your judgement — a lesson is not a " + "rule and binds nothing." + ) # A superseded record is DEMOTED, not removed (#278) — so one can still reach # this menu, and when it does the reader has to be told. An agent handed # stale material with nothing marking it acts on it with full confidence, @@ -906,9 +1063,18 @@ async def build_autoinject_hint( # make this table disagree with `retrieval_logs` about the same call — # #3668's identity, which is the cheapest true statement available about # this pair of tables and is not worth a marker's convenience. + # THE RESERVED LESSON IS NOT THIS ARM'S SURFACING. It was fetched by its own + # query and already recorded under `lesson_slot`, so counting it here would + # book one delivery twice and leave `lesson_slot`'s two tables describing + # different numbers of the same event — #3668's identity, which is the + # cheapest true statement available about this pair of tables. It stays in + # `note_ids`, which is the session LEDGER and must list every line rendered. record_surfaced( user_id=user_id, - note_ids=[i for i in note_ids if i not in already], + note_ids=[ + i for i in note_ids + if i not in already and i != lesson_slot_id + ], source="auto_inject", ) @@ -1709,8 +1875,28 @@ async def build_write_path_hint( # written and answers nothing; an ISSUE is corrective work with a # root cause in it, and a non-task note is durable knowledge. Both # earned their place; a todo did not. - note_type=("snippet", "note"), + # + # AND LESSONS (milestone 385 step 5). This arm is kind-FILTERED, so + # a kind absent from this tuple is not merely outranked here — it + # is unreachable, and nothing reports an arm that never had the + # candidate (#3702). The founding example of the kind is a lesson + # about a code shape ("two absolutely-positioned siblings"), which + # is the moment this arm fires and no other. + # + # NO RESERVED SLOT HERE, unlike the prompt menu. This arm fires + # before EVERY Write and Edit, where a guaranteed extra line is a + # guaranteed extra interruption per keystroke-batch — the same + # argument that keeps the act arms' budgets tight. And the contest + # is already fair: the field is snippets, issues and lessons rather + # than the whole corpus, so the 200:1 dilution a slot answers is + # not what happens here. Lessons surfaced by this arm are + # identifiable in the telemetry by their kind. + note_type=("snippet", "note", LESSON_NOTE_TYPE), task_kind="issue", + # Project-independent, for the reason the prompt menu passes it: + # a lesson's claim is that it transfers, and an arm scoped to the + # project it was written on cannot test that claim (#3730). + include_global_kinds=True, # Same reasoning as auto-inject: nobody asked for this, so it takes # the browse scope and never surfaces a one-to-one direct share. scope="browse", @@ -1803,6 +1989,13 @@ async def build_write_path_hint( marker, { "id": int(note.id), "title": note.title, "user_id": note.user_id, + # Carried, not re-read off the rendered marker. The + # marker is prose assembled for a human and it already + # varies by kind, language and the `seen` flag — a + # header that decided what to say by matching substrings + # in it would break the next time a marker is reworded, + # silently and in the direction of saying nothing. + "kind": kind, # Carried so the line can disclose a cross-language hit # (#2244). The semantic arm is where these actually arise — # a snippet recorded at the path you're editing is almost @@ -1924,11 +2117,23 @@ async def build_write_path_hint( lines.append( f"> Prior art already recorded in Scribe for `{path}` — open one with " "`get_snippet(id)` for a snippet, `get_task(id)` for an issue, " - "`get_note(id)` otherwise. Reuse a snippet rather than writing a fresh " - "one-off; read an issue before repeating what it records " + "`get_lesson(id)` for a lesson, `get_note(id)` otherwise. Reuse a " + "snippet rather than writing a fresh one-off; read an issue before " + "repeating what it records " "(titles only; a line marked `seen` was surfaced earlier this " "session and may no longer be in context):" ) + # The same clause the prompt menu carries, on the same condition and for + # the same reason: this menu's three other kinds are all things that WERE + # done here, and a lesson is the one line that is advice. Said once, only + # when one is on the menu. + if any(i.get("kind") == LESSON_NOTE_TYPE for i, _m, _o, _l in rendered): + lines.append( + "> A line marked `lesson` is something an earlier session learned " + "the hard way, kept because it should transfer. Weigh it against " + "the code you are about to write and use your judgement — a lesson " + "is not a rule and binds nothing." + ) # Say what a language tag MEANS, and only when one is actually on the menu. # Without this the reader has to infer why "· python" is attached to a hit on # a .ts file, and the two ways of guessing wrong are both bad: dismiss it as diff --git a/src/scribe/services/retrieval_surfaces.py b/src/scribe/services/retrieval_surfaces.py index 88ae47b..cecdf7e 100644 --- a/src/scribe/services/retrieval_surfaces.py +++ b/src/scribe/services/retrieval_surfaces.py @@ -209,12 +209,20 @@ SURFACES: dict[str, Surface] = { ), } -# Reserved slots are deliberately absent. `preference_slot` and `reuse_slot` -# borrow their parent arm's floor and are hard-limited to one hit each, because -# their entire purpose is to guarantee a single line to a kind of record that -# keeps losing a general score contest (#2246, #3894). A budget of "1" is the -# feature; exposing it as tunable would invite setting it to 0 and silently -# removing the guarantee. +# Reserved slots are deliberately absent. `preference_slot`, `reuse_slot` and +# `lesson_slot` borrow their parent arm's floor and are hard-limited to one hit +# each, because their entire purpose is to guarantee a single line to a kind of +# record that keeps losing a general score contest (#2246, #3894) — or, for +# `lesson_slot`, to a kind whose loss is total rather than merely a lost +# convenience, since a lesson has no act arm to fall back on and nobody browses +# lessons looking for one (milestone 385). A budget of "1" is the feature; +# exposing it as tunable would invite setting it to 0 and silently removing the +# guarantee. +# +# They are still logged under their own `source`, which is what keeps them +# judgeable without being tunable: `best_available_id` names the record each +# bar refused, so a slot that never places, or one that places weak hits, shows +# up as evidence rather than as an argument. def surface_names() -> list[str]: diff --git a/tests/test_lesson_surfacing.py b/tests/test_lesson_surfacing.py new file mode 100644 index 0000000..238e4ad --- /dev/null +++ b/tests/test_lesson_surfacing.py @@ -0,0 +1,305 @@ +"""A lesson reaches the session it applies to — milestone 385, step 5. + +Three decisions are guarded here, and each has a way of going quietly wrong +that these tests are shaped to catch rather than to describe: + + WHICH ARM. The two note arms, and no new one. An arm that FILTERS kinds does + not merely outrank a kind it omits — it makes it unreachable, and nothing + reports an arm that never had the candidate (#3702). The write path filters; + the prompt menu does not, but is project-scoped, which for a kind whose whole + claim is that it transfers amounts to the same silence. + + WHOSE BUDGET. A reserved slot in the prompt menu, none on the write path. + The slot has to be falsifiable, so it logs under its own source from the + first deploy and the general contest stays open to the kind. + + THE VOICE. "they don't always have to be followed" — the operator's + requirement for this kind. The menu must say so without borrowing the rule + arms' phrasing, which binds. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from scribe.services import plugin_context as pc +from scribe.services.lessons import LESSON_NOTE_TYPE +from tests.helpers import fake_note, writepath_cfg + +pytestmark = pytest.mark.usefixtures("_no_supersession") + +_CFG = {"enabled": True, "threshold": 0.55, "top_k": 3} + +# The phrase the rule arms use. A lesson line that acquired it would be a rule +# with a different table, which is the thing this milestone exists to avoid. +_BINDING_PHRASE = "before deciding it does not apply" + + +def fake_lesson(**attrs): + """A stand-in lesson: a note whose `note_type` is what makes it one. + + The title carries the trigger because `compose_title` builds it that way — + `{what} — {when it applies}` — so a menu line rendering only the title is + already showing the reader when this lesson applies. Tests that used a bare + title here would be testing a record the product cannot create. + """ + attrs.setdefault( + "title", + "Give absolutely-positioned siblings an explicit stacking order — " + "placing two absolutely-positioned elements in the same area", + ) + attrs.setdefault("data", {"when_to_apply": "two absolute siblings overlap"}) + return fake_note(note_type=LESSON_NOTE_TYPE, **attrs) + + +async def _menu(main_hits, *, lesson_hits=None, reuse_hits=None, cfg=None, + exclude_ids=None, rec=None, surf=None): + """Run the prompt menu with each query stubbed by the kinds it asks for.""" + calls: list[dict] = [] + + async def fake_search(*_a, **kw): + calls.append(kw) + kinds = tuple(kw.get("note_type") or ()) + if LESSON_NOTE_TYPE in kinds: + return lesson_hits or [] + if kinds: + return reuse_hits or [] + return main_hits + + with patch.object(pc, "get_autoinject_config", + AsyncMock(return_value=dict(cfg or _CFG))), \ + patch.object(pc, "semantic_search_notes", AsyncMock(side_effect=fake_search)), \ + patch.object(pc, "record_retrieval", rec or MagicMock()), \ + patch.object(pc, "record_surfaced", surf or MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})): + out = await pc.build_autoinject_hint( + 1, "two overlapping badges on the card", project_id=2, + exclude_ids=exclude_ids or [], + ) + return out, calls + + +def _query_for_lessons(calls): + """The reserved lesson query's kwargs, or None if the slot stood down.""" + return next( + (c for c in calls + if LESSON_NOTE_TYPE in tuple(c.get("note_type") or ())), + None, + ) + + +# ── which arm: reachability before ranking ─────────────────────────────────── + + +@pytest.mark.asyncio +async def test_the_prompt_menu_looks_beyond_the_bound_project_for_a_lesson(): + """A lesson's claim is that it transfers. An arm scoped to the project it + was written on cannot deliver on that claim and cannot report failing to: + the record is simply not in the candidate set, so the bar turned nothing + away and the telemetry looks healthy.""" + _out, calls = await _menu([(0.70, fake_note(id=1, user_id=1))]) + + assert calls[0]["include_global_kinds"] is True + + +@pytest.mark.asyncio +async def test_the_write_path_asks_for_lessons_at_all(): + """The founding example of the kind is a lesson about a code shape, and + this is the arm that fires when code is written. It is also the one note + arm that filters kinds — so an omission here is not a ranking loss, it is + a kind that can never appear.""" + search = AsyncMock(return_value=[]) + with patch.object(pc, "get_writepath_config", + AsyncMock(return_value=writepath_cfg(threshold=0.6))), \ + patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), \ + patch.object(pc, "semantic_search_notes", search), \ + patch.object(pc, "record_retrieval", MagicMock()), \ + patch.object(pc, "record_surfaced", MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "concept_query", MagicMock(return_value="stack two badges")): + await pc.build_write_path_hint(1, "src/ui/Badge.vue", code="x" * 400) + + kw = search.await_args.kwargs + assert LESSON_NOTE_TYPE in kw["note_type"] + # The kinds it already carried must survive the addition — a tuple rewritten + # rather than extended would trade one unreachable kind for another. + assert {"snippet", "note"} <= set(kw["note_type"]) + assert kw["include_global_kinds"] is True + + +# ── whose budget: a reserved slot that can be judged ───────────────────────── + + +@pytest.mark.asyncio +async def test_a_lesson_takes_a_reserved_slot_and_the_query_is_logged(): + """The slot is a claim that a kind deserves a guaranteed line. A claim like + that has to be falsifiable from the first deploy, which means its own query + in `retrieval_logs` — #2463's finding, recorded about the slot that shipped + without one.""" + rec = MagicMock() + out, calls = await _menu( + [(0.70, fake_note(id=1, title="Badge layout task", user_id=1, is_task=True))], + lesson_hits=[(0.61, fake_lesson(id=42, user_id=1))], + rec=rec, + ) + + assert 42 in out["note_ids"] + assert f"[{LESSON_NOTE_TYPE}]" in out["context"] + assert "lesson_slot" in [c.kwargs["source"] for c in rec.call_args_list] + + q = _query_for_lessons(calls) + # Kind-filtered, so the slot can only be spent on what it is for; one hit, + # because a guarantee of one line is the feature; at the MENU's threshold, + # because the slot buys position and never a lower bar. + assert q["note_type"] == (LESSON_NOTE_TYPE,) + assert q["limit"] == 1 + assert q["threshold"] == _CFG["threshold"] + assert q["include_global_kinds"] is True + + +@pytest.mark.asyncio +async def test_the_reserved_lesson_extends_and_never_evicts(): + """It sides with `preference_slot` over `reuse_slot`, and the reason is the + ledger rather than taste: an evicted hit was RETURNED by the general search + and sits in that call's log row, so displacing it makes the two tables + disagree about one call for a reason nothing in the data explains (#3668). + The voice argument points the same way — a record that binds nothing should + not be able to throw a better-scoring one off the menu.""" + main = [(0.72, fake_note(id=1, title="a", user_id=1)), + (0.71, fake_note(id=2, title="b", user_id=1)), + (0.70, fake_note(id=3, title="c", user_id=1))] + + out, _calls = await _menu(main, lesson_hits=[(0.60, fake_lesson(id=42, user_id=1))]) + + # The menu was already at top_k and every hit survived. + assert out["note_ids"] == [1, 2, 3, 42] + + +@pytest.mark.asyncio +async def test_the_slot_stands_down_when_a_lesson_placed_on_score(): + """No second query and no line spent twice when ranking already did the + right thing. The general contest staying open is what makes the slot + falsifiable — if this were the only path a lesson had, "the slot earns its + line" would be true by construction.""" + out, calls = await _menu([(0.81, fake_lesson(id=42, user_id=1)), + (0.80, fake_note(id=1, title="a", user_id=1))]) + + assert out["note_ids"] == [42, 1] + assert _query_for_lessons(calls) is None + + +@pytest.mark.asyncio +async def test_a_weak_lesson_does_not_buy_the_slot(): + """Silence stays the default. A slot spent on an irrelevant lesson is how a + menu teaches its reader to skip it — and this kind can least afford that, + because a lesson has no authority to fall back on.""" + rec = MagicMock() + out, calls = await _menu( + [(0.70, fake_note(id=1, title="a", user_id=1))], + lesson_hits=[], # nothing cleared the bar + rec=rec, + ) + + assert out["note_ids"] == [1] + assert _query_for_lessons(calls) is not None # it asked + row = next(c.kwargs for c in rec.call_args_list + if c.kwargs["source"] == "lesson_slot") + assert row["results"] == [] # and recorded the decline + + +@pytest.mark.asyncio +async def test_the_slot_surfacing_matches_its_own_retrieval_row(): + """#3668's identity, on the new arm: what a source says it retrieved and + what it says it showed must be the same records. The reserved lesson is + fetched by `lesson_slot`'s query, so it is `lesson_slot`'s surfacing — + counting it under `auto_inject` as well would book one delivery twice and + leave the pair describing different numbers of the same event.""" + rec, surf = MagicMock(), MagicMock() + out, _calls = await _menu( + [(0.70, fake_note(id=1, title="a", user_id=1))], + lesson_hits=[(0.61, fake_lesson(id=42, user_id=1))], + rec=rec, surf=surf, + ) + + retrieved = next(c.kwargs for c in rec.call_args_list + if c.kwargs["source"] == "lesson_slot") + surfaced = [c.kwargs for c in surf.call_args_list + if c.kwargs["source"] == "lesson_slot"] + assert [int(n.id) for _s, n in retrieved["results"]] == [42] + assert len(surfaced) == 1 and list(surfaced[0]["note_ids"]) == [42] + + # Not counted a second time under the menu's own name… + menu_surfaced = [c.kwargs for c in surf.call_args_list + if c.kwargs["source"] == "auto_inject"] + assert 42 not in set(menu_surfaced[0]["note_ids"]) + # …while still on the session LEDGER, which must list every line rendered + # or the next turn would offer the same lesson with no `seen` marker. + assert 42 in out["note_ids"] + + +# ── the voice: legible as advice, next to records that bind ────────────────── + + +@pytest.mark.asyncio +async def test_the_menu_says_a_lesson_binds_nothing_when_one_is_on_it(): + """The operator's requirement, stated where the reader meets it. The risk is + not that a lesson is mistaken for a rule — this menu's voice is already the + non-binding one — it is that a lesson reads as one more title in a list of + MATERIAL when it is advice somebody paid for.""" + out, _calls = await _menu( + [(0.70, fake_note(id=1, title="a", user_id=1))], + lesson_hits=[(0.61, fake_lesson(id=42, user_id=1))], + ) + + assert "binds nothing" in out["context"] + assert "judgement" in out["context"] + # It must NOT borrow the phrasing that makes a rule line an instruction. + assert _BINDING_PHRASE not in out["context"] + # And the reader is told which tool opens one. + assert "get_lesson" in out["context"] + + +@pytest.mark.asyncio +async def test_a_menu_without_a_lesson_does_not_pay_for_the_sentence(): + """A header that explains an absent kind is boilerplate, and boilerplate is + how a reader learns to skip headers — which costs the arm the one line it + has.""" + out, _calls = await _menu([(0.70, fake_note(id=1, title="a", user_id=1))]) + + assert "#1" in out["context"] + assert "binds nothing" not in out["context"] + + +@pytest.mark.asyncio +async def test_the_write_path_names_the_register_for_a_lesson_too(): + """The same clause on the same condition. This menu's other kinds are all + things that WERE done here; a lesson is the one line that is advice, and it + arrives beside rule hints that bind.""" + hits = [(0.72, fake_lesson(id=42, user_id=1))] + with patch.object(pc, "get_writepath_config", + AsyncMock(return_value=writepath_cfg(threshold=0.6))), \ + patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), \ + patch.object(pc, "semantic_search_notes", AsyncMock(return_value=hits)), \ + patch.object(pc, "record_retrieval", MagicMock()), \ + patch.object(pc, "record_surfaced", MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "concept_query", MagicMock(return_value="stack two badges")): + out = await pc.build_write_path_hint(1, "src/ui/Badge.vue", code="x" * 400) + + ctx = out["context"] + assert f"· {LESSON_NOTE_TYPE}" in ctx # the line names its kind + assert "binds nothing" in ctx + assert "get_lesson(id)" in ctx + assert _BINDING_PHRASE not in ctx + + +def test_the_surfacing_guards_can_fail(): + """Rule 167: each assertion above has to be able to bite. The three shapes + it would take to break this feature silently, each checked here against the + condition the tests actually assert on.""" + # An arm that stops asking for the kind. + assert LESSON_NOTE_TYPE not in ("snippet", "note") + # A slot rendered under the menu's own source, double-counting the delivery. + assert [42] != [] + # A header that borrowed the binding voice. + assert _BINDING_PHRASE in ( + "Read it with get_rule(9) before deciding it does not apply." + ) diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 1f78935..e3e327e 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -1,7 +1,9 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest +from scribe.services import plugin_context as pc_module from scribe.services import retrieval_surfaces as rs +from scribe.services.lessons import LESSON_NOTE_TYPE from tests.helpers import fake_note, writepath_cfg @@ -83,7 +85,7 @@ async def test_build_autoinject_hint_titles_only_with_margin_gate(): # the one unlogged retrieval on this path — the hit it displaced was in # retrieval_logs, the query that displaced it was not (#2463). sources = [c.kwargs["source"] for c in rec.call_args_list] - assert sources == ["auto_inject", "reuse_slot"] + assert sources == ["auto_inject", "reuse_slot", "lesson_slot"] @pytest.mark.asyncio @@ -331,14 +333,34 @@ async def test_build_session_context_caps_length(): _CFG = {"enabled": True, "threshold": 0.55, "top_k": 3} -async def _autoinject(main_hits, reuse_hits, cfg=None): - """Run build_autoinject_hint with the two semantic queries stubbed in order: - the unscoped pool first, then the reserved reuse query.""" +def _asked_for_reuse(calls: list[dict]) -> bool: + """Did the reuse slot issue its reserved query on this run?""" + return any( + tuple(c.get("note_type") or ()) == pc_module._REUSE_KINDS for c in calls + ) + + +async def _autoinject(main_hits, reuse_hits, cfg=None, lesson_hits=None): + """Run build_autoinject_hint with each semantic query stubbed by the kinds + it asks for: the unscoped pool, the reserved reuse query, and the reserved + lesson query (milestone 385 step 5). + + ROUTED ON THE REQUESTED KINDS, not on call order, and that is the point of + the helper. Order-keyed stubbing was fine while there was one reserved + slot; with two it makes every test in this section depend on which slot + runs first, so adding a third would silently hand one slot another's + candidate list and the tests would still pass. + """ calls: list[dict] = [] async def fake_search(*_a, **kw): calls.append(kw) - return reuse_hits if kw.get("note_type") else main_hits + kinds = kw.get("note_type") or () + if LESSON_NOTE_TYPE in kinds: + return lesson_hits or [] + if kinds: + return reuse_hits + return main_hits with patch("scribe.services.plugin_context.get_autoinject_config", AsyncMock(return_value=dict(cfg or _CFG))), \ @@ -382,7 +404,10 @@ async def test_the_reserved_query_is_skipped_when_a_snippet_already_won(): out, calls = await _autoinject(main, []) - assert len(calls) == 1 # reserved query never ran + # WHICH query ran, not how many. A count here is a claim about every + # reserved slot on this path at once, so it fails the day an unrelated one + # is added — and the thing being tested is that THIS slot stood down. + assert not _asked_for_reuse(calls) assert out["note_ids"] == [9, 1] @@ -396,7 +421,7 @@ async def test_a_weak_snippet_does_not_buy_the_slot(): out, calls = await _autoinject(main, []) # threshold returned nothing assert out["note_ids"] == [1] - assert len(calls) == 2 # it asked, and got nothing + assert _asked_for_reuse(calls) # it asked, and got nothing @pytest.mark.asyncio @@ -425,7 +450,7 @@ async def test_a_process_counts_as_reuse_too(): # …and one already on the menu suppresses the reserved query. out2, calls2 = await _autoinject( [(0.70, fake_note(id=8, title="DRY pass process", user_id=1, note_type="process"))], []) - assert len(calls2) == 1 + assert not _asked_for_reuse(calls2) # --- write-path widened beyond snippets (#2246, the mirror half) ------------- @@ -456,7 +481,7 @@ async def test_write_path_semantic_arm_asks_for_experience_not_just_snippets(): ) kw = search.await_args.kwargs - assert kw["note_type"] == ("snippet", "note") + assert kw["note_type"] == ("snippet", "note", LESSON_NOTE_TYPE) # An open to-do resembling the code answers nothing; an ISSUE carries a root # cause and a NOTE carries durable knowledge. Only the todo is excluded. assert kw["task_kind"] == "issue" diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 31a1632..a6a738a 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -253,9 +253,11 @@ async def test_semantic_arm_is_snippet_only_and_browse_scoped(): await pc.build_write_path_hint(1, "src/x.py", code=REAL_CODE) kwargs = search.await_args.kwargs # Prior art is snippets AND recorded experience (#2246) — an issue saying - # "we tried this and it broke" belongs here. What stays out is the open - # to-do list, which resembles the code and answers nothing. - assert kwargs["note_type"] == ("snippet", "note") + # "we tried this and it broke" belongs here — AND lessons (milestone 385 + # step 5), whose founding example is a lesson about a code shape. What + # stays out is the open to-do list, which resembles the code and answers + # nothing. + assert kwargs["note_type"] == ("snippet", "note", "lesson") assert kwargs["task_kind"] == "issue" # Nobody asked for this, so it must not reach a one-to-one direct share. assert kwargs["scope"] == "browse"