From 8b9b3a1d9b985a84c4efe4bda0aeff5c8ca3bd90 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 2 Sep 2026 23:06:40 -0400 Subject: [PATCH 1/2] feat(telemetry): the preload emits, and the always-on set stops being unfalsifiable (#3473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ranked rule arm became measurable in M333. The preload did not — and that is the surface whose value is actually in question. `list_always_on_rules`, the SessionStart block and every `rules_payload` caller handed rules over wholesale and emitted nothing, so the resident set's token cost was certain and its usefulness could not be tested even in principle. Bulk deliveries now record as AMBIENT, beside the ranked count and never inside pull-through. Folding them in would mean growing the always-on set depressed the arm's measured precision and trimming it flattered the arm, neither for any reason to do with the arm. `RANKED_SOURCES` inverts the note twin's `AMBIENT_SOURCES` deliberately: there is one ranked rule source and this change adds seven bulk ones, so naming the rare half makes a forgotten surface default to ambient — under-counting it — rather than padding the denominator with surfacings nobody chose. Two lookalike call sites are deliberately left silent, with a test to keep them that way: the write-path etag arm and `rules_etag_for` read the rules to build or compare a MARKER and show nobody anything. No migration — `event` and `source` are plain Text with no CHECK (rule 36 does not apply). Snippet #2858 updated to the new `rules_payload` contract. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- src/scribe/mcp/tools/milestones.py | 2 +- src/scribe/mcp/tools/projects.py | 4 +- src/scribe/mcp/tools/rulebooks.py | 11 +- src/scribe/mcp/tools/search.py | 26 +++- src/scribe/mcp/tools/tasks.py | 2 +- src/scribe/services/planning.py | 2 +- src/scribe/services/plugin_context.py | 18 +++ src/scribe/services/retrieval_telemetry.py | 47 +++++-- src/scribe/services/rule_usage.py | 135 +++++++++++++++++---- src/scribe/services/rulebooks.py | 30 ++++- tests/test_inception_rules.py | 7 +- tests/test_rule_usage_wiring.py | 127 +++++++++++++++++++ tests/test_services_retrieval_telemetry.py | 65 ++++++++++ tests/test_services_rule_usage.py | 77 ++++++++++++ 14 files changed, 505 insertions(+), 48 deletions(-) diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 39b144d..cde1687 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -57,7 +57,7 @@ async def get_milestone(milestone_id: int) -> dict: return { "milestone": out, "steps": [t.to_dict() for t in steps], - **rulebooks_svc.rules_payload(applicable), + **rulebooks_svc.rules_payload(applicable, user_id=uid, source="get_milestone"), } diff --git a/src/scribe/mcp/tools/projects.py b/src/scribe/mcp/tools/projects.py index b2620d6..8f90d57 100644 --- a/src/scribe/mcp/tools/projects.py +++ b/src/scribe/mcp/tools/projects.py @@ -207,7 +207,7 @@ async def enter_project(project_id: int) -> dict: ], "design_system": design_system, "milestone_summary": milestone_summary, - **rulebooks_svc.rules_payload(applicable), + **rulebooks_svc.rules_payload(applicable, user_id=uid, source="enter_project"), "open_tasks": [ { "id": t.id, "title": t.title, "status": t.status, @@ -251,7 +251,7 @@ async def get_project(project_id: int) -> dict: applicable = await rulebooks_svc.get_applicable_rules( project_id=project_id, user_id=uid, ) - data.update(rulebooks_svc.rules_payload(applicable)) + data.update(rulebooks_svc.rules_payload(applicable, user_id=uid, source="get_project")) return data diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 178cbbe..e21fdb2 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -18,7 +18,7 @@ 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_pulled, record_rule_surfaced # ── Rulebook CRUD ─────────────────────────────────────────────────────── @@ -265,6 +265,15 @@ async def list_always_on_rules(project_id: int = 0) -> dict: """ uid = current_user_id() rules = await rulebooks_svc.list_always_on_rules(uid, project_id=project_id) + # AMBIENT source: the resident set, handed over whole. No ranker chose + # these, so they must not land in the pull-through numerator's denominator + # — but they must land SOMEWHERE, or the largest rule surface in the + # product stays the one surface its own scoreboard cannot see (#3473). + record_rule_surfaced( + user_id=uid, + rule_ids=[r.id for r in rules], + source="list_always_on_rules", + ) return { "rules": [_rule_summary(r) for r in rules], "total": len(rules), diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index e00730f..a686a1c 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -197,17 +197,31 @@ It is an UPPER BOUND per surface: a pull records the door it came query failed while the rest of the readout stood. `rule_usage` — the same question for RULES, from `rule_usage_events`: - `surfaced`, `pulled` split into `pulled_by_agent` / `pulled_by_human`, the - distinct-rule counts, and `pull_through` on the same definition (agent - pulls over surfacings). + `surfaced` and `ambient`, `pulled` split into `pulled_by_agent` / + `pulled_by_human`, the distinct-rule counts, and `pull_through` on the same + definition (agent pulls over RANKED surfacings). A SEPARATE BLOCK, not folded into `usage`, and reading it as one number with that is the mistake to avoid. The corpora differ by orders of magnitude — a few dozen eligible rules against thousands of notes — so a blended ratio would be the note ratio with noise on it and would hide the - rule arm entirely. It also has no `ambient` key, because nothing surfaces a - rule un-ranked: `list_always_on_rules` and `enter_project` hand over rules - wholesale but emit no event, so there is no ambient class to separate. + rule arm entirely. + + `surfaced` VS `ambient` IS THE READING THAT MATTERS HERE. `surfaced` counts + rules a ranker chose — today only the write-path arm — and those are claims + a pull can settle. `ambient` counts BULK DELIVERIES: the SessionStart + preload, `list_always_on_rules`, and the `rules_payload` surfaces + (`enter_project`, `get_project`, `get_milestone`, `start_planning`, + `get_task`), which hand over the whole applicable set at once with nobody + choosing anything. A large `ambient` says the resident set is big and + arrives often — never that it is useful, and never that it is read. + + `pull_through` therefore divides by `surfaced` alone. Fold the preload in + and growing the always-on set would depress the arm's measured precision + while trimming it would flatter it, for reasons having nothing to do with + the arm. To judge the PRELOAD instead, compare `ambient` against pulls of + those same rules over time: a resident set surfaced thousands of times and + opened never is the dead-weight signal, one tier up. Read it against `sources["write_path_rule"]`. That surface has never once declined to fire, and until this block existed there was no way to tell a diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index e097292..32eef56 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -103,7 +103,7 @@ async def get_task(task_id: int) -> dict: applicable = await rulebooks_svc.get_applicable_rules( project_id=note.project_id, user_id=uid, ) - data.update(rulebooks_svc.rules_payload(applicable)) + data.update(rulebooks_svc.rules_payload(applicable, user_id=uid, source="get_task")) data.update(await access_svc.describe_provenance(uid, note)) # Same reasoning as get_note's record_pulled, and this is the tool where it # matters MOST: auto-inject ranks kind-blind over a corpus that is diff --git a/src/scribe/services/planning.py b/src/scribe/services/planning.py index 4f58c97..d7addeb 100644 --- a/src/scribe/services/planning.py +++ b/src/scribe/services/planning.py @@ -60,7 +60,7 @@ async def start_planning(user_id: int, project_id: int, title: str) -> dict: return { "milestone": milestone.to_dict(), - **rulebooks_svc.rules_payload(applicable), + **rulebooks_svc.rules_payload(applicable, user_id=user_id, source="start_planning"), "project_goal": getattr(project, "goal", "") or "", "open_task_count": open_count, } diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 48ccf2f..2dd6d1f 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -1375,6 +1375,24 @@ async def build_session_context( # exclusion (milestone 297) takes a rulebook out of this block, and is # named below so the departure is visible rather than silent. rules = await rulebooks_svc.list_always_on_rules(user_id, project_id=project_id) + # AMBIENT source, and the one that matters most: this is the preload — the + # block every session opens with, chosen by nobody, paid for every turn. + # + # It emitted nothing until 2026-09-03, which made the resident set's cost + # certain and its usefulness unfalsifiable at the same time (#3473). Note + # #3089 is the argument this measurement finally lets someone test: that a + # rule arriving with thirty others, none of them relevant, is read as + # preamble rather than as a claim — so presence is not surfacing, and a + # tier-1 set can grow without anybody noticing it stopped working. + # + # Recorded even when the hook truncates the block below: the rules WERE + # delivered, and counting only the untruncated ones would quietly shrink + # the denominator exactly where the set is too big to read. + record_rule_surfaced( + user_id=user_id, + rule_ids=[r.id for r in rules], + source="session_start", + ) excluded = ( await rulebooks_svc.excluded_always_on_rulebooks(user_id, project_id) if project_id else [] diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index af27dda..e21629a 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -30,6 +30,7 @@ from scribe.models.note_usage import PULLED, SURFACED, NoteUsageEvent from scribe.models.rule_usage import PULLED as RULE_PULLED from scribe.models.rule_usage import SURFACED as RULE_SURFACED from scribe.models.rule_usage import RuleUsageEvent +from scribe.services.rule_usage import is_ambient from scribe.models.retrieval_log import RetrievalLog logger = logging.getLogger(__name__) @@ -428,10 +429,15 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict: .group_by(RuleUsageEvent.event, RuleUsageEvent.source) ) ).all() - # No AMBIENT exclusion here, unlike the note twin: nothing - # surfaces a rule un-ranked yet. `list_always_on_rules` and - # `enter_project` deliver rules wholesale but emit no event, so - # there is no ambient class to subtract (milestone 333 step 1). + # The rows carry `source`, so the ranked/ambient split is done + # below rather than in SQL — the bulk surfaces started emitting + # on 2026-09-03 (#3473), so there IS an ambient class now. + # + # `distinct_rules_surfaced` deliberately counts BOTH classes. It + # answers "how many distinct rules did this install put in front + # of an agent at all", which is the denominator for dead weight + # — and a rule delivered by the preload a hundred times and + # never opened is the most important case that question has. distinct_rules_surfaced = ( await session.execute( select(func.count(func.distinct(RuleUsageEvent.rule_id))) @@ -545,11 +551,18 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict: # been comparing across windows, without telling them it now measures # something else. # - # No `ambient` key, unlike its twin. Nothing surfaces a rule un-ranked yet; - # the absence is a fact about the data rather than an oversight, and it - # returns the moment a bulk loader starts emitting. + # `ambient` now carries the bulk deliveries — the SessionStart preload, + # `list_always_on_rules`, and every `rules_payload` surface (#3473). Before + # they emitted, this block had no ambient key and said the absence was a + # fact about the data. It was, and it was also the thing that made the + # always-on set impossible to judge: the largest rule surface in the + # product was the one surface its own scoreboard could not see. + # + # READ THE TWO SEPARATELY, ALWAYS. `surfaced` is a claim a ranker made and + # a pull can settle. `ambient` is a delivery nobody chose, so a high count + # says the set is large and resident, never that it is useful. rule_usage = { - "surfaced": 0, + "surfaced": 0, "ambient": 0, "pulled": 0, "pulled_by_agent": 0, "pulled_by_human": 0, "distinct_rules_surfaced": int(distinct_rules_surfaced or 0), "distinct_rules_pulled": int(distinct_rules_pulled or 0), @@ -565,7 +578,14 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict: for event, source, n in rule_rows: n = int(n) if event == RULE_SURFACED: - rule_usage["surfaced"] += n + # One definition of ranked-vs-ambient, imported rather than + # restated — the per-rule badge readout reads the same + # predicate, and two spellings of "what counts as surfaced" is + # precisely the uneven wiring #3246 found across this system. + if is_ambient(source): + rule_usage["ambient"] += n + else: + rule_usage["surfaced"] += n elif event == RULE_PULLED: rule_usage["pulled"] += n # Same split, and it carries MORE weight here than for notes. @@ -582,6 +602,15 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict: # empty numerator AND denominator that is a claim the data does not # support, and it is the reading that would make a brand-new install look # like a broken one. + # + # RANKED SURFACINGS ONLY in the denominator, and this is the load-bearing + # line of the whole change. Pull-through asks "was that hint any use", and + # only a surface that CHOSE what it showed can be judged by it. Folding the + # preload in would divide the same pulls by a number that grows with every + # session and every rule added to the resident set — so enlarging the + # always-on set would DEPRESS the arm's measured precision, and trimming it + # would flatter it, neither for any reason to do with the arm. The ambient + # count sits beside it, unaveraged, and is read as size rather than skill. rule_usage["pull_through"] = ( round(rule_usage["pulled_by_agent"] / rule_usage["surfaced"], 4) if rule_usage["surfaced"] else None diff --git a/src/scribe/services/rule_usage.py b/src/scribe/services/rule_usage.py index 061005b..f874d94 100644 --- a/src/scribe/services/rule_usage.py +++ b/src/scribe/services/rule_usage.py @@ -30,23 +30,43 @@ Design notes, mirroring `note_usage`: - Reads (`usage_for_rules`) are awaited and aggregated in one round-trip for a whole page, never per row. -NO AMBIENT BUCKET, YET — and that is a decision, not an omission. The note twin -splits ranked surfacings from ambient ones because `enter_project` and the -skill sync put records in front of the agent without choosing them, and -counting those as surfacings makes recency read as popularity (#2477). Rules -have the same shape of problem waiting: `list_always_on_rules` and -`enter_project` load rules wholesale on every session. They do not emit here -today, so there is nothing to bucket, and an empty `AMBIENT_SOURCES` would be -machinery pretending to a distinction the data does not yet contain. When a -bulk surface starts emitting, the split is a readout-level change — a tuple and -a `case()`, exactly as in the twin — and needs no migration. Keep it that way: -`source` stays granular so the choice remains available. +AMBIENT VS RANKED. The note twin splits ranked surfacings from ambient ones +because `enter_project` and the skill sync put records in front of the agent +without choosing them, and counting those as surfacings makes recency read as +popularity (#2477). Rules have exactly that shape: the SessionStart preload, +`list_always_on_rules`, and every `rules_payload` surface hand over the whole +applicable set at once, chosen by nobody. + +Until 2026-09-03 those bulk surfaces emitted nothing, and this module said so — +"an empty `AMBIENT_SOURCES` would be machinery pretending to a distinction the +data does not yet contain". True as far as it went, but it had a consequence +worth naming, because it is the reason the bucket exists now: the always-on +set's token cost was certain and its usefulness was UNFALSIFIABLE, permanently +and by construction. The one surface whose value was actually in question was +the one surface exempt from the scoreboard that judges every other. + +They emit now. The split is the readout-level change the old note promised — a +`case()`, no migration, because `event` and `source` are plain Text with no +CHECK constraint. `source` stays granular so a reader can still tell the +preload from `enter_project` from the ranked arm. + +WHY THIS NAMES THE RANKED SOURCES AND THE TWIN NAMES THE AMBIENT ONES. A +deliberate divergence, on the failure mode rather than on symmetry. Both shapes +fail silently when someone adds a surface and forgets the list, so the question +is which list changes more often — and here it is emphatically the ambient one: +there is exactly ONE ranked rule source, and this change alone adds seven bulk +ones. Naming the rare, stable half means a newly-added bulk surface defaults to +`ambient`, which merely under-counts it, instead of defaulting to `ranked`, +which would quietly pad the pull-through denominator with surfacings nobody +chose and make the arm look imprecise. Same argument #3191 and #3430 make +against hand-kept lists: keep the list that must be remembered as short and as +slow-moving as possible. """ from __future__ import annotations import logging -from sqlalchemy import func, select +from sqlalchemy import case, func, select from scribe.models import async_session from scribe.models.base import iso @@ -55,6 +75,31 @@ from scribe.services.background import report_telemetry_failure, spawn logger = logging.getLogger(__name__) +# The surfaces that CHOSE the rules they showed. Everything else is ambient — +# see the module docstring for why the rare half is the half that gets named. +# +# Membership is the whole definition of the pull-through denominator: a ranked +# surfacing is a claim ("this rule may apply to what you are doing") that a pull +# can confirm or refute, while an ambient one is a delivery nobody decided on. +# Add a source here only when a ranker picked it. +RANKED_SOURCES = ("write_path_rule",) + + +def is_ambient(source: str) -> bool: + """Was this surfacing a bulk delivery rather than a ranked choice? + + One definition, read by both the per-rule badge readout and the aggregate + in `retrieval_telemetry` — the two used to be able to disagree about what + "surfaced" counted, which is the class of drift #3246 found across the + rules system. + + Sync and pure, per the service canon (#2860), but deliberately PUBLIC where + that canon says such helpers stay `_private`. The departure is the point: + a module-private copy in each caller is exactly the second definition this + exists to prevent. + """ + return source not in RANKED_SOURCES + async def _report_failure(site: str) -> None: await report_telemetry_failure("rule_usage", site) @@ -81,14 +126,21 @@ def record_rule_surfaced( ) -> None: """Fire-and-forget: record that these rules were shown to the agent. - Takes the whole hint at once — one insert per surfacing event, not per rule - — because a hint is a single decision and its rows should land together. + Takes the whole delivery at once — one insert per surfacing event, not per + rule — because a hint is a single decision and its rows should land + together. - Record the RANKED hits only. The arm filters candidates before it speaks - (`exclude_rule_ids` drops what the session already holds), and a rule that - was considered and not shown was not surfaced. Counting those would inflate - the denominator with claims the agent never saw, which reads as a precision - problem the arm does not have. + Record what was actually SHOWN, never what was considered. For the ranked + arm that means the post-filter hits: it drops what the session already + holds (`exclude_rule_ids`) before it speaks, and a rule considered and not + shown was not surfaced. Counting those would inflate the denominator with + claims the agent never saw, which reads as a precision problem the arm does + not have. + + Bulk surfaces pass their whole delivered set, which is the same rule read + from the other end — everything in a preload IS shown. `source` is what + separates the two afterwards (see `RANKED_SOURCES`); this function does not + care which kind it is recording. """ try: rows = [ @@ -137,9 +189,17 @@ def empty_rule_usage() -> dict: distinction matters more here than for notes: every rule in an install predates this table, so for a while "no events" is the normal state and it must not look like a broken readout. + + `surfaced_count` is RANKED surfacings only; `ambient_count` is the bulk + deliveries (see `RANKED_SOURCES`). The split is what keeps the badge's + "shown often, opened never → dead weight" reading honest: every rule in an + always-on set is delivered every session, so an unsplit counter would rank + the resident set as the most-surfaced rules in the install purely for being + resident. """ return { "surfaced_count": 0, + "ambient_count": 0, "pull_count": 0, "last_surfaced_at": None, "last_pulled_at": None, @@ -159,6 +219,19 @@ async def usage_for_rules(rule_ids: list[int]) -> dict[int, dict]: if not ids: return out + # Classified in SQL so the group stays small: per rule we get at most + # (surfaced-ranked, surfaced-ambient, pulled) rather than a row per distinct + # source. ONE labelled expression, bound to a variable and reused in the + # GROUP BY — a second `case()` instance there renders its own expanding-IN + # bind names under asyncpg, so the database sees two DIFFERENT expressions + # and rejects the query with a GroupingError. The note twin carries the + # same warning for the same reason, and #2663 is what it cost: the + # rejection was swallowed and every counter read zero in production while + # the writes were landing fine. + ambient = case( + (RuleUsageEvent.source.notin_(RANKED_SOURCES), True), + else_=False, + ).label("ambient") try: async with async_session() as session: rows = ( @@ -168,9 +241,14 @@ async def usage_for_rules(rule_ids: list[int]) -> dict[int, dict]: RuleUsageEvent.event, func.count().label("n"), func.max(RuleUsageEvent.created_at).label("last_at"), + ambient, ) .where(RuleUsageEvent.rule_id.in_(ids)) - .group_by(RuleUsageEvent.rule_id, RuleUsageEvent.event) + .group_by( + RuleUsageEvent.rule_id, + RuleUsageEvent.event, + ambient, + ) ) ).all() except Exception: @@ -180,14 +258,23 @@ async def usage_for_rules(rule_ids: list[int]) -> dict[int, dict]: await _report_failure("readout") return out - for rule_id, event, n, last_at in rows: + for rule_id, event, n, last_at, is_amb in rows: slot = out.get(int(rule_id)) if slot is None: continue - if event == SURFACED: + if event == SURFACED and is_amb: + slot["ambient_count"] = int(n) + elif event == SURFACED: slot["surfaced_count"] = int(n) slot["last_surfaced_at"] = iso(last_at) elif event == PULLED: - slot["pull_count"] = int(n) - slot["last_pulled_at"] = iso(last_at) + # Pulls are pulls regardless of what surfaced the rule — "did + # anyone ever open this?" does not depend on how it was found. Both + # halves accumulate, so this ADDS rather than assigns: a rule can + # now be pulled after a ranked hint and after a preload, and the + # split arrives as two rows. + slot["pull_count"] = slot["pull_count"] + int(n) + latest = iso(last_at) + if latest and (slot["last_pulled_at"] or "") < latest: + slot["last_pulled_at"] = latest return out diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index ba11b69..c8e380e 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -23,6 +23,7 @@ from scribe.services.verification import ( ) from scribe.services import rule_versions from scribe.models.rule_version import RuleVersion +from scribe.services.rule_usage import record_rule_surfaced logger = logging.getLogger(__name__) @@ -1395,7 +1396,7 @@ async def get_applicable_rules( } -def rules_payload(applicable: dict) -> dict: +def rules_payload(applicable: dict, *, user_id: int | None, source: str) -> dict: """The caller-facing shape of a get_applicable_rules() result. Every surface that hands rules to an agent (enter_project, get_project, @@ -1406,7 +1407,34 @@ def rules_payload(applicable: dict) -> dict: `excluded_always_on` (milestone 297) names the always-on rulebooks this project decided NOT to inherit, so the departure is visible wherever the rules are. + + IT ALSO RECORDS THE SURFACING, which is why it now takes a caller and a + source. Every one of those surfaces is a bulk delivery — the applicable set + handed over whole, chosen by nobody — so this is the one place that has to + emit for all of them. Doing it per-caller instead would be five sites to + remember, and #3430 gap 2 is what that costs: the process→skill sync went + un-emitted through an entire dedicated telemetry survey because nothing + forced its surface to be accounted for. + + `source` stays the CALLER's name rather than a constant, so the readout can + still separate the session handshake from a mid-session milestone read; + `RANKED_SOURCES` in `rule_usage` is what folds them back together. + + Emitting from here is safe in a way emitting from `get_applicable_rules` + would not be: this function is only ever called to BUILD A REPLY. The two + other callers of the rules machinery — the write-path etag arm + (`plugin_context`) and `rules_etag_for` — compute a marker and show nobody + anything, and counting those would put rules in the denominator that no + agent ever saw. """ + record_rule_surfaced( + user_id=user_id, + rule_ids=( + [r["id"] for r in applicable.get("rules", [])] + + [r["id"] for r in applicable.get("project_rules", [])] + ), + source=source, + ) return { "applicable_rules": applicable["rules"], "applicable_rules_truncated": applicable["truncated"], diff --git a/tests/test_inception_rules.py b/tests/test_inception_rules.py index 4f1172f..6a614e6 100644 --- a/tests/test_inception_rules.py +++ b/tests/test_inception_rules.py @@ -15,14 +15,17 @@ def test_rules_payload_carries_excluded_always_on_as_the_seventh_key(): out = rules_payload({ "rules": [], "truncated": False, "subscribed_rulebooks": [], "excluded_always_on": [{"id": 1, "title": "Family"}], - }) + }, user_id=1, source="enter_project") assert set(out) == { "applicable_rules", "applicable_rules_truncated", "subscribed_rulebooks", "project_rules", "suppressed_rules", "suppressed_topics", "excluded_always_on", } assert out["excluded_always_on"] == [{"id": 1, "title": "Family"}] # An older applicable dict without the key still renders (empty list). - assert rules_payload({"rules": [], "truncated": False, "subscribed_rulebooks": []})["excluded_always_on"] == [] + assert rules_payload( + {"rules": [], "truncated": False, "subscribed_rulebooks": []}, + user_id=1, source="enter_project", + )["excluded_always_on"] == [] def test_list_always_on_rules_service_and_tool_take_a_project_id(): diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index 7d4c667..5ccd960 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -274,3 +274,130 @@ def test_the_bulk_loaders_are_not_counted_as_pulls(): "applicable rule at once — so counting it would drown the " "surfaced:pulled ratio in ambient delivery." ) + + +# ── The AMBIENT end: bulk deliveries (#3473) ─────────────────────────── +# +# The preload was the largest rule surface in the product and emitted nothing, +# so its cost was certain and its usefulness unfalsifiable. These assert the +# three delivery shapes now emit — and, just as importantly, that the two +# lookalike call sites which show nobody anything do NOT. + + +@pytest.mark.asyncio +async def test_the_session_start_preload_records_what_it_delivered(): + """The block every session opens with. Chosen by nobody, paid for every + turn — and until it emitted, invisible to the scoreboard that judges every + other surface.""" + from scribe.services import plugin_context as pc + + rec = MagicMock() + rules = [fake_rule(id=1, title="`dev` is home"), + fake_rule(id=2, title="`main` — never without explicit request")] + with ExitStack() as stack: + stack.enter_context( + patch.object(pc.rulebooks_svc, "list_always_on_rules", + AsyncMock(return_value=rules)) + ) + stack.enter_context( + patch.object(pc.rulebooks_svc, "excluded_always_on_rulebooks", + AsyncMock(return_value=[])) + ) + stack.enter_context(patch.object(pc, "record_rule_surfaced", rec)) + stack.enter_context( + patch.object(pc, "_topic_titles", AsyncMock(return_value={})) + ) + await pc.build_session_context(1, project_id=0) + + assert rec.call_count == 1, "the preload recorded nothing" + kw = rec.call_args.kwargs + assert kw["rule_ids"] == [1, 2] + assert kw["source"] == "session_start" + + +@pytest.mark.asyncio +async def test_the_always_on_tool_records_what_it_handed_over(): + from scribe.mcp.tools import rulebooks as tools + + rec = MagicMock() + rules = [fake_rule(id=3, title="No GitHub — Fabled-Git only")] + with ExitStack() as stack: + stack.enter_context( + patch.object(tools.rulebooks_svc, "list_always_on_rules", + AsyncMock(return_value=rules)) + ) + stack.enter_context( + patch.object(tools.rulebooks_svc, "rules_etag", + MagicMock(return_value="etag")) + ) + stack.enter_context(patch.object(tools, "record_rule_surfaced", rec)) + await tools.list_always_on_rules() + + assert rec.call_args.kwargs["rule_ids"] == [3] + assert rec.call_args.kwargs["source"] == "list_always_on_rules" + + +def test_rules_payload_records_both_the_family_and_project_halves(): + """One emit site for all five `rules_payload` surfaces. + + Per-caller emission would be five sites to remember, and #3430 gap 2 is + what that costs: the process→skill sync went un-emitted through an entire + dedicated telemetry survey because nothing forced its surface to be + accounted for. + """ + from scribe.services import rulebooks as svc + + rec = MagicMock() + with patch.object(svc, "record_rule_surfaced", rec): + svc.rules_payload( + { + "rules": [{"id": 10}, {"id": 11}], + "project_rules": [{"id": 12}], + "truncated": False, + "subscribed_rulebooks": [], + }, + user_id=1, + source="enter_project", + ) + + kw = rec.call_args.kwargs + assert kw["rule_ids"] == [10, 11, 12], "project-scoped rules were delivered too" + assert kw["source"] == "enter_project" + + +def test_every_rules_payload_caller_names_itself(): + """`source` is the CALLER's name, so the readout can still separate the + session handshake from a mid-session milestone read. A shared constant here + would collapse five distinguishable surfaces into one.""" + import re + + seen = set() + for path in Path("src/scribe").rglob("*.py"): + for m in re.finditer(r"rules_payload\([^)]*source=\"([a-z_]+)\"", path.read_text()): + seen.add(m.group(1)) + assert seen == { + "enter_project", "get_project", "get_milestone", + "start_planning", "get_task", + }, f"a rules_payload caller is missing or misnamed: {sorted(seen)}" + + +def test_the_marker_paths_stay_silent(): + """The two call sites that read the rules and show NOBODY anything. + + `rules_etag_for` and the write-path staleness arm both call + `list_always_on_rules` to build or compare a marker. Emitting there would + put rules in the denominator that no agent ever saw — the exact inflation + `record_rule_surfaced`'s docstring forbids, arriving from the one direction + nothing else guards. + """ + svc_src = Path("src/scribe/services/rulebooks.py").read_text() + etag_fn = svc_src.split("async def rules_etag_for")[1].split("\ndef ")[0] + assert "record_rule_surfaced" not in etag_fn, ( + "rules_etag_for emits a surfacing — it builds a marker, it shows nothing" + ) + + pc_src = Path("src/scribe/services/plugin_context.py").read_text() + staleness = pc_src.split("if rules_etag:")[1].split("# The guard sits BELOW")[0] + assert "record_rule_surfaced" not in staleness, ( + "the staleness arm emits a surfacing — it compares a marker, it shows nothing" + ) diff --git a/tests/test_services_retrieval_telemetry.py b/tests/test_services_retrieval_telemetry.py index e5b529b..1bea2ca 100644 --- a/tests/test_services_retrieval_telemetry.py +++ b/tests/test_services_retrieval_telemetry.py @@ -511,3 +511,68 @@ async def test_rule_usage_sees_only_its_own_users_events(_dispose_engine): assert (await retrieval_summary(990014, days=30))["rule_usage"]["surfaced"] == 1 finally: await cleanup() + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_the_preload_lands_in_ambient_and_never_in_the_ratio(_dispose_engine): + """The split that makes the always-on set judgeable (#3473). + + Pull-through asks "was that hint any use", and only a surface that CHOSE + what it showed can be judged by it. If the preload counted toward the + denominator, growing the always-on set would DEPRESS the arm's measured + precision and trimming it would flatter it — neither for any reason to do + with the arm. So the resident deliveries are counted, reported, and kept + out of the ratio. + """ + from scribe.services.retrieval_telemetry import retrieval_summary + + cleanup = await _rule_events(990012, [ + # One rule the arm actually chose, and opened. + (5101, "surfaced", "write_path_rule"), + (5101, "pulled", "mcp_get_rule"), + # Four bulk deliveries across every shape of preload. Nobody chose any + # of them, and none may touch the denominator. + (5102, "surfaced", "session_start"), + (5103, "surfaced", "list_always_on_rules"), + (5104, "surfaced", "enter_project"), + (5105, "surfaced", "get_milestone"), + ]) + try: + ru = (await retrieval_summary(990012, days=30))["rule_usage"] + + assert ru["surfaced"] == 1, "only the arm chose a rule" + assert ru["ambient"] == 4, "the four bulk deliveries are reported, not dropped" + + # 1 agent pull over 1 RANKED surfacing. Were the ambient four folded in + # the ratio would read 0.2 — the arm looking four times worse for + # having a large resident set beside it. + assert ru["pull_through"] == 1.0 + + # Dead-weight detection needs both classes: a rule delivered by the + # preload and never opened is the case that reading matters most for. + assert ru["distinct_rules_surfaced"] == 5 + assert ru["distinct_rules_pulled"] == 1 + finally: + await cleanup() + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_ambient_alone_reports_no_ratio(_dispose_engine): + """A brand-new install loads rules every session and may never trigger the + arm. That must read as "no ranked surfacings yet", not as a precision of + zero — the reading that would make a working install look broken.""" + from scribe.services.retrieval_telemetry import retrieval_summary + + cleanup = await _rule_events(990013, [ + (5201, "surfaced", "session_start"), + (5202, "surfaced", "session_start"), + ]) + try: + ru = (await retrieval_summary(990013, days=30))["rule_usage"] + assert ru["ambient"] == 2 + assert ru["surfaced"] == 0 + assert ru["pull_through"] is None + finally: + await cleanup() diff --git a/tests/test_services_rule_usage.py b/tests/test_services_rule_usage.py index 6efb23c..95cbe8b 100644 --- a/tests/test_services_rule_usage.py +++ b/tests/test_services_rule_usage.py @@ -99,12 +99,32 @@ def test_the_zero_readout_names_every_key(): — a missing key here would read as a broken readout on almost every row.""" assert rule_usage.empty_rule_usage() == { "surfaced_count": 0, + "ambient_count": 0, "pull_count": 0, "last_surfaced_at": None, "last_pulled_at": None, } +def test_only_a_ranker_counts_as_ranked(): + """The bulk surfaces are ambient; the write-path arm is the only chooser. + + Inverted against the note twin on purpose (see the module docstring): the + RARE half is the one that gets named, so a bulk surface added later and + forgotten defaults to ambient — under-counting it — instead of defaulting + to ranked and padding the pull-through denominator with surfacings nobody + chose. + """ + assert not rule_usage.is_ambient("write_path_rule") + for bulk in ( + "session_start", "list_always_on_rules", "enter_project", + "get_project", "get_milestone", "start_planning", "get_task", + ): + assert rule_usage.is_ambient(bulk), bulk + # The safe default is the whole point of the inversion. + assert rule_usage.is_ambient("some_surface_invented_next_year") + + def test_the_model_serialises_the_fields_the_ratio_needs(): ev = RuleUsageEvent( user_id=7, rule_id=156, event=SURFACED, source="write_path_rule" @@ -161,6 +181,10 @@ async def test_usage_for_rules_aggregates_per_rule(_dispose_engine): # never have to tell "no events" from "not in the result" — and on any # existing install that is nearly every rule. assert out[6003] == rule_usage.empty_rule_usage() + + # Nothing ambient in this fixture, so the ambient bucket stays empty + # rather than absorbing the ranked hits. + assert out[6001]["ambient_count"] == 0 finally: async with async_session() as s: await s.execute( @@ -178,3 +202,56 @@ async def test_usage_for_rules_on_an_empty_id_list_asks_the_database_nothing( empty topic is nothing. An unguarded `IN ()` is both a pointless round trip and, on some drivers, a syntax error.""" assert await rule_usage.usage_for_rules([]) == {} + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_a_preloaded_rule_does_not_read_as_a_ranked_surfacing(_dispose_engine): + """The split that makes the always-on set judgeable (#3473). + + A resident rule is delivered every session by a surface that chose + nothing. Counting those as `surfaced_count` would rank the always-on set + as the most-surfaced rules in the install purely for being resident — and + the badge's "shown often, opened never → dead weight" reading, which is + the whole reason the counter exists, would then be exactly backwards. + """ + from sqlalchemy import delete + + from scribe.models import async_session + from scribe.models.rule_usage import RuleUsageEvent + + async with async_session() as s: + s.add_all([ + # Delivered by the preload three times over: ambient, all of it. + RuleUsageEvent(user_id=990021, rule_id=6101, + event=SURFACED, source="session_start"), + RuleUsageEvent(user_id=990021, rule_id=6101, + event=SURFACED, source="list_always_on_rules"), + RuleUsageEvent(user_id=990021, rule_id=6101, + event=SURFACED, source="enter_project"), + # ...and once by the arm, which DID choose it. + RuleUsageEvent(user_id=990021, rule_id=6101, + event=SURFACED, source="write_path_rule"), + # Opened once after a hint and once from the list: pulls are pulls + # however the rule was found, so both land in the one counter. + RuleUsageEvent(user_id=990021, rule_id=6101, + event=PULLED, source="mcp_get_rule"), + RuleUsageEvent(user_id=990021, rule_id=6101, + event=PULLED, source="rest_rule"), + ]) + await s.commit() + try: + out = await rule_usage.usage_for_rules([6101]) + + assert out[6101]["surfaced_count"] == 1, "only the arm chose this rule" + assert out[6101]["ambient_count"] == 3, "three bulk deliveries" + # Both PULLED rows accumulate — the loop ADDS rather than assigns, so a + # rule opened after a hint and again from the list reports two, not one. + assert out[6101]["pull_count"] == 2 + assert out[6101]["last_pulled_at"] is not None + finally: + async with async_session() as s: + await s.execute( + delete(RuleUsageEvent).where(RuleUsageEvent.user_id == 990021) + ) + await s.commit() -- 2.54.0 From 2ee24b9d2b7fa9c5d0f38ab279852792c0138974 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 2 Sep 2026 23:31:22 -0400 Subject: [PATCH 2/2] =?UTF-8?q?feat(rules):=20rules=20before=20tools=20?= =?UTF-8?q?=E2=80=94=20a=20PreToolUse=20arm=20keyed=20on=20the=20action=20?= =?UTF-8?q?(#3476)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only just-in-time rule surface was registered on `Write|Edit` and queried with `code or path`, so a rule could be retrieved at the moment of a code write and nowhere else. Every rule about which tool to reach for — don't curl the forge, don't stand up a stack, don't run the suite locally, don't branch — was unreachable exactly when it mattered, and residency in the always-on preload was the only surface it had. That is the pressure that grew the resident set to 31 against #3089's ceiling of ~23; it was never a judgment anybody made. A reflex generates no query, so an instruction to check the rules cannot catch one. A mechanical trigger can: the tool call IS the query, and a reflex has to become a tool call before it can do anything. `build_tool_rule_hint` is deliberately tool-agnostic — a name and a string — so widening the matcher later is a hooks.json edit with no server change. The hook starts on Bash, which is where the action reflexes live. The two pre-tool arms share ONE session ledger of already-named rules (`/.rules.ids`). Two ledgers would mean a rule named by one arm gets re-offered by the other, and the hint that fires most often is exactly the one that must not repeat itself. A test asserts both scripts build the same path, and another checks the shell hook and the Python route agree on every query-arg name (rule 33) — a rename there fails silently, looking like a surface that never finds anything rather than a broken one. Deliberately silent on outage, unlike the prior-art hook: a write is occasional, a Bash call is not, and an outage line before every command is what gets a channel muted. `tier="conditional"` matches the write arm and is the transition point — an always-on rule is already resident, so re-tier one and it starts arriving here instead of in every session's preamble. `pre_tool_rule` joins RANKED_SOURCES: this arm chose what it showed, so a pull can settle whether the choice landed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/hooks.json | 9 ++ plugin/hooks/scribe_tool_rules.sh | 116 +++++++++++++++++ scripts/check_plugin.py | 10 ++ src/scribe/routes/plugin.py | 40 ++++++ src/scribe/services/plugin_context.py | 104 +++++++++++++++ src/scribe/services/rule_usage.py | 9 +- tests/test_rule_usage_wiring.py | 176 ++++++++++++++++++++++++++ 8 files changed, 462 insertions(+), 4 deletions(-) create mode 100644 plugin/hooks/scribe_tool_rules.sh diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index c462aa9..f0f1eaf 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "2026.09.02.0438", + "version": "2026.09.03.0329", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/hooks.json b/plugin/hooks/hooks.json index 93a9299..0bec8ea 100644 --- a/plugin/hooks/hooks.json +++ b/plugin/hooks/hooks.json @@ -33,6 +33,15 @@ "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/scribe_prior_art.sh\"" } ] + }, + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/scribe_tool_rules.sh\"" + } + ] } ], "PostToolUse": [ diff --git a/plugin/hooks/scribe_tool_rules.sh b/plugin/hooks/scribe_tool_rules.sh new file mode 100644 index 0000000..6803a78 --- /dev/null +++ b/plugin/hooks/scribe_tool_rules.sh @@ -0,0 +1,116 @@ +#!/usr/bin/env bash +# Scribe — PreToolUse rule arm for ACTIONS (#3476). +# +# The sibling of scribe_prior_art.sh. That hook is registered on Write|Edit and +# asks "what is recorded about the file being written". This one asks "does a +# standing rule speak to the command about to be run" — the question nothing +# could ask before, and the reason every rule about which tool to reach for had +# to live in the always-on preload instead. +# +# WHY A HOOK AND NOT AN INSTRUCTION. A reflex generates no query (note #3089): +# you reach for `curl` confidently, with no moment of doubt, so a surface that +# waits to be asked never fires. Here nothing is asked — the tool call IS the +# query, and the reflex has to become a tool call before it can do anything. +# +# SILENT ON OUTAGE, deliberately, unlike the prior-art hook. A write is +# occasional; a Bash call is not, and an "instance did not answer" line before +# every command is the noise that gets a channel muted. scribe_prior_art.sh +# still speaks for both when the instance is down. +# +# Env: +# SCRIBE_URL / SCRIBE_TOKEN override for the settings.json dogfooding path. + +command -v jq >/dev/null 2>&1 || exit 0 +command -v curl >/dev/null 2>&1 || exit 0 + +# PreToolUse delivers { session_id, cwd, tool_name, tool_input: {...}, ... } +event=$(cat 2>/dev/null || true) +tool_name=$(printf '%s' "$event" | jq -r '.tool_name // empty' 2>/dev/null) || exit 0 +session_id=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || session_id="" +event_cwd=$(printf '%s' "$event" | jq -r '.cwd // empty' 2>/dev/null) || event_cwd="" + +[ -n "$tool_name" ] || exit 0 + +# The action, as text. `.command` is Bash's field; the fallbacks let the matcher +# in hooks.json widen to other tools without this script changing — which is the +# whole reason the server side takes a name and a string rather than a schema. +command_text=$(printf '%s' "$event" | jq -r ' + .tool_input.command // + .tool_input.url // + .tool_input.prompt // + empty' 2>/dev/null) || command_text="" + +[ -n "$command_text" ] || exit 0 + +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + +# scribe_config, not a hand-rolled pair of parameter expansions: it also treats +# an UNEXPANDED `${...}` placeholder as unset, which would otherwise be sent as +# a garbage Bearer token and 401 on every call (#2198's class). +scribe_config || exit 0 + +# Bounded before encoding: a heredoc or a pasted script can be enormous, and +# the verb and its target — the part a rule is about — sit at the front. The +# server bounds it again; this keeps a huge payload off the wire in the first +# place. `head -c`, never `cut -c`: cut truncates each LINE and caps nothing. +command_text=$(printf '%s' "$command_text" | head -c 2000) + +# -sRr, never -rR: jq -R without -s reads LINE BY LINE, so a multi-line command +# would encode per line and join with raw newlines — an invalid URL. +cmd_enc=$(printf '%s' "$command_text" | jq -sRr '@uri' 2>/dev/null) || exit 0 +tool_enc=$(printf '%s' "$tool_name" | jq -sRr '@uri' 2>/dev/null) || exit 0 + +repo_q="" +lookup_dir=${event_cwd:-${CLAUDE_PROJECT_DIR:-$PWD}} +repo_remote=$(git -C "$lookup_dir" remote get-url origin 2>/dev/null || true) +if [ -n "$repo_remote" ]; then + repo_enc=$(printf '%s' "$repo_remote" | jq -sRr '@uri' 2>/dev/null) || repo_enc="" + [ -n "$repo_enc" ] && repo_q="&repo=${repo_enc}" +fi + +# THE SHARED SESSION LEDGER, and the thing most worth getting right here. +# +# scribe_prior_art.sh keeps the rules it has already named in +# /.rules.ids and passes them as exclude_rule_ids. This hook reads +# and appends to that SAME file rather than keeping its own: two ledgers would +# mean a rule named by one arm gets re-offered by the other, and the hint that +# fires most often is exactly the one that must not repeat itself. +# +# The directory keeps the prior-art name on purpose — renaming it would orphan +# every live session's state for a cosmetic gain. +state_dir="${TMPDIR:-/tmp}/scribe-priorart" +mkdir -p "$state_dir" 2>/dev/null || true +rulefile="" +rule_exclude_q="" +if [ -n "$session_id" ]; then + safe_sid=$(printf '%s' "$session_id" | tr -c 'A-Za-z0-9._-' '_') + rulefile="$state_dir/${safe_sid}.rules.ids" + if [ -f "$rulefile" ]; then + rule_seen=$(tr '\n' ',' < "$rulefile" 2>/dev/null | sed 's/,$//') + [ -n "$rule_seen" ] && rule_exclude_q="&exclude_rule_ids=${rule_seen}" + fi +fi + +# `|| exit 0` here, unlike the prior-art hook: there is no local arm whose +# finding would be discarded, and an outage line before every command is worse +# than silence. See the header. +body=$(curl -fsS --max-time 5 \ + -H "Authorization: Bearer ${token}" \ + "${url%/}/api/plugin/tool-rules?tool=${tool_enc}&command=${cmd_enc}${repo_q}${rule_exclude_q}" 2>/dev/null) || exit 0 + +context=$(printf '%s' "$body" | jq -r '.context // empty' 2>/dev/null) || exit 0 +[ -n "$context" ] || exit 0 + +# Remember what was named so it is not repeated this session. +if [ -n "$rulefile" ]; then + printf '%s' "$body" | jq -r '(.rule_ids // [])[]?' 2>/dev/null >> "$rulefile" || true +fi + +jq -cn --arg ctx "$context" '{ + hookSpecificOutput: { + hookEventName: "PreToolUse", + additionalContext: $ctx + } +}' 2>/dev/null || true +exit 0 diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index dd381ba..d0da1ac 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -305,6 +305,16 @@ SMOKE_EVENTS: dict[str, str] = { "tool_input": {"file_path": "src/x.py", "new_string": f"def {_ABSENT_SYM}():\n pass\n"}} ), + # The pre-tool rule arm (#3476). A real Bash call, and one whose whole + # point is that it looks harmless: reaching for curl against the forge API + # is the reflex the arm exists to catch. With no instance it must stay + # SILENT — it is deliberately not an OUTAGE_SPEAKER, because a Bash call is + # not occasional and an outage line before every command gets the channel + # muted. + "scribe_tool_rules.sh": json.dumps( + {"session_id": "smoke", "cwd": ".", "tool_name": "Bash", + "tool_input": {"command": "curl -s https://example.invalid/api/v1/runs"}} + ), "scribe_sync_processes.sh": json.dumps({"source": "startup"}), "scribe_session_context.sh": json.dumps({"source": "startup"}), # The after-write hook (#2901) diffs the working tree; on CI's clean diff --git a/src/scribe/routes/plugin.py b/src/scribe/routes/plugin.py index b951413..e050018 100644 --- a/src/scribe/routes/plugin.py +++ b/src/scribe/routes/plugin.py @@ -101,6 +101,46 @@ async def autoinject_retrieve(): return jsonify(result) +@plugin_bp.get("/tool-rules") +@login_required +async def pre_tool_rules(): + """Standing rules for the plugin's PreToolUse hook on ACTIONS (#3476). + + Answers "does a recorded rule speak to the command about to be run?" — the + sibling of /prior-art, which can only answer that question about a code + write. Rules about which tool to reach for (don't curl the forge, don't + stand up a stack, don't run the suite locally) had no retrieval surface at + all before this, which is why they all had to live in the resident preload. + + Titles + trigger only, never the statement: the hint says a rule may apply + and hands over `get_rule(id)`. One rule at most (RULEHINT_LIMIT), and empty + most of the time. + + Query: + tool (str) — the tool about to run, e.g. `Bash`. Used in + the hint's wording, not in the search: a + rule is about the action, not the harness. + command (str) — the command about to run; the semantic query. + Absent or blank → empty, no search. + repo (optional) — working repo remote, resolved to the bound + project exactly as /retrieve and /prior-art. + exclude_rule_ids (opt) — comma-separated rule ids already surfaced + this session. SHARED with /prior-art's + ledger on purpose: one session keeps one + list, so a rule named by either arm is not + re-offered by the other. + """ + tool = (request.args.get("tool") or "tool").strip() + command = request.args.get("command") or "" + project_id, _repo, _unbound = await _project_scope() + exclude_rule_ids = _int_list(request.args.get("exclude_rule_ids")) + result = await plugin_ctx_svc.build_tool_rule_hint( + g.user.id, tool, command, + project_id=project_id, exclude_rule_ids=exclude_rule_ids, + ) + return jsonify(result) + + @plugin_bp.get("/prior-art") @login_required async def write_path_prior_art(): diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 2dd6d1f..18bee4e 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -140,6 +140,15 @@ RULEHINT_DEFAULT_THRESHOLD = 0.72 # adds a way to misconfigure the surface (rule 25 cuts both ways). RULEHINT_LIMIT = 1 +# How much of a command reaches the embedding (#3476). A shell call is not a +# file: most are short, and the ones that are not are usually a heredoc or a +# pasted script whose bulk says nothing about which rule applies. The VERB AND +# ITS TARGET sit at the front — `curl https://git.fabledsword.com/api/...`, +# `docker compose up`, `git checkout -b` — and that head is the whole signal. +# Sending the tail as well would push it out of a 512-token window and let a +# heredoc's prose decide the match. +_TOOL_QUERY_CHARS = 400 + # Minimum SUBSTANCE (non-whitespace chars) a payload must carry before the # semantic arm will run at all — the cheap half of the operator's #89 idea # ("a sliding scale between number of characters and semantic threshold"). @@ -1250,6 +1259,101 @@ async def build_write_path_hint( } +async def build_tool_rule_hint( + user_id: int, + tool_name: str, + command: str, + *, + project_id: int = 0, + exclude_rule_ids: list[int] | None = None, +) -> dict: + """Standing rules that may apply to the ACTION about to be taken (#3476). + + The sibling of the write-path rule arm, and the surface that was missing. + That arm is keyed on `code or path`, so a rule can only be retrieved at the + moment of a code WRITE. Every rule about which tool to reach for — don't + curl the forge, don't stand up a stack, don't run the suite locally, don't + branch — was therefore unreachable at the moment it mattered, and residency + in the always-on preload was the only surface it had. + + WHY A MECHANICAL TRIGGER AND NOT AN INSTRUCTION. Note #3089's finding is + that a reflex generates no query: you reach for `curl` confidently, with no + moment of doubt, so any surface that waits to be asked never fires. Here + nothing has to be asked — the tool call IS the query, and the reflex has to + become a tool call before it can do anything. + + Deliberately TOOL-AGNOSTIC: takes a name and a string. The hook decides + which tools it watches, so widening the matcher is a `hooks.json` edit with + no change here. + + CONDITIONAL ONLY, exactly as the write-path arm — an always-on rule is + already resident and repeating it is noise. That filter is also the + transition this arm exists to enable: re-tier a rule to `conditional` and + it starts arriving here instead of in every session's preamble. + + Fails open and returns an empty context on any error: a recall aid may + never break the operator's action. + """ + out: dict = {"context": "", "rule_ids": []} + command = (command or "").strip() + if not command: + return out + + try: + cfg = await get_writepath_config(user_id) + if not cfg.get("enabled"): + return out + + # The command text is the query. A long heredoc or a pasted script + # would otherwise push the meaningful head of the command out of the + # embedding window, so it is bounded — the verb and its target sit at + # the front, which is the part a rule is about. + query = command[:_TOOL_QUERY_CHARS] + + t0 = time.perf_counter() + hits = await semantic_search_rules( + user_id, query, limit=RULEHINT_LIMIT, + threshold=cfg["rule_threshold"], tier="conditional", + ) + duration_ms = (time.perf_counter() - t0) * 1000.0 + + already = set(exclude_rule_ids or []) + fresh = [(score, rule) for score, rule in hits if rule.id not in already] + if not fresh: + return out + + lines: list[str] = [] + rule_ids: list[int] = [] + for _score, rule in fresh: + trigger = (rule.when_to_apply or "").strip() + lines.append( + f"Standing rule that may apply to this {tool_name} call — " + f"“{rule.title}”" + + (f" ({trigger})" if trigger else "") + + f". Read it with get_rule({rule.id}) before deciding it " + "does not apply; it is not in this session's loaded set." + ) + rule_ids.append(rule.id) + + record_retrieval( + user_id=user_id, source="pre_tool_rule", query=query, + threshold=cfg["rule_threshold"], limit=RULEHINT_LIMIT, + project_id=project_id, + is_task=None, results=fresh, duration_ms=duration_ms, + ) + # RANKED, not ambient: this arm chose what it showed, so a pull can + # settle whether the choice was any good. `rule_usage.RANKED_SOURCES` + # carries the same name. + record_rule_surfaced( + user_id=user_id, rule_ids=rule_ids, source="pre_tool_rule", + ) + out["context"] = "\n".join(lines) + out["rule_ids"] = rule_ids + except Exception: + logger.debug("pre-tool rule arm failed", exc_info=True) + return out + + def _derive_line(path: str, derive: list[dict]) -> str: """The ledger's word on the names being written (#2900): a duplicate family to derive, or a canon to reuse — said at the write.""" diff --git a/src/scribe/services/rule_usage.py b/src/scribe/services/rule_usage.py index f874d94..b30402d 100644 --- a/src/scribe/services/rule_usage.py +++ b/src/scribe/services/rule_usage.py @@ -54,8 +54,11 @@ WHY THIS NAMES THE RANKED SOURCES AND THE TWIN NAMES THE AMBIENT ONES. A deliberate divergence, on the failure mode rather than on symmetry. Both shapes fail silently when someone adds a surface and forgets the list, so the question is which list changes more often — and here it is emphatically the ambient one: -there is exactly ONE ranked rule source, and this change alone adds seven bulk -ones. Naming the rare, stable half means a newly-added bulk surface defaults to +there are TWO ranked rule sources (the write-path arm and the pre-tool arm) +against the seven bulk ones the preload alone contributes. Ranked sources are +added when somebody builds a ranker, which is rare and deliberate; bulk ones +appear whenever a surface hands rules over, which is most of them. Naming the +rare, slow-moving half means a newly-added bulk surface defaults to `ambient`, which merely under-counts it, instead of defaulting to `ranked`, which would quietly pad the pull-through denominator with surfacings nobody chose and make the arm look imprecise. Same argument #3191 and #3430 make @@ -82,7 +85,7 @@ logger = logging.getLogger(__name__) # surfacing is a claim ("this rule may apply to what you are doing") that a pull # can confirm or refute, while an ambient one is a delivery nobody decided on. # Add a source here only when a ranker picked it. -RANKED_SOURCES = ("write_path_rule",) +RANKED_SOURCES = ("write_path_rule", "pre_tool_rule") def is_ambient(source: str) -> bool: diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index 5ccd960..a012b8c 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -401,3 +401,179 @@ def test_the_marker_paths_stay_silent(): assert "record_rule_surfaced" not in staleness, ( "the staleness arm emits a surfacing — it compares a marker, it shows nothing" ) + + +# ── The PRE-TOOL arm: rules keyed on the action (#3476) ──────────────── +# +# The write-path arm can only be reached by a code write, so every rule about +# which tool to reach for was unretrievable at the moment it mattered — which +# is why they all had to be resident. These cover the surface that changes it. + + +def _tool_patches(pc, hits, recorder, cfg=None): + return ( + patch.object(pc, "get_writepath_config", + AsyncMock(return_value=cfg or { + "enabled": True, "threshold": 0.6, + "top_k": 3, "rule_threshold": 0.6, + })), + patch.object(pc, "semantic_search_rules", AsyncMock(return_value=hits)), + patch.object(pc, "record_retrieval", MagicMock()), + patch.object(pc, "record_rule_surfaced", recorder), + ) + + +async def _run_tool_arm(hits, recorder, command="curl -s https://git.example/api/v1/runs", + tool="Bash", **kwargs): + from scribe.services import plugin_context as pc + with ExitStack() as stack: + for ctx in _tool_patches(pc, hits, recorder): + stack.enter_context(ctx) + return await pc.build_tool_rule_hint(1, tool, command, **kwargs) + + +@pytest.mark.asyncio +async def test_the_tool_arm_names_a_rule_for_the_command_about_to_run(): + """The 2026-09-03 incident in one test: reaching for curl against the forge + API is a Bash call, and nothing watched Bash.""" + rec = MagicMock() + hits = [(0.71, fake_rule(id=161, + title="Reach the forge through its MCP tools, never curl", + when_to_apply="whenever you need CI status"))] + out = await _run_tool_arm(hits, rec) + + assert out["rule_ids"] == [161] + assert "Reach the forge through its MCP tools" in out["context"] + assert "get_rule(161)" in out["context"], "the hint must hand over the way to read it" + assert "Bash" in out["context"], "the hint names the tool it is about" + assert rec.call_args.kwargs["source"] == "pre_tool_rule" + + +@pytest.mark.asyncio +async def test_the_tool_arm_is_a_ranked_source(): + """It CHOSE what it showed, so a pull can settle whether the choice was any + good — unlike a preload, which chose nothing. If this drifts into the + ambient class the arm becomes unjudgeable, which is the state #3311 + described and M333 existed to end.""" + from scribe.services.rule_usage import is_ambient + + assert not is_ambient("pre_tool_rule") + + +@pytest.mark.asyncio +async def test_a_rule_the_session_already_holds_is_not_re_offered(): + rec = MagicMock() + hits = [(0.71, fake_rule(id=161, title="Reach the forge through its MCP tools")), + (0.70, fake_rule(id=12, title="Don't run a local stack unless asked"))] + out = await _run_tool_arm(hits, rec, exclude_rule_ids=[161]) + + assert out["rule_ids"] == [12] + assert "161" not in out["context"] + + +@pytest.mark.asyncio +async def test_an_empty_command_asks_the_ranker_nothing(): + """Every Bash call reaches this. A blank payload must cost no embedding + query at all, not merely return nothing after paying for one.""" + from scribe.services import plugin_context as pc + + search = AsyncMock(return_value=[]) + rec = MagicMock() + with ExitStack() as stack: + stack.enter_context(patch.object(pc, "semantic_search_rules", search)) + stack.enter_context(patch.object(pc, "record_rule_surfaced", rec)) + out = await pc.build_tool_rule_hint(1, "Bash", " ") + + assert out == {"context": "", "rule_ids": []} + search.assert_not_called() + rec.assert_not_called() + + +@pytest.mark.asyncio +async def test_the_tool_arm_fails_open(): + """A recall aid may never break the operator's action. A ranker that raises + must cost the hint, not the command.""" + from scribe.services import plugin_context as pc + + with ExitStack() as stack: + stack.enter_context(patch.object(pc, "get_writepath_config", + AsyncMock(side_effect=RuntimeError("boom")))) + out = await pc.build_tool_rule_hint(1, "Bash", "docker compose up -d") + + assert out == {"context": "", "rule_ids": []} + + +@pytest.mark.asyncio +async def test_a_long_command_is_bounded_before_it_reaches_the_ranker(): + """A heredoc or a pasted script would push the verb and its target — the + part a rule is about — out of the embedding window.""" + from scribe.services import plugin_context as pc + + search = AsyncMock(return_value=[]) + with ExitStack() as stack: + for ctx in _tool_patches(pc, [], MagicMock()): + stack.enter_context(ctx) + stack.enter_context(patch.object(pc, "semantic_search_rules", search)) + await pc.build_tool_rule_hint(1, "Bash", "git tag v1 && " + "x" * 5000) + + sent = search.call_args.args[1] + assert len(sent) <= pc._TOOL_QUERY_CHARS + assert sent.startswith("git tag v1"), "the head of the command is the signal" + + +def test_the_two_pre_tool_arms_share_one_session_rule_ledger(): + """The integration point most worth guarding. + + Two ledgers would mean a rule named by the write arm gets re-offered by the + tool arm — and the hint that fires most often is exactly the one that must + not repeat itself. Asserted on the FILENAME both scripts build, because + that is the shared thing; a copy of the path in each is how they drift. + """ + prior = Path("plugin/hooks/scribe_prior_art.sh").read_text() + tool = Path("plugin/hooks/scribe_tool_rules.sh").read_text() + + for src, name in ((prior, "scribe_prior_art.sh"), (tool, "scribe_tool_rules.sh")): + assert '"${TMPDIR:-/tmp}/scribe-priorart"' in src, f"{name}: state dir moved" + assert '.rules.ids' in src, f"{name}: rules ledger filename moved" + assert "exclude_rule_ids" in src, f"{name}: does not send the exclusion" + + +def test_the_tool_arm_is_registered_on_bash(): + """A hook that exists and is not registered runs never — and reads exactly + like a surface nobody needed.""" + import json + + manifest = json.loads(Path("plugin/hooks/hooks.json").read_text()) + pre = manifest["hooks"]["PreToolUse"] + entries = { + m.get("matcher"): [h["command"] for h in m["hooks"]] for m in pre + } + assert "Bash" in entries, "nothing watches Bash — the reflex surface is unguarded" + assert any("scribe_tool_rules.sh" in c for c in entries["Bash"]) + # The write arm keeps its own matcher; this is an addition, not a move. + assert any("scribe_prior_art.sh" in c for c in entries["Write|Edit"]) + + +def test_the_hook_and_the_route_agree_on_every_parameter_name(): + """Rule 33, on a brand-new integration between layers. + + The hook is shell and the route is Python; nothing but this test connects + them. A renamed query arg fails SILENTLY — the route reads an absent value, + the arm quietly searches nothing, and the surface looks like one that never + finds anything rather than one that is broken. + """ + import re + + hook = Path("plugin/hooks/scribe_tool_rules.sh").read_text() + route = Path("src/scribe/routes/plugin.py").read_text() + handler = route.split("async def pre_tool_rules")[1].split("\n@plugin_bp")[0] + + sent = set(re.findall(r"[?&]([a-z_]+)=", hook)) + assert sent == {"tool", "command", "repo", "exclude_rule_ids"}, sent + + # `repo` is read by the shared _project_scope() helper, not inline. + assert "_project_scope()" in handler + for arg in ("tool", "command", "exclude_rule_ids"): + assert f'request.args.get("{arg}")' in handler, ( + f"the hook sends {arg!r} and the route never reads it" + ) -- 2.54.0