From 9909cd245051bb3e61d6cd9b6a820d33e70f09af Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 3 Oct 2026 23:02:32 -0400 Subject: [PATCH 1/2] fix(retrieval): the rules never-opened warning stops sending the reader to a review tool that refuses rules (#4798) #4798 "The rules corpus's surfaced_never_pulled warning sends the reader to menus_to_review, which can only review auto_inject". #4772 gave the warning one remedy for both corpora: "judge a sample with menus_to_review". That is right for notes, where a line carries its passage. For rules it is a dead end: retrieval_review.REVIEWABLE holds only auto_inject, so the tool refuses every rule arm. - The reading is now per corpus (_NEVER_PULLED_READING). For rules, the text says: - the count includes rules that only arrived in a listing; - a rule can rightly be set aside on its trigger alone; - no judged sample exists for the rule arms; - a rule set aside again and again is a trigger to fix (update_rule when_to_apply, then what_might_apply), not a floor. - The tool docstring says the same. - The guard is tied to REVIEWABLE, so it can fail in both directions: if the rules text names menus_to_review while no rule arm is reviewable, or if a rule arm becomes reviewable and the text still says there is no sample. Co-Authored-By: Claude Opus 5.5 --- src/scribe/mcp/tools/search.py | 2 ++ src/scribe/services/retrieval_telemetry.py | 34 ++++++++++++++++++---- tests/test_retrieval_warnings.py | 23 +++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 682d24ea..8487dae4 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -624,6 +624,8 @@ It is an UPPER BOUND per surface: a pull records the door it came corpus. Not a verdict on them: a line carries its matched passage, so an unopened record may have been unrelated, enough as shown, or already in context. Judge a sample (`menus_to_review`) before acting on it. + For RULES there is no judged sample: a rule set aside again and again + is a trigger to fix (`update_rule(when_to_apply=...)`), not a floor. - `read_and_unacted` — distinct rules OPENED in the window that recorded no outcome, against the ones that did. The failure milestone 419 was opened on, and the worse sibling of `surfaced_never_pulled` above: a diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index 60d269d3..59eb89f0 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -492,6 +492,34 @@ def _num(raw: str, fallback): return fallback +# What "surfaced and never opened" can and cannot mean, per corpus, and where +# to look next. ONE SENTENCE PER CORPUS because the two lines are built +# differently (#4798): a note line carries its matched passage, so an unopened +# note may have done its job, and the judged sample is the instrument that can +# tell. A rule line carries only its trigger and asks to be opened, and no +# judged sample covers the rule arms (`retrieval_review.REVIEWABLE`), so +# sending that reader to `menus_to_review` sent them to a refusal. +_NEVER_PULLED_READING = { + "notes": ( + "That is not a verdict on them: a line carries its matched passage, " + "so an unopened record may have been unrelated, enough as shown, or " + "already in context. Before acting on it — a title, a floor, a " + "budget — judge a sample with `menus_to_review` and read the " + "`judged` block." + ), + "rules": ( + "Not a verdict either, and read differently from notes: the count " + "includes rules that only arrived in a listing (the project " + "handshake, a planning read), and a rule line shows its trigger, so a " + "session can rightly set one aside on the trigger alone. There is no " + "judged sample for the rule arms. A rule set aside again and again " + "where it does not apply is a trigger to fix, not a floor to move — " + "`update_rule(when_to_apply=...)`, then `what_might_apply` with the " + "moment's own words to see where it ranks." + ), +} + + def _warn(code, detail, source=None, **numbers) -> dict: """One finding, carrying the numbers that produced it. @@ -767,11 +795,7 @@ def _compute_warnings(sources: dict, usage: dict, rule_usage: dict, out.append(_warn( "surfaced_never_pulled", f"{never} of {shown} distinct {label} were surfaced in this " - f"window and never opened. That is not a verdict on them: a " - f"line carries its matched passage, so an unopened record may " - f"have been unrelated, enough as shown, or already in context. " - f"Before acting on it — a title, a floor, a budget — judge a " - f"sample with `menus_to_review` and read the `judged` block.", + f"window and never opened. {_NEVER_PULLED_READING[label]}", source=None, corpus=label, surfaced=int(shown), pulled=int(pulled or 0), never_pulled=never, )) diff --git a/tests/test_retrieval_warnings.py b/tests/test_retrieval_warnings.py index 0bb5f5ab..2f2e36bf 100644 --- a/tests/test_retrieval_warnings.py +++ b/tests/test_retrieval_warnings.py @@ -182,6 +182,29 @@ def test_records_shown_and_never_opened_are_reported_per_corpus() -> None: assert found["rules"]["never_pulled"] == 58 +def test_each_corpus_is_sent_only_to_a_tool_that_takes_it() -> None: + """#4798: one remedy for both corpora sent the rules reader to + `menus_to_review`, which refuses every source but the notes menu. Tied to + `REVIEWABLE` itself, so it goes red in either direction: the rules text + naming the tool while no rule arm is reviewable, or a rule arm becoming + reviewable while the text still says there is no sample.""" + from scribe.services.retrieval_review import REVIEWABLE + + ws = warn( + {}, + usage={"distinct_notes_surfaced": 9, "distinct_notes_pulled": 1}, + rule_usage={"distinct_rules_surfaced": 9, "distinct_rules_pulled": 1}, + ) + detail = {w["numbers"]["corpus"]: w["detail"] + for w in ws if w["code"] == "surfaced_never_pulled"} + assert set(detail) == {"notes", "rules"} + assert "auto_inject" in REVIEWABLE + assert "menus_to_review" in detail["notes"] + rule_arms = {"prompt_rule", "pre_tool_rule", "write_path_rule"} + assert ("menus_to_review" in detail["rules"]) == bool(rule_arms & set(REVIEWABLE)) + assert "when_to_apply" in detail["rules"], "the rules text names no next step" + + def test_everything_opened_reports_nothing() -> None: ws = warn({}, usage={"distinct_notes_surfaced": 5, "distinct_notes_pulled": 5}) assert "surfaced_never_pulled" not in codes(ws) From d2ac7bf2202e87c79a89b8413feb46aeac4d6aab Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 3 Oct 2026 23:04:53 -0400 Subject: [PATCH 2/2] fix(lessons): a lesson's name is one claim on one line (#4797) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4797 "A lesson's what takes a whole narrative, and the narrative becomes its name". `what` is the title every listing and menu prints. Nothing enforced its documented "one line", so lessons written with the incident in `what` printed up to ~1,500 characters as a menu line, buried the claim, and diluted the trigger in the embedded title. That was 14 of the 39 lessons on this install. - lessons.require_claim refuses a `what` over WHAT_MAX_CHARS (240) or running over several lines. The refusal says the story goes in `insight`. - Both doors call it before writing: - MCP create_lesson / update_lesson; - REST create / update. An update checks only a NEW name, so a lesson stored with a long one can still take the edit that repairs it. - lessons.claim_line is the display half. _menu_name shows an over-long stored name as its first sentence marked " …", because a door guard does not undo rows already stored and an unrepaired install would keep printing them. - The tool docstrings state the bound. Co-Authored-By: Claude Opus 5.5 --- src/scribe/mcp/tools/lessons.py | 12 ++- src/scribe/routes/lessons.py | 6 ++ src/scribe/services/lessons.py | 53 +++++++++++ src/scribe/services/plugin_context.py | 5 +- tests/test_lesson_claim.py | 125 ++++++++++++++++++++++++++ 5 files changed, 197 insertions(+), 4 deletions(-) create mode 100644 tests/test_lesson_claim.py diff --git a/src/scribe/mcp/tools/lessons.py b/src/scribe/mcp/tools/lessons.py index 2f72a2d5..eaa3471e 100644 --- a/src/scribe/mcp/tools/lessons.py +++ b/src/scribe/mcp/tools/lessons.py @@ -167,7 +167,9 @@ async def create_lesson( Args: what: The insight in one line — the claim itself, as you would say it. - This becomes the title, joined with the trigger. + This becomes the title, joined with the trigger. At most 240 + characters and no line breaks; a longer one is refused — the + story goes in `insight`. when_to_apply: The situation this applies in, as a symptom. Required. insight: The body — what to do, and the incident that taught it. Write the story here for the reader; it costs the ranking nothing, @@ -208,6 +210,7 @@ async def create_lesson( "record saves, reads correctly, and never surfaces." ) + lessons_svc.require_claim(what) sources = lessons_svc.normalize_sources(learned_from) # Validated before anything is written, so a lesson naming a rule the # caller cannot read fails whole rather than saving half-linked. @@ -334,7 +337,8 @@ async def update_lesson( Args: lesson_id: Lesson to update. - what: New one-line claim. Empty leaves unchanged. + what: New one-line claim, at most 240 characters. Empty leaves + unchanged. when_to_apply: New trigger, as a symptom. Empty leaves unchanged. insight: New body. Empty leaves unchanged. learned_from: Replace the source ids. None leaves unchanged; pass the @@ -362,6 +366,10 @@ async def update_lesson( `convergence`: the group, and the rule it may be missing. """ uid = current_user_id() + # Only a NEW name is checked: a lesson stored with a long one must still + # take a new trigger or a rule link — and the fix for it is this call. + if what: + lessons_svc.require_claim(what) linked = ( await lesson_rules_svc.require_rules(uid, rule_ids) if rule_ids is not None else None diff --git a/src/scribe/routes/lessons.py b/src/scribe/routes/lessons.py index f4f1e0b9..f3f11b5f 100644 --- a/src/scribe/routes/lessons.py +++ b/src/scribe/routes/lessons.py @@ -156,6 +156,10 @@ async def create_lesson_route(): when_to_apply = (data.get("when_to_apply") or "").strip() if not what: return jsonify({"error": "what is required"}), 400 + try: + lessons_svc.require_claim(what) + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 # The trigger is not optional at this door even though the service will # store a lesson without one. A lesson with no trigger saves, reads # correctly in every listing, and never surfaces — there is nothing to @@ -300,6 +304,8 @@ async def update_lesson_route(lesson_id: int): linked = None no_rule = (data.get("no_rule") or "").strip() try: + if kwargs.get("what"): + lessons_svc.require_claim(kwargs["what"]) if data.get("rule_ids") is not None: linked = await lesson_rules_svc.require_rules(uid, data["rule_ids"]) lesson_rules_svc.require_one_answer(linked, no_rule) diff --git a/src/scribe/services/lessons.py b/src/scribe/services/lessons.py index 9a5a5bd7..a2bda83f 100644 --- a/src/scribe/services/lessons.py +++ b/src/scribe/services/lessons.py @@ -255,6 +255,59 @@ def compose_body( return "\n\n".join(lines) +# A lesson's NAME is its claim, and the claim is one line (#4797). `what` is +# the title every listing, menu and search row prints, so a story written there +# instead of in `insight` turns each of those lines into a document — the menu +# line for one such lesson ran to 1,416 characters, and the claim it existed to +# transfer sat somewhere in the middle. It also ranks badly: the embedded title +# is `what — when_to_apply`, so a narrative crowds out the trigger. +# +# 240 is a long sentence. The bound is on what a NAME can be, not on what a +# lesson can say — `insight` is the body, unbounded, chunked like any record, +# and costs the ranking nothing. +WHAT_MAX_CHARS = 240 + + +def require_claim(what: str | None) -> None: + """Refuse a `what` that is not one line — before anything is written. + + A refusal rather than a cut, because a cut name reads as complete: the + writer would never learn the story went to the wrong field, and the + reader would get its first 240 characters as though they were the claim. + """ + text = (what or "").strip() + multiline = "\n" in text + if not multiline and len(text) <= WHAT_MAX_CHARS: + return + shape = "runs over several lines" if multiline else f"is {len(text)} characters" + raise ValueError( + f"`what` {shape}; it is the lesson's NAME — one claim, one line, at " + f"most {WHAT_MAX_CHARS} characters — and every listing and menu prints " + "it whole. Nothing was written. Put the claim in `what`, said as the " + "sentence someone should come away with, and the incident that taught " + "it in `insight`, which is the body and is not bounded." + ) + + +def claim_line(what: str | None) -> str: + """A stored name as one menu line, whatever was stored. + + The display half of `require_claim`, for names written before it existed: + a guard at the door does not undo a value already stored, and on an + install nobody has repaired, those rows would still print their whole + story on every menu. Over the bound, the name's first sentence is shown + and marked as cut — the record itself is unchanged, and opening it shows + the rest. + """ + text = " ".join((what or "").split()) + if len(text) <= WHAT_MAX_CHARS: + return text + first = re.split(r"(?<=[.!?])\s", text, maxsplit=1)[0] + if len(first) > WHAT_MAX_CHARS: + first = first[:WHAT_MAX_CHARS].rsplit(" ", 1)[0] + return first + " …" + + def lesson_document( what: str, when_to_apply: str = "", insight: str = "", learned_from: list[int] | None = None, diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 8081f8d9..660aec85 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -35,7 +35,7 @@ from scribe.services.embeddings import ( semantic_search_rules, ) from scribe.services import lesson_rules as lesson_rules_svc -from scribe.services.lessons import LESSON_NOTE_TYPE +from scribe.services.lessons import LESSON_NOTE_TYPE, claim_line from scribe.services.note_usage import record_surfaced from scribe.services.rule_usage import record_rule_surfaced from scribe.services.supersession import superseded_ids @@ -89,7 +89,8 @@ def _menu_name(title: str | None, note_type: str | None, data=None, body: str | from scribe.services.embeddings import untrigger_title from scribe.services.lessons import lesson_trigger trigger = lesson_trigger(SimpleNamespace(data=data, body=body or "")) - return untrigger_title(title, trigger).strip() or title + # One line even when the stored name is a story (#4797). + return claim_line(untrigger_title(title, trigger).strip() or title) return title diff --git a/tests/test_lesson_claim.py b/tests/test_lesson_claim.py new file mode 100644 index 00000000..73e093b0 --- /dev/null +++ b/tests/test_lesson_claim.py @@ -0,0 +1,125 @@ +"""A lesson's name is one claim on one line (#4797). + +`what` is the title every listing and menu prints. Written as the incident +instead of the claim, it turned a menu line into 1,416 characters with the +claim somewhere in the middle. Two halves are pinned: the doors refuse a new +one, and the menu shows one line for a name stored before they did. +""" +from __future__ import annotations + +import inspect +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.mcp._context import _user_id_ctx +from scribe.mcp.tools.lessons import create_lesson, update_lesson +from scribe.services import lessons as lessons_svc +from scribe.services.lessons import WHAT_MAX_CHARS, claim_line, require_claim + +TRIGGER = "an existing call is about to double as a liveness signal" +CLAIM = ("A check-in piggybacked on an existing call inherits every backoff " + "that call is ever given") +STORY = ( + "The operator reported their agent showing offline while it ran. " + + "The lease poll backed off to 900s while the roster called 300s stopped. " * 6 +).strip() + + +# ── The bound ────────────────────────────────────────────────────────────── + +def test_a_one_line_claim_passes(): + require_claim(CLAIM) + require_claim("x" * WHAT_MAX_CHARS) + + +@pytest.mark.parametrize("what", [ + "x" * (WHAT_MAX_CHARS + 1), + "A claim.\n\nAnd then the story of how it was learned.", +], ids=["over the bound", "several lines"]) +def test_a_story_in_the_name_is_refused_with_where_it_goes(what): + with pytest.raises(ValueError) as err: + require_claim(what) + message = str(err.value) + assert "Nothing was written" in message + assert "insight" in message, "the refusal must say where the story belongs" + + +# ── The doors ────────────────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_the_mcp_create_refuses_before_writing(): + _user_id_ctx.set(7) + created = AsyncMock() + with patch.object(lessons_svc, "create_lesson", created): + with pytest.raises(ValueError): + await create_lesson(what=STORY, when_to_apply=TRIGGER) + created.assert_not_called() + + +@pytest.mark.asyncio +async def test_the_mcp_update_refuses_a_new_long_name(): + _user_id_ctx.set(7) + updated = AsyncMock() + with patch.object(lessons_svc, "update_lesson", updated): + with pytest.raises(ValueError): + await update_lesson(lesson_id=1, what=STORY) + updated.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_lesson_stored_with_a_long_name_can_still_be_edited(): + """Only a NEW name is checked. Refusing every edit of a lesson stored + before the bound would also refuse the edit that repairs it.""" + _user_id_ctx.set(7) + note = SimpleNamespace( + id=1, title=STORY, body="b", tags=[], project_id=None, + note_type="lesson", data={"what": STORY}, arose_from_id=None, + created_at=None, updated_at=None, + ) + updated = AsyncMock(return_value=note) + with patch.object(lessons_svc, "update_lesson", updated), \ + patch("scribe.mcp.tools.lessons.systems_tools.attach_systems", AsyncMock()), \ + patch("scribe.mcp.tools.lessons.lesson_rules_svc.attach_lesson_rules", AsyncMock()): + await update_lesson(lesson_id=1, when_to_apply=TRIGGER) + assert updated.await_args.kwargs["what"] is None + + +@pytest.mark.parametrize("handler", ["create_lesson_route", "update_lesson_route"]) +def test_the_rest_door_refuses_too(handler): + """The web form writes lessons as well, and a bound one door enforces is + a bound the other door walks around.""" + from scribe.routes import lessons as routes + + src = inspect.getsource(getattr(routes, handler)) + write = "lessons_svc." + handler.removesuffix("_route") + "(" + assert "require_claim(" in src + assert src.index("require_claim(") < src.index(write), \ + "the check runs after the write it exists to prevent" + + +# ── The display ──────────────────────────────────────────────────────────── + +def test_a_short_name_is_shown_as_it_is(): + assert claim_line(CLAIM) == CLAIM + + +def test_a_stored_story_shows_as_its_first_sentence_marked_cut(): + line = claim_line(STORY) + assert line == "The operator reported their agent showing offline while it ran. …" + assert "\n" not in line + + +def test_a_first_sentence_past_the_bound_is_cut_at_a_word(): + line = claim_line("word " * 100) + assert line.endswith(" …") + assert len(line) <= WHAT_MAX_CHARS + 2 + + +def test_the_menu_names_a_long_lesson_in_one_line(): + from scribe.services.plugin_context import _menu_name + + name = _menu_name(STORY, "lesson", {"what": STORY, "when_to_apply": TRIGGER}, "") + assert name.endswith(" …") + assert len(name) <= WHAT_MAX_CHARS + 2