From d05d4f36323603b969dddd7978bfb8e1e1611946 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 22 Oct 2026 19:20:32 -0400 Subject: [PATCH] feat(4636): rule pull-through per surface, and per-rule usage with confirmed lessons on list_rules retrieval_telemetry gains rule_usage.by_source, the twin of usage.by_source: distinct rules each surface showed, how many an agent opened, and a ratio that is null on a bulk delivery. list_rules(with_usage=True, days=N) adds each rule's usage, windowed, and how many lessons are confirmed instances of it, so linked and unlinked rules can be compared through MCP. Co-Authored-By: Claude Opus 5.5 --- src/grimoire/mcp/tools/rulebooks.py | 36 ++++++++-- src/grimoire/mcp/tools/search.py | 9 +++ src/grimoire/services/lesson_rules.py | 28 ++++++++ src/grimoire/services/retrieval_telemetry.py | 71 +++++++++++++++++++- src/grimoire/services/rule_usage.py | 14 +++- tests/test_integration_lesson_rule_links.py | 16 +++++ tests/test_mcp_tool_rulebooks.py | 33 +++++++++ tests/test_services_retrieval_telemetry.py | 48 +++++++++++++ tests/test_services_rule_usage.py | 41 +++++++++++ 9 files changed, 287 insertions(+), 9 deletions(-) diff --git a/src/grimoire/mcp/tools/rulebooks.py b/src/grimoire/mcp/tools/rulebooks.py index 1a2272f7..19c8711a 100644 --- a/src/grimoire/mcp/tools/rulebooks.py +++ b/src/grimoire/mcp/tools/rulebooks.py @@ -10,13 +10,16 @@ preview-style warning. """ from __future__ import annotations +from datetime import datetime, timedelta, timezone + from grimoire.mcp._context import current_user_id from grimoire.services import dedup as dedup_svc from grimoire.services import moments as moments_svc from grimoire.services import rulebooks as rulebooks_svc from grimoire.services import trash as trash_svc +from grimoire.services.lesson_rules import confirmed_lesson_counts from grimoire.services.rule_usage import ( - record_rule_outcome, record_rule_pulled, + empty_rule_usage, record_rule_outcome, record_rule_pulled, usage_for_rules, ) from grimoire.services import moment_delivery @@ -204,6 +207,7 @@ def _rule_summary(r, moments: list[str] | None = None) -> dict: async def list_rules( rulebook_id: int = 0, topic_id: int = 0, project_id: int = 0, + with_usage: bool = False, days: int = 0, ) -> dict: """List rules — filter by rulebook, topic, and/or project. @@ -213,9 +217,21 @@ async def list_rules( project_id: 0 = no filter; positive = that project's OWN rules. Global rules apply to every project, so they are listed by rulebook or topic (or unfiltered), not under each project. + with_usage: add each rule's `usage` and `confirmed_lessons`. Off by + default: a listing is for applying rules, and this is for judging + them. + days: with `with_usage`, count only the last N days; 0 = everything + recorded. rulebook_id and topic_id AND-combine; project_id lists a project's rules on its own. Ownership-scoped. + + `usage` is the rule's own telemetry: `surfaced_count` (a ranked surface + chose it), `ambient_count` (a bulk delivery carried it), `pull_count`, + `applied_count`, `departed_count` and the latest of each. + `confirmed_lessons` counts the lessons judged instances of the rule. Read + together they answer whether rules that situations keep proving are the + ones that surface and get opened. """ uid = current_user_id() rows = await rulebooks_svc.list_rules( @@ -224,12 +240,20 @@ async def list_rules( topic_id=topic_id or None, project_id=project_id or None, ) + ids = [r.id for r in rows] # One batched read for the page, not one per row. - mounted = await rulebooks_svc.list_rule_moments([r.id for r in rows]) - return { - "rules": [_rule_summary(r, mounted.get(r.id)) for r in rows], - "total": len(rows), - } + mounted = await rulebooks_svc.list_rule_moments(ids) + rules = [_rule_summary(r, mounted.get(r.id)) for r in rows] + if with_usage: + since = ( + datetime.now(timezone.utc) - timedelta(days=days) if days > 0 else None + ) + usage = await usage_for_rules(ids, since=since) + lessons = await confirmed_lesson_counts(uid, ids) + for row in rules: + row["usage"] = usage.get(row["id"], empty_rule_usage()) + row["confirmed_lessons"] = lessons.get(row["id"], 0) + return {"rules": rules, "total": len(rows)} async def get_rule(rule_id: int) -> dict: diff --git a/src/grimoire/mcp/tools/search.py b/src/grimoire/mcp/tools/search.py index 26b0d850..025a84d4 100644 --- a/src/grimoire/mcp/tools/search.py +++ b/src/grimoire/mcp/tools/search.py @@ -509,6 +509,15 @@ async def retrieval_telemetry( Read it against `sources["write_path_rule"]`, which declines the large majority of its calls like any other surface. + `rule_usage.by_source` is the per-surface split, the twin of + `usage.by_source`: for each surface that showed rules, `rules_surfaced` + (distinct rules), `rules_pulled` (how many of those an agent opened in the + window) and `pull_through`, null on an ambient surface. It is the readout + that says which arm — the prompt arm, the tool arm, a lesson pointing at + its rule — earns its lines. The same upper bound as the note split: a rule + two surfaces showed and an agent opened once counts for both. + `by_source_failed: true` means that one query failed. + `system_usage` — the RULINGS arm, from `system_usage_events`: how often an area's rulings were shown because a command (`rulings_pre_tool`) or an edit (`rulings_write_path`) touched its files, how often a System diff --git a/src/grimoire/services/lesson_rules.py b/src/grimoire/services/lesson_rules.py index dbc68e1d..008cecdf 100644 --- a/src/grimoire/services/lesson_rules.py +++ b/src/grimoire/services/lesson_rules.py @@ -324,6 +324,34 @@ async def lessons_for_rule(user_id: int, rule_id: int) -> list[dict]: ] +async def confirmed_lesson_counts(user_id: int, rule_ids) -> dict[int, int]: + """{rule_id: confirmed lessons the caller can read}, for a list of rules. + + `lessons_for_rule` in bulk, counted rather than listed: one GROUP BY, so a + rule list can say which rules are proven by situations without a query + per row. Rules with none read 0. + """ + from grimoire.services.access import readable_notes_clause + + ids = [int(i) for i in rule_ids or []] + out = {i: 0 for i in ids} + if not ids: + return out + async with async_session() as session: + rows = (await session.execute( + select(LessonRuleLink.rule_id, func.count()) + .join(Note, Note.id == LessonRuleLink.lesson_id) + .where(LessonRuleLink.rule_id.in_(ids)) + .where(LessonRuleLink.state == CONFIRMED) + .where(Note.deleted_at.is_(None)) + .where(readable_notes_clause(user_id)) + .group_by(LessonRuleLink.rule_id) + )).all() + for rule_id, n in rows: + out[int(rule_id)] = int(n) + return out + + async def judgments_for_lessons(lesson_ids) -> dict[int, dict]: """{lesson_id: {"rule_judgment": linked|no_rule|unjudged, "no_rule"?}}. diff --git a/src/grimoire/services/retrieval_telemetry.py b/src/grimoire/services/retrieval_telemetry.py index e3e3bd2a..eaee032f 100644 --- a/src/grimoire/services/retrieval_telemetry.py +++ b/src/grimoire/services/retrieval_telemetry.py @@ -1109,7 +1109,7 @@ async def retrieval_summary( # Assigned inside the try below; named here so the readout can tell # "this query failed" from "this window has no rows". - by_source_rows = None + by_source_rows = rule_by_source_rows = None rule_rows = None distinct_rules_surfaced = distinct_rules_pulled = 0 # None means the coverage read did not happen — distinct from a table with @@ -1426,6 +1426,53 @@ async def retrieval_summary( rule_rows = None distinct_rules_surfaced = distinct_rules_pulled = 0 distinct_rules_acted = 0 + + # The note block's per-source join, at the rule grain. The + # top-level rule ratio blends every arm — the prompt arm, the tool + # arm, a lesson pointing at its rule — and "which of them earns its + # lines" is the question each arm's bar is tuned against. Its own + # guard, for the reason the note join has one. + try: + rule_pulled_ids = ( + select(RuleUsageEvent.rule_id) + .where( + RuleUsageEvent.created_at >= since, + RuleUsageEvent.user_id == user_id, + RuleUsageEvent.event == RULE_PULLED, + RuleUsageEvent.source.startswith("mcp_", autoescape=True), + ) + .distinct() + .subquery() + ) + rule_surfaced_pairs = ( + select(RuleUsageEvent.source, RuleUsageEvent.rule_id) + .where( + RuleUsageEvent.created_at >= since, + RuleUsageEvent.user_id == user_id, + RuleUsageEvent.event == RULE_SURFACED, + ) + .distinct() + .subquery() + ) + rule_by_source_rows = ( + await session.execute( + select( + rule_surfaced_pairs.c.source, + func.count().label("rules_surfaced"), + func.count(rule_pulled_ids.c.rule_id).label("rules_pulled"), + ) + .select_from( + rule_surfaced_pairs.outerjoin( + rule_pulled_ids, + rule_pulled_ids.c.rule_id == rule_surfaced_pairs.c.rule_id, + ) + ) + .group_by(rule_surfaced_pairs.c.source) + ) + ).all() + except Exception: + logger.warning("per-source rule pull-through read failed", exc_info=True) + rule_by_source_rows = None except Exception: logger.warning("retrieval summary read failed", exc_info=True) out["read_failed"] = True @@ -1596,6 +1643,28 @@ async def retrieval_summary( round(rule_usage["pulled_by_agent"] / rule_usage["surfaced"], 4) if rule_usage["surfaced"] else None ) + # Per surface, the note block's shape and its caveat: a pull records the + # door, not the surface that led to it, so a rule two arms surfaced and an + # agent opened once counts as pulled for both. An upper bound — a surface + # reading near zero here is not being flattered. + if rule_by_source_rows is None: + rule_usage["by_source"] = {} + rule_usage["by_source_failed"] = True + else: + rule_usage["by_source"] = { + source: { + "rules_surfaced": int(n_surfaced or 0), + "rules_pulled": int(n_pulled or 0), + # None on an ambient surface, as for notes: nothing chose + # those rules, so the ratio would be misread. + "pull_through": ( + None if is_ambient(source) or not n_surfaced + else round(int(n_pulled or 0) / int(n_surfaced), 4) + ), + "ambient": is_ambient(source), + } + for source, n_surfaced, n_pulled in rule_by_source_rows + } rule_usage.update(_coverage((rule_complete or {}).get("*"), since)) out["rule_usage"] = rule_usage out["system_usage"] = await _system_usage(user_id, since) diff --git a/src/grimoire/services/rule_usage.py b/src/grimoire/services/rule_usage.py index cc2a246b..9ee388d1 100644 --- a/src/grimoire/services/rule_usage.py +++ b/src/grimoire/services/rule_usage.py @@ -59,8 +59,9 @@ as short and as slow-moving as possible. from __future__ import annotations import logging +from datetime import datetime -from sqlalchemy import case, func, select +from sqlalchemy import case, func, select, true from grimoire.models import async_session from grimoire.models.base import iso @@ -333,13 +334,18 @@ def empty_rule_usage() -> dict: } -async def usage_for_rules(rule_ids: list[int]) -> dict[int, dict]: +async def usage_for_rules( + rule_ids: list[int], since: datetime | None = None, +) -> dict[int, dict]: """Aggregate usage for a set of rules: {rule_id: {counts + timestamps}}. One GROUP BY for the whole page rather than a query per row — this feeds a list view, so the per-row shape would be N+1 by construction. Rules with no events come back with `empty_rule_usage()`, so the caller never has to tell "no events" from "not in the result". + + `since` limits the counts to events at or after it, so two windows can be + compared; without it they cover everything recorded. """ ids = [int(r) for r in rule_ids] out: dict[int, dict] = {rid: empty_rule_usage() for rid in ids} @@ -370,6 +376,10 @@ async def usage_for_rules(rule_ids: list[int]) -> dict[int, dict]: ambient, ) .where(RuleUsageEvent.rule_id.in_(ids)) + .where( + RuleUsageEvent.created_at >= since + if since is not None else true() + ) .group_by( RuleUsageEvent.rule_id, RuleUsageEvent.event, diff --git a/tests/test_integration_lesson_rule_links.py b/tests/test_integration_lesson_rule_links.py index 6284d4e8..99b75b7f 100644 --- a/tests/test_integration_lesson_rule_links.py +++ b/tests/test_integration_lesson_rule_links.py @@ -93,6 +93,22 @@ async def test_naming_rules_confirms_them_and_both_sides_read_the_link(world): assert [(l["id"], l["state"]) for l in lessons] == [(world["lesson"], "confirmed")] +async def test_a_rule_list_counts_confirmed_lessons_only(world): + """The bulk count a rule list reads: a rejected link is not a lesson + proving the rule, a rule with no links reads 0, and a stranger counts + nothing through a lesson they cannot read.""" + owner = world["owner"] + await links_svc.set_lesson_rules(owner, world["lesson"], [world["r1"], world["r2"]]) + await links_svc.judge_link(owner, world["lesson"], world["r2"], "reject", "not it") + rules = [world["r1"], world["r2"], world["foreign"]] + + assert await links_svc.confirmed_lesson_counts(owner, rules) == { + world["r1"]: 1, world["r2"]: 0, world["foreign"]: 0, + } + assert (await links_svc.confirmed_lesson_counts(world["stranger"], rules))[world["r1"]] == 0 + assert await links_svc.confirmed_lesson_counts(owner, []) == {} + + async def test_a_rule_left_out_of_the_set_is_rejected_not_forgotten(world): owner = world["owner"] await links_svc.set_lesson_rules(owner, world["lesson"], [world["r1"], world["r2"]]) diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index 63f54afc..31d05d20 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -573,3 +573,36 @@ def test_rule_outcome_is_registered_and_is_not_read_only(): assert "rule_outcome" in mcp.names assert "rule_outcome" in _WRITE_TOOLS assert "rule_outcome" not in _READ_ONLY_TOOLS + + +@pytest.mark.asyncio +async def test_list_rules_carries_usage_only_when_asked(): + """A listing is for applying rules, so it stays lean; `with_usage` adds + each rule's telemetry and confirmed-lesson count for judging them, with + `days` passed down as a window.""" + from grimoire.mcp.tools import rulebooks as tools + from grimoire.services.rule_usage import empty_rule_usage + + rows = [fake_rule(id=1, title="dev is home"), fake_rule(id=2, title="ask first")] + seen = {**empty_rule_usage(), "surfaced_count": 4, "pull_count": 1} + usage = AsyncMock(return_value={1: seen}) + lessons = AsyncMock(return_value={1: 3}) + with patch.object(tools.rulebooks_svc, "list_rules", AsyncMock(return_value=rows)), \ + patch.object(tools.rulebooks_svc, "list_rule_moments", AsyncMock(return_value={})), \ + patch.object(tools, "usage_for_rules", usage), \ + patch.object(tools, "confirmed_lesson_counts", lessons): + lean = await tools.list_rules() + assert all("usage" not in r and "confirmed_lessons" not in r for r in lean["rules"]) + usage.assert_not_awaited() + + out = await tools.list_rules(with_usage=True, days=14) + + by_id = {r["id"]: r for r in out["rules"]} + assert by_id[1]["usage"]["surfaced_count"] == 4 + assert by_id[1]["confirmed_lessons"] == 3 + # A rule missing from either read is zero-filled, never absent. + assert by_id[2]["usage"] == empty_rule_usage() + assert by_id[2]["confirmed_lessons"] == 0 + assert usage.await_args.args[0] == [1, 2] + assert usage.await_args.kwargs["since"] is not None + lessons.assert_awaited_once_with(7, [1, 2]) diff --git a/tests/test_services_retrieval_telemetry.py b/tests/test_services_retrieval_telemetry.py index bde651b5..2f80a94f 100644 --- a/tests/test_services_retrieval_telemetry.py +++ b/tests/test_services_retrieval_telemetry.py @@ -618,6 +618,54 @@ async def test_the_preload_lands_in_ambient_and_never_in_the_ratio(_dispose_engi await cleanup() +@pytest.mark.integration +@pytest.mark.asyncio +async def test_rule_by_source_says_which_arm_earns_its_lines(_dispose_engine): + """The rule ratio blends every arm. Per surface it can say which one is + working: distinct rules per arm, the agent opens among them, and no ratio + on a bulk delivery.""" + from grimoire.services.retrieval_telemetry import retrieval_summary + + cleanup = await _rule_events(990016, [ + # The write arm chose rule 5301 twice — one rule — and 5302 once. + (5301, "surfaced", "write_path_rule"), + (5301, "surfaced", "write_path_rule"), + (5302, "surfaced", "write_path_rule"), + # The tool arm chose 5303 and nothing came of it. + (5303, "surfaced", "pre_tool_rule"), + # The preload carried 5301 as well. + (5301, "surfaced", "session_start"), + # An agent opened 5301; a person opened 5303, which is not a hint + # landing. + (5301, "pulled", "mcp_get_rule"), + (5303, "pulled", "rest_rule"), + ]) + try: + ru = (await retrieval_summary(990016, days=30))["rule_usage"] + assert "by_source_failed" not in ru, "the join did not execute" + by_source = ru["by_source"] + + wp = by_source["write_path_rule"] + assert (wp["rules_surfaced"], wp["rules_pulled"]) == (2, 1) + assert wp["pull_through"] == 0.5 + assert wp["ambient"] is False + + tool = by_source["pre_tool_rule"] + assert (tool["rules_surfaced"], tool["rules_pulled"]) == (1, 0) + assert tool["pull_through"] == 0.0 + + preload = by_source["session_start"] + # The pull is counted against every surface that showed the rule, + # the upper bound the readout states. + assert (preload["rules_surfaced"], preload["rules_pulled"]) == (1, 1) + assert preload["pull_through"] is None + assert preload["ambient"] is True + + assert "mcp_get_rule" not in by_source, "a pull is not a surface" + finally: + await cleanup() + + @pytest.mark.integration @pytest.mark.asyncio async def test_ambient_alone_reports_no_ratio(_dispose_engine): diff --git a/tests/test_services_rule_usage.py b/tests/test_services_rule_usage.py index f51a358e..6928b5e4 100644 --- a/tests/test_services_rule_usage.py +++ b/tests/test_services_rule_usage.py @@ -198,6 +198,47 @@ async def test_usage_for_rules_aggregates_per_rule(_dispose_engine): await s.commit() +@pytest.mark.integration +@pytest.mark.asyncio +async def test_usage_for_rules_counts_only_the_window_it_is_given(_dispose_engine): + """Two windows have to be comparable, so an event before `since` is left + out of every count — and without `since` it is counted.""" + from datetime import datetime, timedelta, timezone + + from sqlalchemy import delete + + from grimoire.models import async_session + from grimoire.models.rule_usage import RuleUsageEvent + + old = datetime.now(timezone.utc) - timedelta(days=30) + async with async_session() as s: + s.add_all([ + RuleUsageEvent(user_id=990022, rule_id=6011, event=SURFACED, + source="write_path_rule", created_at=old), + RuleUsageEvent(user_id=990022, rule_id=6011, event=PULLED, + source="mcp_get_rule", created_at=old), + RuleUsageEvent(user_id=990022, rule_id=6011, event=SURFACED, + source="write_path_rule"), + ]) + await s.commit() + try: + since = datetime.now(timezone.utc) - timedelta(days=14) + recent = (await rule_usage.usage_for_rules([6011], since=since))[6011] + assert recent["surfaced_count"] == 1 + assert recent["pull_count"] == 0 + assert recent["last_pulled_at"] is None + + everything = (await rule_usage.usage_for_rules([6011]))[6011] + assert everything["surfaced_count"] == 2 + assert everything["pull_count"] == 1 + finally: + async with async_session() as s: + await s.execute( + delete(RuleUsageEvent).where(RuleUsageEvent.user_id == 990022) + ) + await s.commit() + + @pytest.mark.integration @pytest.mark.asyncio async def test_usage_for_rules_on_an_empty_id_list_asks_the_database_nothing(