feat(lessons): judged when written — a new lesson is offered its rules, and "no rule fits" is an answer (milestone 440 step 2, #4631)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / integration (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m15s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / integration (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m15s
CI & Build / Build & push image (push) Skipped
"Which rule is this lesson an instance of?" now has three recorded answers: a rule named (a confirmed link, #4630), no rule fits (new), or unjudged. - Model + migration 0112: lesson_no_rule (lesson_id PK, CASCADE from the note; why; judged_at). A table rather than a key in notes.data, because that mirror is re-composed from the body on every edit and would erase it. - Service (lesson_rules): set_no_rule rejects any confirmed link with the reason; a confirmation (set_lesson_rules or judge_link) deletes the answer; require_one_answer refuses both answers in one call before any write; judgments_for_lessons + attach_lesson_rules add rule_judgment (and no_rule) to every lesson payload; list_unjudged lists the open ones; rule_candidates searches rules with the lesson's claim + trigger at the explicit-search bar, None when the search could not run. - MCP: create_lesson/update_lesson take no_rule; an unanswered create returns rule_candidates, rule_judgment and a rule_hint; list_lessons(unjudged=true). - REST: the same on POST/PATCH /api/lessons and GET ?unjudged=1; create returns rule_candidates. - Backup v19: a lesson_no_rule section, export (full and per-user) and import. - Guidance: create_lesson docstring, writing-records.md in using-scribe (owner, pinned in test_guidance_ownership), create_rule docstring on linking the lessons a new rule governs. Plugin version minted. - Tests: door units, integration for the three states, the rejection reason, scoping, cascade; backup registries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+6
-1
@@ -152,6 +152,10 @@ def _no_lesson_rule_links(request):
|
||||
Both decorations are fail-open, so the cost would be a slow failed connect
|
||||
per test rather than a failure, which is worse: it would never be noticed.
|
||||
|
||||
The candidate search a new lesson runs (#4631) is stubbed to "nothing
|
||||
resembles it" for the same reason: it embeds, and the model load would
|
||||
otherwise ride along with every unit test that creates a lesson.
|
||||
|
||||
Skipped for integration tests, which exercise the real links against
|
||||
Postgres (tests/test_integration_lesson_rule_links.py).
|
||||
"""
|
||||
@@ -159,7 +163,8 @@ def _no_lesson_rule_links(request):
|
||||
yield
|
||||
return
|
||||
with patch("scribe.services.lesson_rules.attach_lesson_rules", AsyncMock()), \
|
||||
patch("scribe.services.lesson_rules.attach_rule_lessons", AsyncMock()):
|
||||
patch("scribe.services.lesson_rules.attach_rule_lessons", AsyncMock()), \
|
||||
patch("scribe.services.lesson_rules.rule_candidates", AsyncMock(return_value=[])):
|
||||
yield
|
||||
|
||||
|
||||
|
||||
@@ -145,6 +145,15 @@ TOPICS: tuple[Topic, ...] = (
|
||||
# one fires mid-work, the moment a lesson arrives in a situation it
|
||||
# actually names. `lessons inform` already points at the kind.
|
||||
shared_with=("docstrings",)),
|
||||
Topic("a lesson names the rule it is an instance of", U,
|
||||
("rule_ids", "no_rule", "unjudged=true"),
|
||||
"a rule about the same subsystem that governs a different moment "
|
||||
"is not its rule",
|
||||
# Mid-work, like the topic above: it fires when a lesson is written,
|
||||
# so no index marker. create_lesson's docstring states the two
|
||||
# answers at the moment they are given, and may — tool contracts
|
||||
# are not copy-scanned.
|
||||
shared_with=("docstrings",)),
|
||||
Topic("preferences shape how work is done, never what is recorded", U,
|
||||
("never what gets recorded",),
|
||||
"a preference never makes a task into a note"),
|
||||
|
||||
@@ -160,3 +160,84 @@ async def test_one_row_per_pair(world):
|
||||
])
|
||||
with pytest.raises(IntegrityError):
|
||||
await s.commit()
|
||||
|
||||
|
||||
# ── #4631: "no rule fits" and the unjudged list ─────────────────────────────
|
||||
|
||||
|
||||
async def _judgment(lesson_id: int) -> dict:
|
||||
return (await links_svc.judgments_for_lessons([lesson_id]))[lesson_id]
|
||||
|
||||
|
||||
async def _unjudged_ids(user_id: int) -> set[int]:
|
||||
items, _total = await links_svc.list_unjudged(user_id, limit=100)
|
||||
return {i["id"] for i in items}
|
||||
|
||||
|
||||
async def test_a_new_lesson_is_unjudged_until_it_is_answered_either_way(world):
|
||||
owner, lesson = world["owner"], world["lesson"]
|
||||
assert (await _judgment(lesson))["rule_judgment"] == "unjudged"
|
||||
assert lesson in await _unjudged_ids(owner)
|
||||
|
||||
await links_svc.set_no_rule(owner, lesson, "specific to this CI host")
|
||||
judged = await _judgment(lesson)
|
||||
assert judged["rule_judgment"] == "no_rule"
|
||||
assert judged["no_rule"]["why"] == "specific to this CI host"
|
||||
assert lesson not in await _unjudged_ids(owner)
|
||||
|
||||
# Naming a rule overturns the answer — both cannot stand.
|
||||
await links_svc.set_lesson_rules(owner, lesson, [world["r1"]])
|
||||
judged = await _judgment(lesson)
|
||||
assert judged == {"rule_judgment": "linked"}
|
||||
assert lesson not in await _unjudged_ids(owner)
|
||||
|
||||
|
||||
async def test_no_rule_rejects_a_confirmed_link_with_its_reason(world):
|
||||
owner, lesson = world["owner"], world["lesson"]
|
||||
await links_svc.set_lesson_rules(owner, lesson, [world["r1"]])
|
||||
await links_svc.set_no_rule(owner, lesson, "the log was not the issue")
|
||||
|
||||
assert await _states(lesson) == {world["r1"]: "rejected"}
|
||||
rules = (await links_svc.rules_for_lessons(owner, [lesson]))[lesson]
|
||||
assert rules[0]["note"].endswith("the log was not the issue")
|
||||
assert (await _judgment(lesson))["rule_judgment"] == "no_rule"
|
||||
|
||||
|
||||
async def test_a_confirmation_by_judgment_also_clears_no_rule(world):
|
||||
owner, lesson = world["owner"], world["lesson"]
|
||||
await links_svc.set_no_rule(owner, lesson, "nothing yet")
|
||||
await links_svc.judge_link(owner, lesson, world["r2"], "confirm")
|
||||
assert (await _judgment(lesson))["rule_judgment"] == "linked"
|
||||
|
||||
|
||||
async def test_a_rejection_alone_leaves_the_lesson_unjudged(world):
|
||||
""""Not that rule" says nothing about whether another one fits."""
|
||||
owner, lesson = world["owner"], world["lesson"]
|
||||
await links_svc.judge_link(owner, lesson, world["r1"], "reject", "different failure")
|
||||
assert (await _judgment(lesson))["rule_judgment"] == "unjudged"
|
||||
assert lesson in await _unjudged_ids(owner)
|
||||
|
||||
|
||||
async def test_no_rule_needs_a_reason_and_a_writer(world):
|
||||
with pytest.raises(ValueError):
|
||||
await links_svc.set_no_rule(world["owner"], world["lesson"], " ")
|
||||
with pytest.raises((ValueError, PermissionError)):
|
||||
await links_svc.set_no_rule(world["stranger"], world["lesson"], "mine now")
|
||||
assert (await _judgment(world["lesson"]))["rule_judgment"] == "unjudged"
|
||||
|
||||
|
||||
async def test_the_unjudged_list_is_scoped_to_the_reader(world):
|
||||
assert world["lesson"] not in await _unjudged_ids(world["stranger"])
|
||||
|
||||
|
||||
async def test_deleting_the_lesson_takes_its_answer_with_it(world):
|
||||
from scribe.models.lesson_rule_link import LessonNoRule
|
||||
|
||||
await links_svc.set_no_rule(world["owner"], world["lesson"], "alone")
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(Note).where(Note.id == world["lesson"]))
|
||||
await s.commit()
|
||||
left = (await s.execute(
|
||||
select(LessonNoRule).where(LessonNoRule.lesson_id == world["lesson"])
|
||||
)).scalars().all()
|
||||
assert left == []
|
||||
|
||||
@@ -143,3 +143,123 @@ def test_backup_keeps_an_unjudged_link_unjudged():
|
||||
)
|
||||
assert (built.lesson_id, built.rule_id, built.state) == (110, 120, "suggested")
|
||||
assert built.judged_at is None
|
||||
|
||||
|
||||
# ── #4631: judged when written ───────────────────────────────────────────────
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_unanswered_lesson_comes_back_with_rules_to_judge_against():
|
||||
"""Neither answer given: the create offers the rules the lesson resembles,
|
||||
searched with its claim and trigger and scoped to its project."""
|
||||
_user_id_ctx.set(7)
|
||||
offered = [{"id": 5, "title": "Read the log", "kind": "rule",
|
||||
"when_to_apply": "a run overran", "score": 0.61}]
|
||||
search = AsyncMock(return_value=offered)
|
||||
p1, p2, p3 = _create_patches(AsyncMock(return_value=_stub_note()))
|
||||
with p1, p2, p3, patch.object(links_svc, "rule_candidates", search):
|
||||
out = await lesson_tools.create_lesson(what="x", when_to_apply=TRIGGER, project_id=2)
|
||||
assert search.await_args.args == (7, "x", TRIGGER, 2)
|
||||
assert out["rule_candidates"] == offered
|
||||
assert out["rule_judgment"] == links_svc.UNJUDGED
|
||||
assert "no_rule" in out["rule_hint"] and "rule_ids" in out["rule_hint"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_search_that_could_not_run_offers_no_list_rather_than_an_empty_one():
|
||||
"""None means unavailable; an empty list would claim nothing resembles it."""
|
||||
_user_id_ctx.set(7)
|
||||
p1, p2, p3 = _create_patches(AsyncMock(return_value=_stub_note()))
|
||||
with p1, p2, p3, patch.object(links_svc, "rule_candidates", AsyncMock(return_value=None)):
|
||||
out = await lesson_tools.create_lesson(what="x", when_to_apply=TRIGGER)
|
||||
assert "rule_candidates" not in out
|
||||
assert out["rule_judgment"] == links_svc.UNJUDGED
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_rule_on_create_records_the_answer_and_offers_nothing():
|
||||
_user_id_ctx.set(7)
|
||||
answered = AsyncMock()
|
||||
search = AsyncMock(return_value=[])
|
||||
p1, p2, p3 = _create_patches(AsyncMock(return_value=_stub_note(id=41)))
|
||||
with p1, p2, p3, patch.object(links_svc, "set_no_rule", answered), \
|
||||
patch.object(links_svc, "rule_candidates", search):
|
||||
out = await lesson_tools.create_lesson(
|
||||
what="x", when_to_apply=TRIGGER, no_rule="a one-off of this CI host",
|
||||
)
|
||||
assert answered.await_args.args[1:] == (41, "a one-off of this CI host")
|
||||
search.assert_not_awaited()
|
||||
assert "rule_candidates" not in out
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rules_and_no_rule_together_write_nothing():
|
||||
_user_id_ctx.set(7)
|
||||
created = AsyncMock(return_value=_stub_note())
|
||||
p1, p2, p3 = _create_patches(created)
|
||||
with p1, p2, p3, patch.object(links_svc, "require_rules", AsyncMock(return_value=[5])):
|
||||
with pytest.raises(ValueError, match="no_rule"):
|
||||
await lesson_tools.create_lesson(
|
||||
what="x", when_to_apply=TRIGGER, rule_ids=[5], no_rule="none fits",
|
||||
)
|
||||
created.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_records_no_rule_and_refuses_it_beside_named_rules():
|
||||
_user_id_ctx.set(7)
|
||||
answered = AsyncMock()
|
||||
updated = AsyncMock(return_value=_stub_note())
|
||||
with patch.object(lessons_svc, "update_lesson", updated), \
|
||||
patch.object(links_svc, "set_no_rule", answered):
|
||||
await lesson_tools.update_lesson(lesson_id=41, no_rule="stands alone")
|
||||
assert answered.await_args.args[1:] == (41, "stands alone")
|
||||
|
||||
updated.reset_mock()
|
||||
with patch.object(lessons_svc, "update_lesson", updated), \
|
||||
patch.object(links_svc, "require_rules", AsyncMock(return_value=[5])):
|
||||
with pytest.raises(ValueError):
|
||||
await lesson_tools.update_lesson(lesson_id=41, rule_ids=[5], no_rule="none")
|
||||
updated.assert_not_awaited()
|
||||
|
||||
|
||||
def test_an_empty_rule_list_beside_no_rule_is_one_answer_not_two():
|
||||
"""`rule_ids=[]` clears; it does not name a rule, so it cannot contradict."""
|
||||
links_svc.require_one_answer([], "none fits")
|
||||
links_svc.require_one_answer(None, "none fits")
|
||||
links_svc.require_one_answer([5], "")
|
||||
with pytest.raises(ValueError):
|
||||
links_svc.require_one_answer([5], "none fits")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_unjudged_is_a_listing_and_routes_to_its_own_query():
|
||||
_user_id_ctx.set(7)
|
||||
listed = AsyncMock(return_value=([], 0))
|
||||
with patch.object(links_svc, "list_unjudged", listed), \
|
||||
patch("scribe.mcp.tools.lessons.access_svc.label_shared_items", AsyncMock(return_value=[])), \
|
||||
patch("scribe.mcp.tools.lessons.attach_usage", AsyncMock()):
|
||||
out = await lesson_tools.list_lessons(unjudged=True, tag="ci", project_id=2)
|
||||
assert out == {"lessons": [], "total": 0}
|
||||
assert listed.await_args.kwargs["tag"] == "ci"
|
||||
assert listed.await_args.kwargs["project_id"] == 2
|
||||
with pytest.raises(ValueError):
|
||||
await lesson_tools.list_lessons(unjudged=True, q="log")
|
||||
|
||||
|
||||
def test_backup_skips_a_no_rule_answer_whose_lesson_did_not_restore():
|
||||
maps = backup._Maps()
|
||||
assert backup._build_lesson_no_rule({"lesson_id": 10, "why": "alone"}, maps) is None
|
||||
maps.notes[10] = 110
|
||||
built = backup._build_lesson_no_rule({"lesson_id": 10, "why": "alone"}, maps)
|
||||
assert (built.lesson_id, built.why) == (110, "alone")
|
||||
# An answer with no reason is not an answer; it restores as unjudged.
|
||||
assert backup._build_lesson_no_rule({"lesson_id": 10, "why": " "}, maps) is None
|
||||
|
||||
|
||||
def test_the_no_rule_migration_follows_the_link_migration():
|
||||
path = Path(__file__).resolve().parents[1] / "alembic" / "versions" / "0112_lesson_no_rule.py"
|
||||
spec = importlib.util.spec_from_file_location("m0112", path)
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(module)
|
||||
assert (module.revision, module.down_revision) == ("0112", "0111")
|
||||
|
||||
@@ -27,7 +27,7 @@ def test_backup_version_is_current():
|
||||
|
||||
(Named for the number it asserted until v10, which is exactly the drift a
|
||||
name-carrying-a-value invites; it now says what it checks.)"""
|
||||
assert backup.BACKUP_VERSION == 18
|
||||
assert backup.BACKUP_VERSION == 19
|
||||
|
||||
|
||||
def _exportable_note(**over):
|
||||
@@ -131,7 +131,7 @@ def test_a_repo_binding_carries_the_branch_its_ledger_follows():
|
||||
def _column_guard_targets():
|
||||
from scribe.models.canonical_system import CanonicalSystem
|
||||
from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse
|
||||
from scribe.models.lesson_rule_link import LessonRuleLink
|
||||
from scribe.models.lesson_rule_link import LessonNoRule, LessonRuleLink
|
||||
from scribe.models.design_system import DesignSystem, DesignToken
|
||||
from scribe.models.milestone import Milestone
|
||||
from scribe.models.note import Note
|
||||
@@ -169,6 +169,7 @@ def _column_guard_targets():
|
||||
"note_supersessions": (NoteSupersession, backup._note_supersession_rows),
|
||||
"rule_relations": (RuleRelation, backup._rule_relation_rows),
|
||||
"lesson_rule_links": (LessonRuleLink, backup._lesson_rule_link_rows),
|
||||
"lesson_no_rule": (LessonNoRule, backup._lesson_no_rule_rows),
|
||||
"note_usage_events": (NoteUsageEvent, backup._usage_event_rows),
|
||||
"rule_usage_events": (RuleUsageEvent, backup._rule_usage_event_rows),
|
||||
"retrieval_tuning_events": (
|
||||
@@ -286,6 +287,7 @@ def _import_guard_targets():
|
||||
"note_supersessions": backup._build_note_supersession,
|
||||
"rule_relations": backup._build_rule_relation,
|
||||
"lesson_rule_links": backup._build_lesson_rule_link,
|
||||
"lesson_no_rule": backup._build_lesson_no_rule,
|
||||
"note_usage_events": backup._build_usage_event,
|
||||
"rule_usage_events": backup._build_rule_usage_event,
|
||||
"retrieval_tuning_events": backup._build_retrieval_tuning_event,
|
||||
@@ -539,7 +541,9 @@ async def test_export_full_backup_contains_every_declared_section():
|
||||
# v16: the reasons beside the settings they explain.
|
||||
"retrieval_tuning_events",
|
||||
# v18: which rule each lesson is an instance of.
|
||||
"lesson_rule_links"):
|
||||
"lesson_rule_links",
|
||||
# v19: the lessons judged to fall under no rule.
|
||||
"lesson_no_rule"):
|
||||
assert key in out, f"missing export section: {key}"
|
||||
assert out[key] == []
|
||||
|
||||
|
||||
Reference in New Issue
Block a user