Merge pull request 'Rules never-opened advice per corpus (#4798); a lesson's name is one line (#4797)' (#200) from dev into main

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

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

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)

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,

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
 
 

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,
             ))

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

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)