fix(lessons): a lesson's name is one claim on one line (#4797)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 54s
CI & Build / Python tests (push) Successful in 1m48s
CI & Build / Build & push image (push) Successful in 28s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 54s
CI & Build / Python tests (push) Successful in 1m48s
CI & Build / Build & push image (push) Successful in 28s
#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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user