From dfcb000719c915b456e50baf0d58e4a6bedb4741 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 20 Sep 2026 23:56:05 -0400 Subject: [PATCH] feat(rules): a surfaced rule gets an outcome, not just a read (#4212) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone 419 step 1. `rule_usage_events` could say a rule was SURFACED and that it was PULLED. It could not say what happened next, so a rule that fires constantly and is always obeyed and a rule that fires constantly and is never obeyed left byte-identical telemetry. The second is far the more urgent and was the one the readout could not name — measured on a session where three of seven misses were caught by the operator and none by the system. Two new events, `applied` and `departed`, and a `detail` column carrying the why of a departure. No CHECK migration: `event` was created in 0094 as plain Text with no constraint, verified in the migration rather than assumed from the model, so rule 36 does not bite here — said in both places because the next person adding a value will reach for it. THE THIRD STATE IS DERIVED, AND THAT IS THE DESIGN. Read-and-silently- unchanged is the failure this milestone was opened on, and it cannot be reported: an agent that knew it was ignoring a rule would not be ignoring it. So nothing here asks. `applied` and `departed` are reported; the third state is a rule that was opened and left no trace. An `ignored` enum member would collect nothing while reading as though it had measured something, which is #3311's failure — a statistic that could not vary being taken for a finding. `detail` is a column rather than two more bare event strings because a departure stripped of its reason reads back as a miss, so the two states this exists to separate would collapse again one layer down, in the readout, where nobody would see it happen. Nullable: following a rule needs no argument, and an expensive event is one that stops being recorded. `outcome_state` is the single reading of the four states, taking the aggregate `usage_for_rules` already returns, so the badge, the readout and any later session summary cannot disagree about what "followed" means — the drift #3246 found across the rules system. A departure outranks an application: a rule both applied and argued with is a rule someone argued with, and the argument is the half worth surfacing. `rule_outcome` is the MCP door, classed as a WRITE. The read-only set tolerates getters that call record_pulled, but those are reads that leave a trace; this tool's entire effect is the row, and the row carries prose the agent authored. A read-scoped key that can put text in the operator's database is not read-scoped, whatever table it lands in. Backup carries `detail` on both sides. It is the one field here a fresh install cannot re-earn — counts come back by being used again, a stated reason exists once — and #4197 records that the column guard watches the export side only, so the round-trip test is the thing that would catch a one-sided add. Delivery is deliberately not settled here: how an agent gets prompted to record an outcome is step 3's subject, and the same record serves whichever answer that step reaches. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- ...0106_rule_usage_events_carry_an_outcome.py | 65 +++++++++ src/scribe/mcp/server.py | 8 + src/scribe/mcp/tools/rulebooks.py | 66 ++++++++- src/scribe/models/rule_usage.py | 38 ++++- src/scribe/services/backup.py | 12 ++ src/scribe/services/rule_usage.py | 134 ++++++++++++++++- ...integration_backup_rule_usage_roundtrip.py | 61 +++++++- tests/test_mcp_tool_rulebooks.py | 104 +++++++++++++ tests/test_services_rule_usage.py | 138 +++++++++++++++++- 9 files changed, 615 insertions(+), 11 deletions(-) create mode 100644 alembic/versions/0106_rule_usage_events_carry_an_outcome.py diff --git a/alembic/versions/0106_rule_usage_events_carry_an_outcome.py b/alembic/versions/0106_rule_usage_events_carry_an_outcome.py new file mode 100644 index 0000000..2eef757 --- /dev/null +++ b/alembic/versions/0106_rule_usage_events_carry_an_outcome.py @@ -0,0 +1,65 @@ +"""rule_usage_events carry an outcome, not just a read (#4212, milestone 419) + +Revision ID: 0106 +Revises: 0105 +Create Date: 2026-09-20 + +Milestone 419's first step. `rule_usage_events` can say a rule was SURFACED +and that it was PULLED. It cannot say what happened next, so these two +sessions leave identical telemetry: + + - a rule surfaced, opened, and followed; + - a rule surfaced, opened, and silently ignored. + +The second is the more urgent by a distance, and it is the one the readout +cannot name. That is the whole of what this milestone is about, measured on a +session where three of seven misses were caught by the operator and none by +the system. + +TWO NEW EVENT VALUES, AND NO CHECK MIGRATION. `event` was created in 0094 as +plain `sa.Text()` with no constraint — checked in the migration itself, not +assumed from the model — so `applied` and `departed` join `surfaced` and +`pulled` without a DROP/ADD pair. Rule 36 governs CHECK-whitelisted columns +and this is not one; noted explicitly because the next reader will reach for +rule 36 here, and should be able to see in one place why it does not bite. + +THE THIRD STATE IS DERIVED, AND THAT IS NOT A SHORTCUT. Read-and-silently- +unchanged is the absence of an outcome, and it has to be: an agent that knew +it was ignoring a rule would not be ignoring it. There is no honest way to ask +for that event, so nothing here tries. `applied` and `departed` are reported; +the third state is what is left over when a rule was pulled and neither +arrived. A schema that offered an `ignored` value would collect nothing and +read as though it had measured something, which is the #3311 failure — a +statistic that cannot vary being mistaken for a finding. + +`detail` CARRIES THE WHY OF A DEPARTURE, and is the reason this is a column +rather than two more bare event strings. A departure without its reason is +indistinguishable from a miss when someone reads the table back, so the two +states the milestone wants to tell apart would collapse again one layer down. +Nullable because `applied` needs no argument — following a rule is the +unremarkable case, and demanding prose for it would make the cheap event +expensive and stop it being recorded at all. + +NO NEW INDEX. Every outcome readout starts from a set of rule ids and narrows +by event, which is exactly `ix_rule_usage_rule_event` (rule_id, event) from +0094. Adding a `detail` index would serve no query anyone has — the column is +read, never filtered on. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0106" +down_revision = "0105" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column( + "rule_usage_events", + sa.Column("detail", sa.Text(), nullable=True), + ) + + +def downgrade() -> None: + op.drop_column("rule_usage_events", "detail") diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index dfe1f4f..6a07d7c 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -185,6 +185,14 @@ _WRITE_TOOLS = frozenset({ "create_rule", "create_project_rule", "update_rule", "move_rule", "delete_rule", "create_preference", "update_preference", "relate_rules", "unrelate_rules", "mark_rule_verified", + # rule_outcome writes only telemetry, which is the case _READ_ONLY_TOOLS + # above explicitly tolerates for getters that call record_pulled. It is + # classed as a WRITE anyway, on the difference that matters: those are + # reads that happen to leave a trace, while this tool's entire effect is + # the row — and the row carries `detail`, free prose the agent authored. + # A read-scoped key that can put text into the operator's database is not + # read-scoped, whatever table it lands in (#4212). + "rule_outcome", # retrieval tuning — a write in both senses: it moves the number the arm # reads, and it appends the reason to the audit trail (#4102). "tune_retrieval", diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index d6c6268..650d5fb 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -18,7 +18,9 @@ from scribe.mcp._context import current_user_id from scribe.services import dedup as dedup_svc from scribe.services import rulebooks as rulebooks_svc from scribe.services import trash as trash_svc -from scribe.services.rule_usage import record_rule_pulled +from scribe.services.rule_usage import ( + record_rule_outcome, record_rule_pulled, +) # ── Rulebook CRUD ─────────────────────────────────────────────────────── @@ -245,6 +247,66 @@ async def get_rule(rule_id: int) -> dict: return await rulebooks_svc.rule_detail(uid, rule) +async def rule_outcome(rule_id: int, outcome: str, why: str = "") -> dict: + """Record what a rule you read ACTUALLY CHANGED — applied, or departed from. + + Call this after a rule has been surfaced to you and you have acted. It is + the only way the system can tell a rule that is working from a rule that + is being read and ignored: `surfaced` says it was offered, `get_rule` says + it was opened, and until this exists neither says whether it made any + difference. A rule obeyed every time and a rule ignored every time leave + identical telemetry, and the second is the one worth knowing about. + + `outcome` is one of: + + "applied" — it changed what you did, or it confirmed the approach you + were already taking. `why` is optional; following a rule is + the ordinary case and does not need an argument. + "departed" — you read it and deliberately did not follow it. `why` is + REQUIRED and is the whole value of the call: a departure + without its reason is indistinguishable from a miss when + somebody reads this back, and "somebody" is usually you, in + a later session, with none of today's context. + + There is deliberately NO value for "read it and ignored it". That state is + real, and it is the one this measurement exists to expose — but it is not + something you can report, because noticing it is the same act as not doing + it. It is derived instead: a rule you opened and never came back to. The + honest way to keep yourself out of that bucket is to call this, not to + reach for a word that describes it. + + A departure is a legitimate answer and is not a confession. Rules are + written for the common case; recording the edge you found is how the rule + gets better, and a corpus where nothing is ever departed from is a corpus + nobody is really reading. + """ + uid = current_user_id() + rule = await rulebooks_svc.get_rule(rule_id, uid) + if rule is None: + raise ValueError(f"rule {rule_id} not found") + choice = (outcome or "").strip().lower() + if choice not in ("applied", "departed"): + raise ValueError( + f"outcome must be 'applied' or 'departed', got {outcome!r}" + ) + if choice == "departed" and not (why or "").strip(): + raise ValueError( + "a departure needs its reason — pass `why`. Without it the record " + "cannot be told from a rule that was simply missed." + ) + record_rule_outcome( + user_id=uid, rule_id=int(rule.id), outcome=choice, + source="mcp_rule_outcome", detail=why, + ) + return { + "rule_id": int(rule.id), + "title": rule.title, + "outcome": choice, + "why": (why or "").strip() or None, + "recorded": True, + } + + async def create_rule( topic_id: int, title: str, statement: str, when_to_apply: str, why: str = "", how_to_apply: str = "", order_index: int = 0, @@ -1086,7 +1148,7 @@ def register(mcp) -> None: for fn in ( list_rulebooks, get_rulebook, create_rulebook, update_rulebook, delete_rulebook, list_topics, create_topic, update_topic, delete_topic, - list_rules, get_rule, + list_rules, get_rule, rule_outcome, create_rule, create_project_rule, update_rule, move_rule, delete_rule, create_preference, update_preference, relate_rules, unrelate_rules, diff --git a/src/scribe/models/rule_usage.py b/src/scribe/models/rule_usage.py index ea53c96..44cb65b 100644 --- a/src/scribe/models/rule_usage.py +++ b/src/scribe/models/rule_usage.py @@ -7,9 +7,24 @@ from scribe.models.base import CreatedAtMixin, iso SURFACED = "surfaced" PULLED = "pulled" +# The outcome half (#4212, milestone 419). A rule that was read and then +# ignored has always been indistinguishable from one that was read and +# obeyed; these are the two events that can tell them apart. +# +# There is deliberately NO third value for "read and ignored". That state is +# real and is the whole point of the milestone, but it cannot be reported: +# an agent that knew it was ignoring a rule would not be ignoring it. It is +# DERIVED — a pull with no outcome — and an enum member for it would collect +# nothing while reading as though it had measured something, which is #3311's +# failure exactly. +APPLIED = "applied" +DEPARTED = "departed" +OUTCOMES = (APPLIED, DEPARTED) + class RuleUsageEvent(Base, CreatedAtMixin): - """One row per time a rule was SURFACED to the agent, or PULLED in full. + """One row per time a rule was SURFACED to the agent, PULLED in full, or + ACTED ON — applied, or departed from with a stated reason. The sibling `note_usage_events` has had since 2026-07, third in the line after `rule_embeddings` and `rule_versions` — and, like those, it exists @@ -58,7 +73,13 @@ class RuleUsageEvent(Base, CreatedAtMixin): user_id: Mapped[int | None] = mapped_column(BigInteger, nullable=True) rule_id: Mapped[int] = mapped_column(BigInteger, nullable=False) - # 'surfaced' | 'pulled' + # 'surfaced' | 'pulled' | 'applied' | 'departed' + # + # Plain Text with no CHECK, as created in 0094 — which is why 0106 added + # the outcome pair without a DROP/ADD migration. Rule 36 governs + # CHECK-whitelisted columns and this is not one. Said here as well as in + # the migration because this is where the next person adding a value will + # look first. event: Mapped[str] = mapped_column(Text, nullable=False) # Which surface produced it. A CONVENTION, not a fixed vocabulary, and the @@ -76,6 +97,18 @@ class RuleUsageEvent(Base, CreatedAtMixin): # without saying why. source: Mapped[str] = mapped_column(Text, nullable=False) + # The WHY of a departure, and the reason the outcome pair is not simply + # two more bare event strings. A departure stripped of its reason reads + # back as a miss, so the two states this table exists to separate would + # collapse again one layer down — in the readout, where nobody would see + # it happen. + # + # Nullable because `applied` needs no argument. Following a rule is the + # unremarkable case; demanding prose for it would make the cheap event + # expensive, and an expensive event is one that stops being recorded. + # Empty on a `surfaced` or `pulled` row, which nobody asks a reason of. + detail: Mapped[str | None] = mapped_column(Text, nullable=True) + __table_args__ = ( # Every readout is "these rule ids, split by event" — a covering # composite beats separate single-column indexes for it. @@ -92,4 +125,5 @@ class RuleUsageEvent(Base, CreatedAtMixin): "rule_id": self.rule_id, "event": self.event, "source": self.source, + "detail": self.detail, } diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 735b559..b7190c0 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -345,6 +345,12 @@ def _rule_usage_event_rows(rows) -> list[dict]: { "user_id": r.user_id, "rule_id": r.rule_id, "event": r.event, "source": r.source, + # The reason a rule was departed from (#4212). Exported because + # it is the only field on this table that cannot be recomputed: + # counts can be re-derived from a fresh install's own use, a + # stated reason cannot, and a `departed` row that comes back + # without one is indistinguishable from a rule that was missed. + "detail": r.detail, "created_at": r.created_at.isoformat() if r.created_at else None, } for r in rows @@ -1573,6 +1579,12 @@ async def _restore_v2(data: dict) -> dict: rule_id=mapped_rid, event=ev.get("event", ""), source=ev.get("source", ""), + # `.get(...) or None` rather than a bare default: an archive + # written before 0106 has no key at all, and one written + # after may carry "" for a non-departure row. Both mean "no + # reason", and both must land as NULL so the readout does not + # have to tell an empty string from an absent one. + detail=(ev.get("detail") or None), created_at=_dt(ev.get("created_at")), )) stats["rule_usage_events"] += 1 diff --git a/src/scribe/services/rule_usage.py b/src/scribe/services/rule_usage.py index 1fc53cc..6b80a5d 100644 --- a/src/scribe/services/rule_usage.py +++ b/src/scribe/services/rule_usage.py @@ -85,7 +85,9 @@ from sqlalchemy import case, func, select from scribe.models import async_session from scribe.models.base import iso -from scribe.models.rule_usage import PULLED, SURFACED, RuleUsageEvent +from scribe.models.rule_usage import ( + APPLIED, DEPARTED, OUTCOMES, PULLED, SURFACED, RuleUsageEvent, +) from scribe.services.background import report_telemetry_failure, spawn logger = logging.getLogger(__name__) @@ -207,6 +209,107 @@ def record_rule_pulled(*, user_id: int | None, rule_id: int, source: str) -> Non _schedule(rows) +def record_rule_outcome( + *, + user_id: int | None, + rule_id: int, + outcome: str, + source: str, + detail: str = "", +) -> None: + """Fire-and-forget: record what a rule ACTUALLY CHANGED (#4212). + + The third stream, and the one milestone 419 exists for. `surfaced` says + the system offered a rule; `pulled` says somebody opened it. Neither says + whether it made any difference, so a rule that fires constantly and is + always obeyed and a rule that fires constantly and is never obeyed have, + until now, produced identical telemetry. The second is far the more + urgent and is precisely the one the readout could not name. + + Two outcomes, because there are only two a judge can honestly report: + + APPLIED — the rule changed what was done, or confirmed it. No `detail` + required: following a rule is the unremarkable case and + charging prose for it is how an event stops being recorded. + DEPARTED — read, and deliberately not followed. `detail` is REQUIRED + and is the entire value of the event. A departure without + its reason reads back as a miss, which collapses the two + states this exists to separate. + + THERE IS NO THIRD CALL, and the absence is the design. Read-and-silently- + unchanged is real — it is the failure this milestone was opened on — but + it cannot be reported, because an agent that knew it was ignoring a rule + would not be ignoring it. It is derived: a rule pulled, with no outcome + behind it. See `outcome_state`. + + Guarded rather than trusting: a bad outcome or a reasonless departure is + dropped and REPORTED, never written. Telemetry that lies is worse than + telemetry that is missing (#2663), and a `departed` row with an empty + reason is a lie the readout cannot detect. + """ + if outcome not in OUTCOMES: + logger.warning("rule outcome rejected: unknown outcome %r", outcome) + spawn(_report_failure("outcome_unknown"), site="rule_usage_outcome") + return + if outcome == DEPARTED and not (detail or "").strip(): + logger.warning("rule outcome rejected: departure with no reason") + spawn(_report_failure("outcome_no_reason"), site="rule_usage_outcome") + return + try: + rows = [ + { + "user_id": user_id, + "rule_id": int(rule_id), + "event": outcome, + "source": source, + "detail": (detail or "").strip() or None, + } + ] + except Exception: + logger.debug("rule usage payload build failed", exc_info=True) + return + _schedule(rows) + + +# What a rule's usage says happened to it, in one word. The four states are +# ordered by how much the system actually knows, and only the last two are +# new — the point of the milestone is that UNACTED used to be invisible +# inside APPLIED. +UNREAD = "unread" # surfaced, never opened +UNACTED = "unacted" # opened, and nothing recorded after — the blind spot +FOLLOWED = "followed" # opened and applied +DEPARTED_FROM = "departed" # opened and deliberately not followed, with a why + + +def outcome_state(usage: dict) -> str: + """The three states milestone 419 asked to be able to tell apart, plus + the one that already existed. + + Pure, and reading only the aggregate `usage_for_rules` already returns — + so the readout, the badge and any later session summary all answer this + question the same way. Two callers computing "was this followed" from raw + counts is the drift #3246 found across the rules system, arriving again. + + PRECEDENCE, and it is deliberate: a departure outranks an application. + A rule both applied and departed from in the same window is a rule + someone argued with, and the argument is the interesting half — reporting + it as plain compliance would hide the one row a reader most wants. + + UNACTED is the derived state and the reason this function exists. It is + not "no data"; it is a rule that was surfaced, deliberately OPENED, and + then left no trace of having mattered. That is a much stronger signal + than never having been opened at all, and it is the signal that was + previously indistinguishable from compliance. + """ + if int(usage.get("departed_count") or 0): + return DEPARTED_FROM + if int(usage.get("applied_count") or 0): + return FOLLOWED + if int(usage.get("pull_count") or 0): + return UNACTED + return UNREAD + + def empty_rule_usage() -> dict: """The zero readout — what a rule with no recorded events looks like. @@ -227,6 +330,21 @@ def empty_rule_usage() -> dict: "surfaced_count": 0, "ambient_count": 0, "pull_count": 0, + # The outcome half (#4212). Zero here means "nothing recorded", which + # for a rule that was also never pulled is simply silence — and for + # one that WAS pulled is the blind spot this milestone is named for. + # `outcome_state` is what tells those apart; no caller should be + # reading these counts raw to decide it. + # + # The REASON for a departure is on the row (`detail`), not here. One + # GROUP BY cannot carry the text of the latest departure without a + # DISTINCT ON alongside it, and a key that the aggregate could never + # fill would read as "no reason given" on every rule that has one — + # a permanently-null field that lies. The readout that needs the + # prose reads the rows (#4213). + "applied_count": 0, + "departed_count": 0, + "last_outcome_at": None, "last_surfaced_at": None, "last_pulled_at": None, } @@ -288,7 +406,19 @@ async def usage_for_rules(rule_ids: list[int]) -> dict[int, dict]: slot = out.get(int(rule_id)) if slot is None: continue - if event == SURFACED and is_amb: + # Outcomes first, and never split by ambient. `ambient` asks whether + # a RANKER chose to show the rule; an outcome is reported by a judge + # after the fact and has no ranker behind it, so the flag is noise + # here. Branching on it would silently drop every outcome row into a + # bucket nothing reads. + if event in OUTCOMES: + key = "applied_count" if event == APPLIED else "departed_count" + slot[key] = slot[key] + int(n) + prev = slot["last_outcome_at"] + now = iso(last_at) + if now and (prev is None or now > prev): + slot["last_outcome_at"] = now + elif event == SURFACED and is_amb: slot["ambient_count"] = int(n) elif event == SURFACED: slot["surfaced_count"] = int(n) diff --git a/tests/test_integration_backup_rule_usage_roundtrip.py b/tests/test_integration_backup_rule_usage_roundtrip.py index 10f2dc9..5e8f0dc 100644 --- a/tests/test_integration_backup_rule_usage_roundtrip.py +++ b/tests/test_integration_backup_rule_usage_roundtrip.py @@ -25,7 +25,9 @@ from sqlalchemy import select from scribe.models import async_session from scribe.models.note import Note -from scribe.models.rule_usage import PULLED, SURFACED, RuleUsageEvent +from scribe.models.rule_usage import ( + APPLIED, DEPARTED, PULLED, SURFACED, RuleUsageEvent, +) from scribe.models.rulebook import Rule, Rulebook, RulebookTopic from scribe.models.user import User from scribe.services import backup @@ -141,6 +143,21 @@ async def source(): user_id=None, rule_id=rule.id, event=SURFACED, source="write_path_rule", ), + # A departure and its reason (#4212). Seeded here because + # `detail` is the ONE field on this table that cannot be + # recomputed: a fresh install re-earns its counts by being used, + # but a stated reason exists once and is gone if a restore drops + # it — and a `departed` row that comes back reasonless reads as a + # rule that was simply missed. + RuleUsageEvent( + user_id=uid, rule_id=rule.id, + event=DEPARTED, source="mcp_rule_outcome", + detail="the integration lane has no registry credentials", + ), + RuleUsageEvent( + user_id=uid, rule_id=rule.id, + event=APPLIED, source="mcp_rule_outcome", + ), ]) await s.commit() book_id, rule_id, note_id = book.id, rule.id, note.id @@ -232,8 +249,8 @@ async def restored(source): async def test_every_event_comes_back(restored): """The count first: every shape assertion below reads the same on an empty - list, so without this a restore that dropped all three would pass them.""" - assert len(restored["events"]) == 3 + list, so without this a restore that dropped all five would pass them.""" + assert len(restored["events"]) == 5 async def test_the_events_attach_to_the_RESTORED_rule(restored): @@ -272,7 +289,9 @@ async def test_the_actor_is_remapped_and_a_missing_one_survives(restored): surfacings of exactly the surface being measured.""" attributed = [e for e in restored["events"] if e.user_id is not None] orphaned = [e for e in restored["events"] if e.user_id is None] - assert len(attributed) == 2 + # Four attributed: the surfacing, the pull, and the two outcome rows + # added with `detail` (#4212). One orphaned, deliberately. + assert len(attributed) == 4 assert len(orphaned) == 1, ( "the event with no actor did not come back. Telemetry outlives the " "account it was recorded for; dropping it silently lowers the " @@ -289,6 +308,40 @@ async def test_the_event_and_source_survive(restored): assert pairs == { (SURFACED, "write_path_rule"), (PULLED, "mcp_get_rule"), + (DEPARTED, "mcp_rule_outcome"), + (APPLIED, "mcp_rule_outcome"), } assert sum(1 for e in restored["events"] if e.event == SURFACED) == 2 assert sum(1 for e in restored["events"] if e.event == PULLED) == 1 + + +async def test_a_departures_reason_survives_the_round_trip(restored): + """The one field here that a fresh install cannot re-earn. + + Counts come back by being used again; a stated reason exists once. A + restore that kept the `departed` row and dropped its `detail` would turn + a deliberate, argued departure into something indistinguishable from a + rule that was read and missed — which is the exact distinction milestone + 419 was opened to create, undone silently at the one moment nobody is + watching. + + #4197 is the standing warning behind this test: the backup column guard + watches the export side only, so a column added to the model and to the + exporter and NOT to the importer round-trips as null with nothing to say + so. + """ + departures = [e for e in restored["events"] if e.event == DEPARTED] + assert len(departures) == 1 + assert departures[0].detail == ( + "the integration lane has no registry credentials" + ) + + +async def test_an_application_carries_no_reason_and_that_is_not_a_loss(restored): + """`applied` is the unremarkable case and is stored reasonless on + purpose. Asserted so that a later change making `detail` NOT NULL — or + backfilling it with a placeholder — has to argue with a test rather than + quietly make every application look like it had something to say.""" + applications = [e for e in restored["events"] if e.event == APPLIED] + assert len(applications) == 1 + assert applications[0].detail is None diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index 202a30b..932fc84 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -478,3 +478,107 @@ async def test_move_rule_on_someone_elses_rule_is_not_found(): from scribe.mcp.tools.rulebooks import move_rule with pytest.raises(ValueError, match="not found"): await move_rule(rule_id=94, project_id=3) + + +# ── rule_outcome: what a rule actually changed (#4212, milestone 419) ───── + +@pytest.mark.asyncio +async def test_rule_outcome_records_an_application(): + rule = fake_rule(id=156, title="`dev` is home") + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=rule), + ), patch( + "scribe.mcp.tools.rulebooks.record_rule_outcome", MagicMock() + ) as rec: + from scribe.mcp.tools.rulebooks import rule_outcome + out = await rule_outcome(rule_id=156, outcome="applied") + assert out["outcome"] == "applied" and out["recorded"] is True + assert out["why"] is None + assert rec.call_args.kwargs["outcome"] == "applied" + assert rec.call_args.kwargs["source"] == "mcp_rule_outcome" + + +@pytest.mark.asyncio +async def test_rule_outcome_records_a_departure_with_its_reason(): + rule = fake_rule(id=156, title="`dev` is home") + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=rule), + ), patch( + "scribe.mcp.tools.rulebooks.record_rule_outcome", MagicMock() + ) as rec: + from scribe.mcp.tools.rulebooks import rule_outcome + out = await rule_outcome( + rule_id=156, outcome="departed", why="the operator asked for main" + ) + assert out["outcome"] == "departed" + assert rec.call_args.kwargs["detail"] == "the operator asked for main" + + +@pytest.mark.asyncio +async def test_a_departure_without_a_reason_is_refused_at_the_door(): + """Refused with a message that says WHY a reason is needed, not just + that one is missing — the caller is an agent deciding whether to bother, + and "it cannot be told from a miss" is the argument that lands.""" + rule = fake_rule(id=156, title="`dev` is home") + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=rule), + ), patch( + "scribe.mcp.tools.rulebooks.record_rule_outcome", MagicMock() + ) as rec: + from scribe.mcp.tools.rulebooks import rule_outcome + with pytest.raises(ValueError, match="departure needs its reason"): + await rule_outcome(rule_id=156, outcome="departed", why=" ") + rec.assert_not_called() + + +@pytest.mark.asyncio +async def test_there_is_no_way_to_report_having_ignored_a_rule(): + """Deliberate, and the reason is in the tool's docstring: noticing that + you ignored a rule is the same act as not ignoring it, so the state is + derived rather than reported. A caller reaching for the word gets an + error rather than a row that would read as measurement.""" + rule = fake_rule(id=156, title="`dev` is home") + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=rule), + ), patch( + "scribe.mcp.tools.rulebooks.record_rule_outcome", MagicMock() + ) as rec: + from scribe.mcp.tools.rulebooks import rule_outcome + for bogus in ("ignored", "skipped", "read", ""): + with pytest.raises(ValueError, match="must be 'applied' or 'departed'"): + await rule_outcome(rule_id=156, outcome=bogus) + rec.assert_not_called() + + +@pytest.mark.asyncio +async def test_rule_outcome_refuses_a_rule_the_caller_cannot_see(): + """The access check comes FIRST, so a miss cannot be used to probe for + the existence of someone else's rule, and nothing is recorded against + an id the caller has no claim on.""" + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=None), + ), patch( + "scribe.mcp.tools.rulebooks.record_rule_outcome", MagicMock() + ) as rec: + from scribe.mcp.tools.rulebooks import rule_outcome + with pytest.raises(ValueError, match="rule 999 not found"): + await rule_outcome(rule_id=999, outcome="applied") + rec.assert_not_called() + + +def test_rule_outcome_is_registered_and_is_not_read_only(): + """Its whole effect is a write, and `why` is prose the agent authored — + a read-scoped key that can put text in the operator's database is not + read-scoped, whatever table it lands in.""" + from scribe.mcp.server import _READ_ONLY_TOOLS, _WRITE_TOOLS + from scribe.mcp.tools import rulebooks as mod + mcp = FakeMCP() + mod.register(mcp) + assert "rule_outcome" in mcp.names + assert "rule_outcome" in _WRITE_TOOLS + assert "rule_outcome" not in _READ_ONLY_TOOLS diff --git a/tests/test_services_rule_usage.py b/tests/test_services_rule_usage.py index 95cbe8b..0142c7a 100644 --- a/tests/test_services_rule_usage.py +++ b/tests/test_services_rule_usage.py @@ -6,7 +6,9 @@ where a mistake is silent rather than loud. """ import pytest -from scribe.models.rule_usage import PULLED, SURFACED, RuleUsageEvent +from scribe.models.rule_usage import ( + APPLIED, DEPARTED, PULLED, SURFACED, RuleUsageEvent, +) from scribe.services import rule_usage @@ -101,6 +103,9 @@ def test_the_zero_readout_names_every_key(): "surfaced_count": 0, "ambient_count": 0, "pull_count": 0, + "applied_count": 0, + "departed_count": 0, + "last_outcome_at": None, "last_surfaced_at": None, "last_pulled_at": None, } @@ -255,3 +260,134 @@ async def test_a_preloaded_rule_does_not_read_as_a_ranked_surfacing(_dispose_eng delete(RuleUsageEvent).where(RuleUsageEvent.user_id == 990021) ) await s.commit() + + +# ── the outcome stream (#4212, milestone 419) ───────────────────────────── +# +# What these guard is a distinction, not a payload. Before this existed, a +# rule read and obeyed and a rule read and ignored left byte-identical +# telemetry, so the readout could not name the failure the whole milestone +# was opened on. The tests that matter most below are the ones asserting +# that a REASONLESS DEPARTURE IS NEVER WRITTEN, and that an unacted rule is +# a state in its own right rather than the absence of one. + +def test_an_applied_outcome_needs_no_argument(captured): + """Following a rule is the ordinary case. Charging prose for it would + make the cheap event expensive, and an expensive event stops being + recorded — which costs the whole measurement.""" + rule_usage.record_rule_outcome( + user_id=7, rule_id=156, outcome=APPLIED, source="mcp_rule_outcome" + ) + [batch] = captured + assert batch == [{ + "user_id": 7, "rule_id": 156, "event": APPLIED, + "source": "mcp_rule_outcome", "detail": None, + }] + + +def test_a_departure_carries_its_reason(captured): + rule_usage.record_rule_outcome( + user_id=7, rule_id=156, outcome=DEPARTED, source="mcp_rule_outcome", + detail=" the integration lane has no registry credentials ", + ) + [batch] = captured + assert batch[0]["event"] == DEPARTED + assert batch[0]["detail"] == "the integration lane has no registry credentials" + + +@pytest.mark.parametrize("reason", ["", " ", "\n", None]) +def test_a_departure_with_no_reason_is_never_written(captured, reason): + """THE ONE THAT MATTERS. A `departed` row without its why reads back as a + miss, so writing one would collapse the two states this table exists to + separate — silently, in the readout, where nobody would see it happen. + Dropped and reported, never stored: telemetry that lies is worse than + telemetry that is absent (#2663).""" + rule_usage.record_rule_outcome( + user_id=7, rule_id=156, outcome=DEPARTED, + source="mcp_rule_outcome", detail=reason or "", + ) + assert captured == [] + + +def test_an_unknown_outcome_is_never_written(captured): + """Including the one somebody will reach for. There is no `ignored` + event by design — see `record_rule_outcome` — and a caller inventing one + must not get a row that reads as though the state were measurable.""" + for bogus in ("ignored", "skipped", "surfaced", "", "APPLIED "): + rule_usage.record_rule_outcome( + user_id=7, rule_id=156, outcome=bogus, source="mcp_rule_outcome" + ) + assert captured == [] + + +def test_an_outcome_row_is_one_row(captured): + """A judgement is about one rule. Unlike a surfacing, which delivers a + whole hint at once, there is no batch shape to get wrong here — asserted + so that a later 'helpful' bulk variant has to change a test that says + why.""" + rule_usage.record_rule_outcome( + user_id=7, rule_id=1, outcome=APPLIED, source="mcp_rule_outcome" + ) + [batch] = captured + assert len(batch) == 1 + + +# ── the four states, read off the aggregate ─────────────────────────────── + +def _usage(**kw): + base = rule_usage.empty_rule_usage() + base.update(kw) + return base + + +def test_a_rule_surfaced_and_never_opened_is_unread(): + assert rule_usage.outcome_state(_usage(surfaced_count=4)) == rule_usage.UNREAD + + +def test_a_rule_opened_and_acted_on_is_followed(): + assert rule_usage.outcome_state( + _usage(surfaced_count=4, pull_count=1, applied_count=1) + ) == rule_usage.FOLLOWED + + +def test_a_rule_opened_and_departed_from_is_departed(): + assert rule_usage.outcome_state( + _usage(surfaced_count=4, pull_count=1, departed_count=1) + ) == rule_usage.DEPARTED_FROM + + +def test_a_rule_opened_and_never_acted_on_is_unacted() -> None: + """THE STATE THAT DID NOT EXIST, and the reason for the milestone. Not + "no data": the rule was surfaced, deliberately opened, and then left no + trace of having mattered. Until now that was arithmetically identical to + compliance, which is why nothing could report it.""" + assert rule_usage.outcome_state( + _usage(surfaced_count=4, pull_count=2) + ) == rule_usage.UNACTED + + +def test_unacted_and_followed_are_not_the_same_reading(): + """Stated as its own test because it IS the milestone in one line. If a + change ever makes these two agree, the measurement is gone and every + other test here would still pass.""" + opened_only = _usage(surfaced_count=4, pull_count=2) + opened_and_applied = _usage(surfaced_count=4, pull_count=2, applied_count=1) + assert rule_usage.outcome_state(opened_only) != rule_usage.outcome_state( + opened_and_applied + ) + + +def test_a_departure_outranks_an_application(): + """A rule both applied and argued with is a rule someone argued with, and + the argument is the half worth surfacing. Reporting it as plain + compliance would bury the one row a reader most wants to see.""" + assert rule_usage.outcome_state( + _usage(pull_count=3, applied_count=5, departed_count=1) + ) == rule_usage.DEPARTED_FROM + + +def test_the_state_reads_the_aggregate_the_readout_already_returns(): + """`outcome_state` takes `usage_for_rules`' own shape, so the badge, the + readout and any later session summary cannot disagree about what + "followed" means — the drift #3246 found across the rules system.""" + assert rule_usage.outcome_state(rule_usage.empty_rule_usage()) == rule_usage.UNREAD