From f6b824b214d937ead2a9d235ba3ebb50454333de Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 5 Oct 2026 08:01:23 -0400 Subject: [PATCH] refactor(retrieval): the three rule arms run through one pipeline (milestone 456 step 2, #4904) prompt_rule, pre_tool_rule and the rule half of the write path each wrote the same steps out by hand: search, band, split fresh from repeats, log the call before any early return, reserve a slot, render, record surfacings. The copies drifted, and #3497, #3750 and #3752 were fixed one copy at a time. - New services/retrieval_pipeline.py. run_rule_arm runs the stages in one order. RuleArm is the spec (source, band, compact_tail, checkpoint, preference_slot). RuleMoment is the query and the session ledger. - The I/O (ranker and recorders) is passed in as RuleIO. plugin_context resolves it from its own names at call time, so existing patch points still apply. - _rule_band, _rule_hint_line, checkpoint_for, checkpoint_reason, the band constant and the preference slot moved into the pipeline unchanged. plugin_context re-exports them. - A recorder that raises now costs only its row, never a rendered line. - The flags reproduce today exactly. Whether prompt_rule should band, and whether the act arms should reserve a preference, are step 7. - Registry: the pipeline call sites are declared in FAN_OUT_SITES, with values read from the specs. - The #3497 structural guard now checks the one implementation, and that plugin_context writes no rule-source row of its own. plugin_context.py: 3,558 -> 2,980 lines. Co-Authored-By: Claude Opus 5.5 --- src/scribe/services/plugin_context.py | 726 +++------------------- src/scribe/services/retrieval_pipeline.py | 637 +++++++++++++++++++ src/scribe/services/retrieval_registry.py | 12 + tests/test_rule_usage_wiring.py | 124 ++-- 4 files changed, 783 insertions(+), 716 deletions(-) create mode 100644 src/scribe/services/retrieval_pipeline.py diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 660aec85..db107a4e 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -45,6 +45,10 @@ from scribe.services.retrieval_surfaces import ( budget_for, floor_for, ) +from scribe.services import retrieval_pipeline as rp +from scribe.services.retrieval_pipeline import ( # noqa: F401 - re-exported + _RULEHINT_BAND, _rule_band, _rule_hint_line, checkpoint_for, checkpoint_reason, +) from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.settings import get_setting from scribe.services.systems import system_names_for @@ -351,33 +355,6 @@ TOOLRULE_DEFAULT_THRESHOLD = SURFACES["pre_tool_rule"].floor_default # the value it is stuck with. The reasoning below is why 5 is where it starts. RULEHINT_LIMIT = SURFACES["write_path_rule"].budget_default -# MEASURED, NOT REASONED — and the reasoning it replaced was wrong (#3851). -# -# The prediction was that rules would rank SHARPLY, because `rule_document()` -# shapes them the way snippets are shaped — trigger in the embedded title and -# again above the body — and note 2485 measured snippets separating their top -# hit by 0.153 while every other kind managed 0.010–0.023. -# -# They do not. Three probes against real act queries, scored by the same -# embedding the arms use: -# -# `git push origin dev` top 0.757, gap to second 0.022 -# `docker compose up -d` top 0.685, gap to second 0.016 -# a bare-owner-filter query top 0.656, gap to second 0.020 -# -# That is dev-log territory, not snippet territory: rules arrive as a -# tightly-packed block. Shaping alone did not buy separation, which is worth -# recording because the opposite was the natural inference from 2485. -# -# So the band is narrow BECAUSE the corpus is flat. At 0.10 — the notes -# menu's value — every one of the top eight on the push probe falls inside, -# including a CI-registry rule and another project's branch policy. At 0.05 -# it admits roughly three ranks, which is the span where the scores are still -# saying something. 2485's Finding 3 is the standing caveat: no band value -# fixes a tie, and if rules ever rank as flat as dev-logs did this control -# stops working and the answer is a reranker (#1038), not a smaller number. -_RULEHINT_BAND = 0.05 - # ── The pre-act checkpoint (#4214, milestone 419) ─────────────────────── # # WHAT A CHECKPOINT IS, AND WHY IT IS NOT A LOUDER HINT. Every arm above @@ -1392,109 +1369,6 @@ async def build_autoinject_hint( } -async def _reserve_slot_for_preference( - user_id: int, - query: str, - hits: list, - *, - threshold: float, - project_id: int, - already: set[int], -) -> tuple[list, int | None]: - """Guarantee a preference one slot, if one clears the bar (#3894). - - THE ASYMMETRY THIS EXISTS FOR. A rule and a preference are not equally - served by a shared score contest, because their losses are not equal: - - - a RULE crowded out here can still fire at the act arm. A `git push` - reaches `pre_tool_rule`, a file write reaches `write_path_rule`. The - prompt hit is a preview of a second chance. - - a PREFERENCE about how to answer has no second chance. There is no - later act — the response IS the act — so crowded out here it is never - delivered at all. - - A straight ranking therefore favours the record whose loss is recoverable - over the one whose loss is total, and it does so INVISIBLY: the rule that - won is a legitimate hit, the telemetry looks healthy, and the only symptom - is a preference that quietly never arrives. `reuse_slot` exists for the - same shape one corpus over (#2463), where snippets kept losing to project - records that merely resembled the query. - - THE SLOT BUYS POSITION, NOT A LOWER BAR — same as `reuse_slot`, which also - reserves at `cfg["threshold"]`. A weak preference cannot buy the slot, so - silence stays the default and the reserved line is never worse than the - ones it sits beside. If `preference_slot` later shows a stream of - near-misses, `best_available_id` (#3807) names which preference was - refused and a separate bar becomes an argument with evidence behind it - rather than a knob added on a guess. - - LEDGER REPEATS STILL COUNT AS REPRESENTED. A preference already on the - session's ledger occupies the slot rather than being skipped for a fresh - one: it is still rendered (#3750), just with the tail that says so, and a - preference is the kind of record where being reminded is the point. - - Returns the possibly-extended hit list, and the id the slot spent — the - caller needs that to keep each source's surfaced set matching its own log - row (#3668), since the slot logs under its own name. - """ - if any(rule.kind == "preference" for _s, rule in hits): - return hits, None - - _t0 = time.perf_counter() - _rep: dict = {} - # KIND-FILTERED, so the query can only answer with what the slot is for. - # Verifying the kind afterwards would be weaker: an unfiltered search that - # happened to return a rule would spend the slot on it, and the line would - # be indistinguishable from one that earned its place. - found = await semantic_search_rules( - user_id, query, limit=1, threshold=threshold, - kind="preference", report=_rep, project_id=project_id or None, - ) - fresh = [(s, r) for s, r in found if r.id not in already] - # ITS OWN SOURCE, and both sides of the trade logged. #2463's own finding - # is the warning rather than the precedent here: the hit that slot pushed - # OUT was in retrieval_logs while the query that pushed it out was not, so - # the slot could never be judged against what it displaced. `results` is - # fresh-only, matching what gets recorded as surfaced below (#3752/#3668). - record_retrieval( - user_id=user_id, source="preference_slot", query=query, - threshold=threshold, limit=1, project_id=project_id, - is_task=None, results=fresh, - best_available=_rep.get("best_available_score"), - best_available_id=_rep.get("best_available_id"), - searched=bool(_rep.get("searched", True)), - suppressed=len(found) - len(fresh), - duration_ms=(time.perf_counter() - _t0) * 1000.0, - ) - seen = {rule.id for _s, rule in hits} - slot = [(s, r) for s, r in found - if r.kind == "preference" and r.id not in seen][:1] - if not slot: - return hits, None - - slot_id = int(slot[0][1].id) - if slot_id not in already: - record_rule_surfaced( - user_id=user_id, rule_ids=[slot_id], source="preference_slot", - ) - # IT EXTENDS, IT NEVER DISPLACES — and here it parts company with - # `reuse_slot`, which evicts its menu's weakest hit. The reason is the - # ledger rather than taste. A displaced hit was RETURNED by the general - # search and is sitting in that call's `retrieval_logs` row, but would not - # have been shown — so `prompt_rule`'s surfaced set would stop matching - # its own log row, and #3668's identity would break for a reason nothing - # in the data explains. That identity is the cheapest true statement - # available about this pair of tables, and milestone #379 is what it costs - # to lose it: five steps planned against a gap that was two counters - # disagreeing, not a write path dropping rows. - # - # The price is one extra line, only when the general search already filled - # the limit AND a preference cleared the bar without placing. Cheap, and - # it buys a surface whose two tables can always be checked against each - # other. - return hits + slot, slot_id - - # ── A rule reached through its lessons (milestone 440, #4633) ────────────── # # A rule's own document is written in the rule's words, which are general by @@ -1688,95 +1562,48 @@ async def _prompt_rule_hint( out["_via_query"] = q try: - threshold = await floor_for(user_id, "prompt_rule") - limit = await budget_for(user_id, "prompt_rule") - - t0 = time.perf_counter() - _rep: dict = {} # SCOPED TO THIS SESSION'S PROJECT (milestone 414): global rules plus # the bound project's own. An unbound session (project_id 0) gets - # global rules only. This arm used to search every rule the user owned, - # so each project's rules were injected into every other project's - # sessions — this surface speaks unasked, and a whole-rulebook answer - # is only right for someone who asked the whole rulebook. - hits = await semantic_search_rules( - user_id, q, limit=limit, threshold=threshold, - report=_rep, project_id=project_id or None, + # global rules only — this surface speaks unasked, and a whole-rulebook + # answer is only right for someone who asked the whole rulebook. + result = await rp.run_rule_arm( + rp.PROMPT_RULE, + rp.RuleMoment( + user_id=user_id, query=q, project_id=project_id, + where="to this request", + exclude=frozenset(exclude_rule_ids or []), + held=frozenset(held_rule_ids or []), + ), + floor=await floor_for(user_id, "prompt_rule"), + budget=await budget_for(user_id, "prompt_rule"), + io=_rule_io(), ) - duration_ms = (time.perf_counter() - t0) * 1000.0 - - already = set(exclude_rule_ids or []) - held = set(held_rule_ids or []) - fresh = [(score, rule) for score, rule in hits if rule.id not in already] - - # BEFORE the early return, for the reason both sibling arms spell out - # at length: a call that found nothing is the only evidence a bar is - # too high, and an arm that logs only the calls it liked reports a - # flawless clear-rate however badly it is tuned. This bar is inherited - # and unverified for this corpus, so the zero rows are the point. - record_retrieval( - user_id=user_id, source="prompt_rule", query=q, - threshold=threshold, limit=limit, - project_id=project_id, - is_task=None, results=fresh, duration_ms=duration_ms, - best_available=_rep.get("best_available_score"), - best_available_id=_rep.get("best_available_id"), - searched=bool(_rep.get("searched", True)), - suppressed=len(hits) - len(fresh), - ) - # THE RESERVED SLOT RUNS BEFORE THE BAIL-OUT, and that ordering is - # load-bearing rather than tidy. An empty general result is not proof - # that no preference qualifies: the general search overfetches by - # distance and then collapses, so a preference ranked below that - # window is invisible to it while a kind-filtered query finds it at - # once. Bailing first would make the slot dead in exactly the corpus - # it exists for — one where rules outnumber preferences. - hits, slot_id = await _reserve_slot_for_preference( - user_id, q, hits, threshold=threshold, - project_id=project_id, already=already, - ) - - # `hits`, not `fresh` (#3750): a call whose only hit is a repeat still - # has something to say, it just says it differently. - if not hits: - return out - - lines = [ - _rule_hint_line( - rule, where="to this request", - seen=rule.id in already, held=rule.id in held, - ) - for _score, rule in hits - ] - # FRESH-ONLY (#3752). A reference is a rendering decision, not a - # retrieval outcome, and counting one here would inflate the - # denominator pull_through is read from. - # - # `fresh` is the PRE-SLOT list on purpose: it is exactly what this - # call's own `retrieval_logs` row recorded, so the two stay equal - # (#3668). The slot's hit is surfaced under `preference_slot` by the - # helper, against that source's own row. - rule_ids = [rule.id for _score, rule in fresh] - # Every rule LINE, repeats and the reserved slot included — what the - # reader actually had in front of them, for the soft-link recorder - # (#4637). Distinct from `rule_ids`, which is telemetry's fresh-only cut. - out["shown_rule_ids"] = [rule.id for _score, rule in hits] - - # RANKED, not ambient: this arm chose what it showed. The name is also - # in `rule_usage.RANKED_SOURCES`, and it has to be — a ranked source - # missing from that tuple is counted as a bulk delivery nobody decided - # on, which silently moves it out of the pull-through denominator. - if rule_ids: - record_rule_surfaced( - user_id=user_id, rule_ids=rule_ids, source="prompt_rule", - ) - out["context"] = "\n".join(lines) - out["rule_ids"] = rule_ids + if result.lines: + out["context"] = "\n".join(result.lines) + out["rule_ids"] = result.rule_ids + # Every rule LINE, repeats and the reserved slot included — for + # the soft-link recorder (#4637). Distinct from `rule_ids`, which + # is telemetry's fresh-only cut. + out["shown_rule_ids"] = result.shown_rule_ids except Exception: logger.debug("prompt rule arm failed", exc_info=True) return out +def _rule_io() -> rp.RuleIO: + """The ranker and recorders the rule arms report to, read at call time. + + Resolved from this module's names on every call rather than bound once, so + the pipeline reports to whatever this module's `semantic_search_rules`, + `record_retrieval` and `record_rule_surfaced` are at the moment it runs. + """ + return rp.RuleIO( + search=semantic_search_rules, + record_retrieval=record_retrieval, + record_rule_surfaced=record_rule_surfaced, + ) + + # --- Write-path trigger (#2082): prior art at the moment code is written ------ # Auto-inject above fires on the operator's prompt. The moment reuse is actually # lost is later — when the AGENT decides mid-task to write a helper — and nothing @@ -2025,264 +1852,6 @@ async def _checkpoint_floor(user_id: int) -> float: return _CHECKPOINT_DEFAULT return min(1.0, max(0.0, value)) -def _rule_band(hits: list) -> list: - """The top hit, plus every hit within `_RULEHINT_BAND` of it (#3851). - - The instrument that lets an act surface a SET without inventing one: a - fixed k fills its slots whether or not anything deserves them, while this - keeps only what the scores say is close, so one clearly-relevant rule - still shows one and four competing rules show four. - - Sync and pure, and deliberately its own function rather than a comparison - written twice — the two act arms are the pair #3497 records drifting apart - by being modelled on each other instead of sharing. - - Takes `(score, rule)` pairs already ordered best-first, as both - `semantic_search_*` helpers return them. - """ - if not hits: - return [] - top = hits[0][0] - return [(s, r) for s, r in hits if s >= top - _RULEHINT_BAND] - - -def checkpoint_for( - kept: list, *, held: set[int], floor: float, where: str, -) -> dict: - """The one rule, if any, that should STOP this act rather than annotate it. - - Sync and pure so it can be read against a fixed list of hits without a - database — the two arms share it for the reason `_rule_band` is shared: - #3497 is the record of these two drifting apart by being modelled on each - other instead of sharing one function. - - FOUR CONDITIONS, AND EACH IS A DIFFERENT KIND OF WRONG IT PREVENTS. - - 1. THE SCORE CLEARS `floor`. Not the arm's own floor — a much higher bar, - measured at `_CHECKPOINT_DEFAULT`. The hint arms keep nudging at their - floor; only a hit the corpus is confident about is allowed to stop - anything. - - 2. IT IS A RULE, NEVER A PREFERENCE. A preference says how something has - been done before and following it is what keeps work consistent; a rule - says what happens if you do not. Stopping an act over a preference - would assert a force the record explicitly does not claim, and - `_rule_hint_line` already keeps that distinction in the one word that - names it. - - 3. THE SESSION HAS NOT OPENED IT. `held` is observable — a PostToolUse - hook watches for the `get_rule` call (#4100) — so this is a recorded - event and not a model's self-report about its own context. A session - that read the rule has already had the thing the checkpoint exists to - produce, and stopping it again would be punishing the behaviour being - asked for. - - 4. IT IS THE TOP HIT. `kept` is a band, and a band's tail is there to let - an act surface a SET; the ranker's confidence claim attaches to its - first element only. A checkpoint raised on the fourth line of a band is - a stop justified by a score nobody claimed. - - Returns a dict rather than a rule or a tuple. Widening a tuple is an - interface change to every unpack site that the compiler does not report - (#4207, learned the expensive way in this same milestone's first week), and - this value crosses a JSON boundary into a shell script where a missing - field is a silently empty variable. - """ - if not kept or floor <= 0: - return {} - score, rule = kept[0] - if score < floor: - return {} - if getattr(rule, "kind", "") == "preference": - return {} - if rule.id in held: - return {} - found = { - "rule_id": rule.id, - "title": rule.title, - "trigger": (rule.when_to_apply or "").strip(), - "score": round(float(score), 4), - "where": where, - } - # RENDERED HERE, at the one call site, rather than by each arm. Two arms - # that each remember to render it are two arms that can stop agreeing on - # what a stop says — which is #3497's history for this exact pair. The - # text stays a separate function so it can be read and tested without a - # rule object, but nothing outside this line decides whether to call it. - found["reason"] = checkpoint_reason(found) - return found - - -def checkpoint_reason(checkpoint: dict) -> str: - """The text the agent reads INSTEAD of running the act. - - Written as a practice rather than a prohibition (rule 165): it says what - to do and why it is worth doing, not what is forbidden. The act is not - wrong — nothing here knows whether it is — and saying so plainly is what - keeps the stop from reading as an accusation the system is in no position - to make. - - It names the remedy as ONE call, because a stop whose remedy is vague - costs more than the miss it prevents. And it says the act may simply be - re-submitted afterwards, so a reader who finds the rule irrelevant is out - in two calls rather than negotiating with a hook. - """ - if not checkpoint: - return "" - trigger = checkpoint.get("trigger") or "" - return ( - f"Held for one read. “{checkpoint['title']}” is a standing rule " - f"this session has not opened, and it scores {checkpoint['score']} " - f"against what you are about to do" - + (f" ({trigger})" if trigger else "") - + f". Read it with get_rule({checkpoint['rule_id']}), then go ahead — " - f"re-submit this call unchanged if the rule does not apply, which is a " - f"judgement only you can make. Nothing here has decided the act is " - f"wrong; the rule is being put in front of it rather than beside it, " - f"because a rule delivered alongside a result arrives after the " - f"decision it was meant to inform." - ) - - -def _rule_hint_line( - rule, *, where: str, seen: bool, held: bool = False, compact: bool = False, -) -> str: - """One rule hint line — both arms, both tails, both kinds (#3750, #3849). - - THREE INDEPENDENT AXES SINCE #3851. `compact` joins `kind` and `seen`, and - like them it reads none of the others: it says how much ROOM this line - gets, which is a fact about its rank among today's hits rather than about - the rule. A compact line is still a full claim that the rule may apply — - it simply cites the rule instead of quoting its trigger. Crucially it - still carries the `seen` tail, so the three axes stay genuinely - independent: shortening a line must not decide what it says about whether - the session is holding the rule. - - WHY THE LATER LINES ARE QUIETER. Measured at #3851: a full line runs ~143 - tokens once the trigger is rendered, and #3855 tripled trigger lengths - across the corpus, so five full lines cost ~568 tokens before every Bash - call. Top-full-plus-references costs ~299 — about 2x the old single line, - for four more rules. The budget argument that once justified a single slot - was real; what it actually forbids is four voices at full volume, not four - voices. - - The top hit keeps the full rendering because it is the one the ranker is - most confident about, and a reader who acts on exactly one line should - have acted on that one. - - TWO INDEPENDENT AXES. `kind` decides the head, `seen` decides the tail, - and neither reads the other. A preference and a rule differ in force; a - repeat and a first surfacing differ in whether the session already holds - the line. Those are unrelated facts, and keeping them unrelated in the - code is what stopped the second kind from reopening the repeat question. - - ONE FUNCTION BECAUSE THE TAILS MUST NOT DRIFT. The two arms phrase their - heads differently ("may apply here" vs "may apply to this Bash call") and - that difference is deliberate. Everything after it must not differ, and - #3497's history is that the pre-tool arm inherited a defect from its - sibling by being modelled on it rather than sharing with it. Two copies of - a two-branch string is how one branch gets fixed and the other does not. - - WHY A REPEAT GETS A LINE AT ALL. Both arms used to drop a hit whose id was - already on the session's exclusion ledger and emit nothing. That is correct - only while the session still HOLDS what it was told, and a compaction - breaks exactly that: the earlier injection is summarized away while the id - stays on the ledger, so the rule is absent from context AND unreachable for - the rest of the session (#3749 closes the compaction half; this closes the - ordinary half, where a session simply stops holding a line it read an hour - ago). - - Only ONE CLAUSE of the original line is false on a repeat — the claim that - the rule is not in the session's loaded set. So only that clause changes. - Title, trigger and pull pointer are identical either way, the statement is - never injected either way, and a repeat therefore costs the same ~40 tokens - as a first surfacing and no more. - - DELIBERATELY NOT ASKING THE SESSION WHETHER IT HOLDS THE RULE. A model - asked "do you still hold rule 156?" will say yes, and the claim is - unverifiable self-report about its own context. The answer is also not - needed: the line is cheap enough to always emit and carries its own remedy - in both branches. Removing the question removes the fragility rather than - managing it. - """ - trigger = (rule.when_to_apply or "").strip() - preference = rule.kind == "preference" - # KIND CHANGES THE HEAD; `seen` CHANGES THE TAIL. The two axes are - # independent and stay that way, which is what lets the repeat logic above - # survive a second kind without being reasoned about again: whether a - # record is already on the ledger has nothing to do with how much force it - # carries, so the seen-branch is shared verbatim. - # - # The noun is the whole of the visual difference, and that is deliberate. - # A reader skimming an injected block gets one word to place the register \u2014 - # so it is the SECOND word that moves, and it is the word naming force. - # Everything structural after it is identical, so the three kinds read as - # one set rather than three formats (milestone 385's step 5 writes the - # lesson voice against these two; they are one paragraph, not three). - noun = "Preference" if preference else "Standing rule" - # The only other place force is asserted. A rule's line tells the reader - # not to dismiss it unread, because dismissing a rule unread is how the - # thing it prevents happens. A preference makes no such claim: it says - # where to find how this has been done, and following it is what keeps - # things consistent rather than what keeps them correct. - reason = ( - "for how this has been done before" if preference - else "before deciding it does not apply" - ) - # THREE STATES, BECAUSE TWO OF THEM WERE BEING TOLD THE SAME LIE (#4100). - # - # `seen` means an arm NAMED this rule earlier. It does not mean the session - # read it — the line is a teaser, and a teaser skimmed past leaves nothing - # behind, least of all after a compaction summarises the turn it arrived - # in. "You saw it earlier this session" asserted something about the - # reader's context that the server had no way to know. - # - # `held` is the observable half: a PostToolUse hook watches for the - # `get_rule` call itself, so this is a recorded EVENT rather than a claim. - # That distinction is what keeps the non-goal above intact — the objection - # was to asking a model about its own context, not to noticing what it did. - # - # The middle state is the honest one and the one that was missing: named, - # not opened. It gets the full invitation, because a session that skipped - # the teaser is in almost the same position as one that never saw it. - if held: - tail = ( - f"You opened it earlier this session; pull it with " - f"get_rule({rule.id}) again if you no longer hold it." - ) - elif seen: - tail = ( - f"Mentioned earlier this session but not opened — read it with " - f"get_rule({rule.id}) {reason}." - ) - else: - tail = ( - f"Read it with get_rule({rule.id}) {reason}; it is not in this " - "session's loaded set." - ) - if compact: - # THE TRIGGER GOES; THE TAIL STAYS. Only one of the two is expensive \u2014 - # a trigger runs 300-400 characters after #3855, the tail about 100 \u2014 - # so dropping the trigger is nearly the whole saving and dropping the - # tail would be mostly sacrifice. - # - # It would also destroy the one thing #3750 exists to say. The tail is - # what tells a reader whether they were already told this and may no - # longer be holding it, and that is the entire difference they can act - # on; a reference with no tail reads as a first surfacing whether it is - # one or not. This branch shipped without it for one commit and - # test_a_rule_the_session_already_holds_is_referenced_not_re_offered - # caught it, which is the guard working exactly as #3750 intended. - return ( - f"Also \u2014 {noun.lower()} \u201c{rule.title}\u201d. {tail}" - ) - return ( - f"{noun} that may apply {where} \u2014 \u201c{rule.title}\u201d" - + (f" ({trigger})" if trigger else "") - + f". {tail}" - ) - - async def build_write_path_hint( user_id: int, path: str, @@ -2862,117 +2431,35 @@ async def build_write_path_hint( # session was told about an hour ago is not a rule in front of the reader # now. # + # The arm itself is `retrieval_pipeline.run_rule_arm` (milestone 456): the + # band, the fresh/repeat split, the unconditional call row (#3497) and the + # fresh-only surfacing rows (#3752) are written there once for all three + # rule arms. What is decided HERE is only what makes this moment this + # moment — the query is the code being written (or the path, for an edit + # with no body), and a line says the rule may apply "here". + # # Fails open like every other arm: a rule hint must never break a write. rule_ids: list[int] = [] shown_rule_ids: list[int] = [] checkpoint: dict = {} try: - already = set(exclude_rule_ids or []) - held = set(held_rule_ids or []) - # Timed like the notes arm above. Without this the rule row was the one - # source in the whole readout reporting a null p90_duration_ms (#3311) - # — a gap that reads as "this surface is somehow not measurable" rather - # than "nobody passed the number". - rule_t0 = time.perf_counter() - _rep_wpr: dict = {} - hits = await semantic_search_rules( - user_id, code or path, limit=cfg["rule_top_k"], - threshold=cfg["rule_threshold"], - report=_rep_wpr, project_id=project_id or None, + result = await rp.run_rule_arm( + rp.WRITE_PATH_RULE, + rp.RuleMoment( + user_id=user_id, query=code or path, project_id=project_id, + where="here", checkpoint_where="here", + exclude=frozenset(exclude_rule_ids or []), + held=frozenset(held_rule_ids or []), + ), + floor=cfg["rule_threshold"], budget=cfg["rule_top_k"], + io=_rule_io(), checkpoint_floor=cfg["checkpoint_threshold"], ) - rule_ms = (time.perf_counter() - rule_t0) * 1000.0 - # BAND FIRST, dedup second, and the order is the whole point (#3851). - # The band is a statement about the SCORES — what the ranker thinks is - # close to the best match — so letting the ledger reorder it would let - # "you were told this already" change what counts as relevant. Those - # are the independent axes the renderer keeps apart. - kept = _rule_band(hits) - fresh = [(score, rule) for score, rule in kept if rule.id not in already] - # EVERY kept hit gets a line; `already` only changes the tail (#3750), - # and rank only changes how much room it gets (#3851). - for idx, (_score, rule) in enumerate(kept): - lines.append( - _rule_hint_line( - rule, where="here", seen=rule.id in already, - held=rule.id in held, - compact=idx > 0, - ) - ) - # `rule_ids` stays FRESH-ONLY, and that is the whole telemetry story of - # this change (#3752). It is what the hook writes to the exclusion - # ledger and what `record_rule_surfaced` counts; a referenced rule is - # already on the ledger by definition, and counting it as a surfacing - # would inflate pull_through's denominator with a choice this arm never - # made. A reference is a RENDERING decision, not a retrieval outcome. - rule_ids.extend(rule.id for _score, rule in fresh) + lines.extend(result.lines) + rule_ids.extend(result.rule_ids) # Every rule line, repeats included — what the soft-link recorder # pairs with this response's lessons (#4637). - shown_rule_ids = [rule.id for _score, rule in kept] - # The stop, beside the lines rather than instead of them — see - # `checkpoint_for`. `kept` is passed, not `fresh`: whether a rule - # was named earlier this session says nothing about whether this - # act should wait for it to be READ, and those are the two axes - # #3750 exists to keep apart. - checkpoint = checkpoint_for( - kept, held=held, floor=cfg["checkpoint_threshold"], where="here", - ) - # TWO tables, and the split is not arbitrary. retrieval_logs is one - # row per CALL, keyed on the score distribution a threshold is tuned - # from. rule_usage_events is one row per RULE per event, which is the - # grain "was this hint ever acted on" needs and the grain a JSONB - # result_ids array cannot be indexed at. - # - # This comment used to say rule ids had nowhere to go — that - # note_usage_events remaps ids on restore, so a rule id there would - # return attached to whatever note took that number. That is still - # true of the NOTE table, and it is exactly why rule_usage_events is - # its own (milestone 333 step 1). The gap it described is closed. - # - # THE CALL LOG IS UNCONDITIONAL; THE SURFACING LOG IS NOT, and the - # asymmetry is the correction #3497 exists to make. Both used to sit - # inside an `if fresh:`, which is how this arm came to report - # `zero_result_calls: 0` and `cleared_threshold: 133/133` — not a - # perfectly tuned surface but one structurally unable to record its - # own misses. #3311 read that artifact as a measurement and a whole - # milestone was scoped on it. A call that found nothing is the ONLY - # evidence a threshold is set too high, and it is the row every note - # surface has always written (write_path: 421 zeroes of 613 calls; - # auto_inject: 114 of 326). A SURFACING is different in kind: nothing - # was shown, so no such event occurred, and its log stays guarded. - # - # `results=fresh`, not `hits`: the note arms pass their exclusions - # INTO semantic_search_notes, so what they log is already - # post-exclusion. semantic_search_rules takes no such parameter and - # this filter is where the equivalent happens — logging `hits` would - # quietly make this row mean something other than every other row in - # the same readout. - record_retrieval( - user_id=user_id, source="write_path_rule", query=code or path, - threshold=cfg["rule_threshold"], limit=cfg["rule_top_k"], - project_id=project_id, - is_task=None, results=fresh, duration_ms=rule_ms, - best_available=_rep_wpr.get("best_available_score"), - best_available_id=_rep_wpr.get("best_available_id"), - searched=bool(_rep_wpr.get("searched", True)), - # FOUND BUT NOT SHOWN, which since #3851 has TWO causes: the - # session had already been told (the ledger), or the score fell - # outside `_RULEHINT_BAND` of the top hit. Both are counted here - # because the question this answers is unchanged — a zero row must - # be able to say whether the bar was too high or whether the arm - # simply chose not to speak, and only the first is a reason to - # move the threshold. Splitting the two causes needs its own - # column and is worth doing only if the band turns out to be - # dropping rules anyone wanted. - suppressed=len(hits) - len(fresh), - ) - if fresh: - # `rule_ids` is `fresh`, i.e. AFTER exclude_rule_ids. A rule the - # session already holds was considered and not shown, and counting - # it would inflate the denominator with claims the agent never saw - # — which reads as a precision problem this arm does not have. - record_rule_surfaced( - user_id=user_id, rule_ids=rule_ids, source="write_path_rule", - ) + shown_rule_ids = result.shown_rule_ids + checkpoint = result.checkpoint except Exception: logger.debug("write-path rule arm failed", exc_info=True) @@ -3128,87 +2615,22 @@ async def _tool_rule_hint( # For the via-lesson step (#4633) — set only once the arm is enabled. out["_via_query"] = query - t0 = time.perf_counter() - _rep_ptr: dict = {} - hits = await semantic_search_rules( - user_id, query, limit=cfg["tool_rule_top_k"], - threshold=cfg["tool_rule_threshold"], - report=_rep_ptr, project_id=project_id or None, - ) - duration_ms = (time.perf_counter() - t0) * 1000.0 - - already = set(exclude_rule_ids or []) - held = set(held_rule_ids or []) - # Band first, dedup second — see the sibling arm for why that order is - # load-bearing rather than incidental. - kept = _rule_band(hits) - fresh = [(score, rule) for score, rule in kept if rule.id not in already] - - # Logged BEFORE the early return, for the reason spelled out at length - # on the write-path arm above: a call that found nothing is the only - # evidence a threshold is too high, and an arm that logs only the calls - # it liked reports a flawless clear-rate however badly it is tuned. - # This arm shipped with the same defect inherited from its sibling, and - # it mattered more here — a surface with no rows at all cannot be told - # apart from a hook that never fired, which is precisely the silent - # failure the arm was built to stop. - record_retrieval( - user_id=user_id, source="pre_tool_rule", query=query, - threshold=cfg["tool_rule_threshold"], limit=cfg["tool_rule_top_k"], - project_id=project_id, - is_task=None, results=fresh, duration_ms=duration_ms, - best_available=_rep_ptr.get("best_available_score"), - best_available_id=_rep_ptr.get("best_available_id"), - searched=bool(_rep_ptr.get("searched", True)), - # See the sibling arm, including why this now counts BOTH the - # ledger and the band. It matters more here: this arm fires on - # every Bash call, so a long session excludes its way to an - # all-zero row and the threshold looks wrong when nothing about it - # is. - suppressed=len(hits) - len(fresh), - ) - # `kept`, not `fresh` (#3750). A call whose only hit is a repeat still - # has something to say — the arm just says it differently. - if not kept: - return out - - lines = [ - _rule_hint_line( - rule, where=f"to this {tool_name} call", - seen=rule.id in already, - held=rule.id in held, - # Rank decides volume (#3851): the ranker's best guess gets the - # trigger, the rest get cited. - compact=idx > 0, - ) - for idx, (_score, rule) in enumerate(kept) - ] - # FRESH-ONLY, for the reason given on the sibling arm: a reference is a - # rendering decision, not a retrieval outcome, and counting it here - # would inflate the denominator pull_through is read from. - rule_ids = [rule.id for _score, rule in fresh] - - # 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. - # - # GUARDED, which it did not need to be before #3750: `fresh` can now be - # empty on a call that still emitted a line, and recording a surfacing - # of nothing would write an event with no rules in it. - if rule_ids: - 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 - # AFTER the lines, never instead of them. A checkpoint stops the act; - # it does not decide what the act should be told, and a reader who - # reads the rule and re-submits must find the same hint waiting. The - # two are independent renderings of one retrieval. - out["checkpoint"] = checkpoint_for( - kept, held=held, floor=cfg["checkpoint_threshold"], - where=f"this {tool_name} call", + result = await rp.run_rule_arm( + rp.PRE_TOOL_RULE, + rp.RuleMoment( + user_id=user_id, query=query, project_id=project_id, + where=f"to this {tool_name} call", + checkpoint_where=f"this {tool_name} call", + exclude=frozenset(exclude_rule_ids or []), + held=frozenset(held_rule_ids or []), + ), + floor=cfg["tool_rule_threshold"], budget=cfg["tool_rule_top_k"], + io=_rule_io(), checkpoint_floor=cfg["checkpoint_threshold"], ) + if result.lines: + out["context"] = "\n".join(result.lines) + out["rule_ids"] = result.rule_ids + out["checkpoint"] = result.checkpoint except Exception: logger.debug("pre-tool rule arm failed", exc_info=True) return out diff --git a/src/scribe/services/retrieval_pipeline.py b/src/scribe/services/retrieval_pipeline.py new file mode 100644 index 00000000..b375f51b --- /dev/null +++ b/src/scribe/services/retrieval_pipeline.py @@ -0,0 +1,637 @@ +"""One retrieval pipeline: a ranked surface is a spec, not a hand-copied arm. + +Milestone 456. Every ranked rule surface used to write out the same sequence +by hand — search, band, split fresh from repeats, log the call before any early +return, reserve a slot, render, record what was surfaced — once per arm, each +modelled on the nearest sibling. The copies drifted, and the defects that +mattered were fixed one copy at a time (#3497, #3750, #3752): "this arm shipped +with the same defect inherited from its sibling" was a comment in the code. +The invariants were held together by a parametrised test over three copies +rather than by there being one implementation. + +Here the sequence is written ONCE (`run_rule_arm`), and an arm is a `RuleArm`: +which source it records under, and which of the stages it takes. The +differences between arms that used to be buried in their copies are now four +booleans on one screen — and the ones that are accidents rather than +decisions are milestone 456 step 7's to settle, one at a time, on evidence. + +THE INVARIANTS, each written once: + + - the call row is written before any early return (#3497), because a call + that found nothing is the only evidence a bar is too high; + - the call row's results and the surfacing rows are both the FRESH cut, so + the two tables describe the same delivery (#3668, #3752); + - the session ledger decides how a repeat RENDERS, never whether it is + retrieved (#3750, #4101); + - band before dedup (#3851), so "you were shown this" never changes what + counts as relevant; + - telemetry never costs the reader a line, and the arm as a whole fails + open: a recall aid may never break the prompt, the command or the write. + +THE I/O IS PASSED IN (`RuleIO`), not imported. The pipeline is the policy; +which ranker it asks and which recorders it reports to belong to the caller. +The callers resolve them from their own module at call time. +""" +from __future__ import annotations + +import logging +import time +from dataclasses import dataclass, field +from typing import Any, Callable + +logger = logging.getLogger(__name__) + +# ── Shared rendering and ranking helpers (moved from plugin_context) ────── + +# MEASURED, NOT REASONED — and the reasoning it replaced was wrong (#3851). +# +# The prediction was that rules would rank SHARPLY, because `rule_document()` +# shapes them the way snippets are shaped — trigger in the embedded title and +# again above the body — and note 2485 measured snippets separating their top +# hit by 0.153 while every other kind managed 0.010–0.023. +# +# They do not. Three probes against real act queries, scored by the same +# embedding the arms use: +# +# `git push origin dev` top 0.757, gap to second 0.022 +# `docker compose up -d` top 0.685, gap to second 0.016 +# a bare-owner-filter query top 0.656, gap to second 0.020 +# +# That is dev-log territory, not snippet territory: rules arrive as a +# tightly-packed block. Shaping alone did not buy separation, which is worth +# recording because the opposite was the natural inference from 2485. +# +# So the band is narrow BECAUSE the corpus is flat. At 0.10 — the notes +# menu's value — every one of the top eight on the push probe falls inside, +# including a CI-registry rule and another project's branch policy. At 0.05 +# it admits roughly three ranks, which is the span where the scores are still +# saying something. 2485's Finding 3 is the standing caveat: no band value +# fixes a tie, and if rules ever rank as flat as dev-logs did this control +# stops working and the answer is a reranker (#1038), not a smaller number. +_RULEHINT_BAND = 0.05 + + +def _rule_band(hits: list) -> list: + """The top hit, plus every hit within `_RULEHINT_BAND` of it (#3851). + + The instrument that lets an act surface a SET without inventing one: a + fixed k fills its slots whether or not anything deserves them, while this + keeps only what the scores say is close, so one clearly-relevant rule + still shows one and four competing rules show four. + + Sync and pure, and deliberately its own function rather than a comparison + written twice — the two act arms are the pair #3497 records drifting apart + by being modelled on each other instead of sharing. + + Takes `(score, rule)` pairs already ordered best-first, as both + `semantic_search_*` helpers return them. + """ + if not hits: + return [] + top = hits[0][0] + return [(s, r) for s, r in hits if s >= top - _RULEHINT_BAND] + + +def checkpoint_for( + kept: list, *, held: set[int], floor: float, where: str, +) -> dict: + """The one rule, if any, that should STOP this act rather than annotate it. + + Sync and pure so it can be read against a fixed list of hits without a + database — the two arms share it for the reason `_rule_band` is shared: + #3497 is the record of these two drifting apart by being modelled on each + other instead of sharing one function. + + FOUR CONDITIONS, AND EACH IS A DIFFERENT KIND OF WRONG IT PREVENTS. + + 1. THE SCORE CLEARS `floor`. Not the arm's own floor — a much higher bar, + measured at `_CHECKPOINT_DEFAULT`. The hint arms keep nudging at their + floor; only a hit the corpus is confident about is allowed to stop + anything. + + 2. IT IS A RULE, NEVER A PREFERENCE. A preference says how something has + been done before and following it is what keeps work consistent; a rule + says what happens if you do not. Stopping an act over a preference + would assert a force the record explicitly does not claim, and + `_rule_hint_line` already keeps that distinction in the one word that + names it. + + 3. THE SESSION HAS NOT OPENED IT. `held` is observable — a PostToolUse + hook watches for the `get_rule` call (#4100) — so this is a recorded + event and not a model's self-report about its own context. A session + that read the rule has already had the thing the checkpoint exists to + produce, and stopping it again would be punishing the behaviour being + asked for. + + 4. IT IS THE TOP HIT. `kept` is a band, and a band's tail is there to let + an act surface a SET; the ranker's confidence claim attaches to its + first element only. A checkpoint raised on the fourth line of a band is + a stop justified by a score nobody claimed. + + Returns a dict rather than a rule or a tuple. Widening a tuple is an + interface change to every unpack site that the compiler does not report + (#4207, learned the expensive way in this same milestone's first week), and + this value crosses a JSON boundary into a shell script where a missing + field is a silently empty variable. + """ + if not kept or floor <= 0: + return {} + score, rule = kept[0] + if score < floor: + return {} + if getattr(rule, "kind", "") == "preference": + return {} + if rule.id in held: + return {} + found = { + "rule_id": rule.id, + "title": rule.title, + "trigger": (rule.when_to_apply or "").strip(), + "score": round(float(score), 4), + "where": where, + } + # RENDERED HERE, at the one call site, rather than by each arm. Two arms + # that each remember to render it are two arms that can stop agreeing on + # what a stop says — which is #3497's history for this exact pair. The + # text stays a separate function so it can be read and tested without a + # rule object, but nothing outside this line decides whether to call it. + found["reason"] = checkpoint_reason(found) + return found + + +def checkpoint_reason(checkpoint: dict) -> str: + """The text the agent reads INSTEAD of running the act. + + Written as a practice rather than a prohibition (rule 165): it says what + to do and why it is worth doing, not what is forbidden. The act is not + wrong — nothing here knows whether it is — and saying so plainly is what + keeps the stop from reading as an accusation the system is in no position + to make. + + It names the remedy as ONE call, because a stop whose remedy is vague + costs more than the miss it prevents. And it says the act may simply be + re-submitted afterwards, so a reader who finds the rule irrelevant is out + in two calls rather than negotiating with a hook. + """ + if not checkpoint: + return "" + trigger = checkpoint.get("trigger") or "" + return ( + f"Held for one read. “{checkpoint['title']}” is a standing rule " + f"this session has not opened, and it scores {checkpoint['score']} " + f"against what you are about to do" + + (f" ({trigger})" if trigger else "") + + f". Read it with get_rule({checkpoint['rule_id']}), then go ahead — " + f"re-submit this call unchanged if the rule does not apply, which is a " + f"judgement only you can make. Nothing here has decided the act is " + f"wrong; the rule is being put in front of it rather than beside it, " + f"because a rule delivered alongside a result arrives after the " + f"decision it was meant to inform." + ) + + +def _rule_hint_line( + rule, *, where: str, seen: bool, held: bool = False, compact: bool = False, +) -> str: + """One rule hint line — both arms, both tails, both kinds (#3750, #3849). + + THREE INDEPENDENT AXES SINCE #3851. `compact` joins `kind` and `seen`, and + like them it reads none of the others: it says how much ROOM this line + gets, which is a fact about its rank among today's hits rather than about + the rule. A compact line is still a full claim that the rule may apply — + it simply cites the rule instead of quoting its trigger. Crucially it + still carries the `seen` tail, so the three axes stay genuinely + independent: shortening a line must not decide what it says about whether + the session is holding the rule. + + WHY THE LATER LINES ARE QUIETER. Measured at #3851: a full line runs ~143 + tokens once the trigger is rendered, and #3855 tripled trigger lengths + across the corpus, so five full lines cost ~568 tokens before every Bash + call. Top-full-plus-references costs ~299 — about 2x the old single line, + for four more rules. The budget argument that once justified a single slot + was real; what it actually forbids is four voices at full volume, not four + voices. + + The top hit keeps the full rendering because it is the one the ranker is + most confident about, and a reader who acts on exactly one line should + have acted on that one. + + TWO INDEPENDENT AXES. `kind` decides the head, `seen` decides the tail, + and neither reads the other. A preference and a rule differ in force; a + repeat and a first surfacing differ in whether the session already holds + the line. Those are unrelated facts, and keeping them unrelated in the + code is what stopped the second kind from reopening the repeat question. + + ONE FUNCTION BECAUSE THE TAILS MUST NOT DRIFT. The two arms phrase their + heads differently ("may apply here" vs "may apply to this Bash call") and + that difference is deliberate. Everything after it must not differ, and + #3497's history is that the pre-tool arm inherited a defect from its + sibling by being modelled on it rather than sharing with it. Two copies of + a two-branch string is how one branch gets fixed and the other does not. + + WHY A REPEAT GETS A LINE AT ALL. Both arms used to drop a hit whose id was + already on the session's exclusion ledger and emit nothing. That is correct + only while the session still HOLDS what it was told, and a compaction + breaks exactly that: the earlier injection is summarized away while the id + stays on the ledger, so the rule is absent from context AND unreachable for + the rest of the session (#3749 closes the compaction half; this closes the + ordinary half, where a session simply stops holding a line it read an hour + ago). + + Only ONE CLAUSE of the original line is false on a repeat — the claim that + the rule is not in the session's loaded set. So only that clause changes. + Title, trigger and pull pointer are identical either way, the statement is + never injected either way, and a repeat therefore costs the same ~40 tokens + as a first surfacing and no more. + + DELIBERATELY NOT ASKING THE SESSION WHETHER IT HOLDS THE RULE. A model + asked "do you still hold rule 156?" will say yes, and the claim is + unverifiable self-report about its own context. The answer is also not + needed: the line is cheap enough to always emit and carries its own remedy + in both branches. Removing the question removes the fragility rather than + managing it. + """ + trigger = (rule.when_to_apply or "").strip() + preference = rule.kind == "preference" + # KIND CHANGES THE HEAD; `seen` CHANGES THE TAIL. The two axes are + # independent and stay that way, which is what lets the repeat logic above + # survive a second kind without being reasoned about again: whether a + # record is already on the ledger has nothing to do with how much force it + # carries, so the seen-branch is shared verbatim. + # + # The noun is the whole of the visual difference, and that is deliberate. + # A reader skimming an injected block gets one word to place the register \u2014 + # so it is the SECOND word that moves, and it is the word naming force. + # Everything structural after it is identical, so the three kinds read as + # one set rather than three formats (milestone 385's step 5 writes the + # lesson voice against these two; they are one paragraph, not three). + noun = "Preference" if preference else "Standing rule" + # The only other place force is asserted. A rule's line tells the reader + # not to dismiss it unread, because dismissing a rule unread is how the + # thing it prevents happens. A preference makes no such claim: it says + # where to find how this has been done, and following it is what keeps + # things consistent rather than what keeps them correct. + reason = ( + "for how this has been done before" if preference + else "before deciding it does not apply" + ) + # THREE STATES, BECAUSE TWO OF THEM WERE BEING TOLD THE SAME LIE (#4100). + # + # `seen` means an arm NAMED this rule earlier. It does not mean the session + # read it — the line is a teaser, and a teaser skimmed past leaves nothing + # behind, least of all after a compaction summarises the turn it arrived + # in. "You saw it earlier this session" asserted something about the + # reader's context that the server had no way to know. + # + # `held` is the observable half: a PostToolUse hook watches for the + # `get_rule` call itself, so this is a recorded EVENT rather than a claim. + # That distinction is what keeps the non-goal above intact — the objection + # was to asking a model about its own context, not to noticing what it did. + # + # The middle state is the honest one and the one that was missing: named, + # not opened. It gets the full invitation, because a session that skipped + # the teaser is in almost the same position as one that never saw it. + if held: + tail = ( + f"You opened it earlier this session; pull it with " + f"get_rule({rule.id}) again if you no longer hold it." + ) + elif seen: + tail = ( + f"Mentioned earlier this session but not opened — read it with " + f"get_rule({rule.id}) {reason}." + ) + else: + tail = ( + f"Read it with get_rule({rule.id}) {reason}; it is not in this " + "session's loaded set." + ) + if compact: + # THE TRIGGER GOES; THE TAIL STAYS. Only one of the two is expensive \u2014 + # a trigger runs 300-400 characters after #3855, the tail about 100 \u2014 + # so dropping the trigger is nearly the whole saving and dropping the + # tail would be mostly sacrifice. + # + # It would also destroy the one thing #3750 exists to say. The tail is + # what tells a reader whether they were already told this and may no + # longer be holding it, and that is the entire difference they can act + # on; a reference with no tail reads as a first surfacing whether it is + # one or not. This branch shipped without it for one commit and + # test_a_rule_the_session_already_holds_is_referenced_not_re_offered + # caught it, which is the guard working exactly as #3750 intended. + return ( + f"Also \u2014 {noun.lower()} \u201c{rule.title}\u201d. {tail}" + ) + return ( + f"{noun} that may apply {where} \u2014 \u201c{rule.title}\u201d" + + (f" ({trigger})" if trigger else "") + + f". {tail}" + ) + + +# ── The specs ──────────────────────────────────────────────────────────── + + +@dataclass(frozen=True) +class RuleArm: + """One ranked rule surface: where it records, and which stages it takes.""" + + source: str + """The telemetry `source`. MUST equal the registry and SURFACES key.""" + + band: bool + """Keep only the hits within `_RULEHINT_BAND` of the top one (#3851).""" + + compact_tail: bool + """Lines after the first cite the rule instead of quoting its trigger.""" + + checkpoint: bool + """The top hit may hold the act for one read (`checkpoint_for`).""" + + preference_slot: bool + """Reserve one line for a preference that lost the ranking (#3894).""" + + +# Today's differences, reproduced exactly (milestone 456 step 2). Whether the +# prompt arm should band, and whether the act arms should reserve a +# preference, are step 7's questions — not settled by this refactor. +PROMPT_RULE = RuleArm( + "prompt_rule", band=False, compact_tail=False, checkpoint=False, + preference_slot=True, +) +PRE_TOOL_RULE = RuleArm( + "pre_tool_rule", band=True, compact_tail=True, checkpoint=True, + preference_slot=False, +) +WRITE_PATH_RULE = RuleArm( + "write_path_rule", band=True, compact_tail=True, checkpoint=True, + preference_slot=False, +) +RULE_ARMS: tuple[RuleArm, ...] = (WRITE_PATH_RULE, PRE_TOOL_RULE, PROMPT_RULE) + +PREFERENCE_SLOT_SOURCE = "preference_slot" + +# Every source the shared stages below can record under — what the registry +# declares for this module's fan-out sites, since `source` reaches the +# recorders here as a value rather than a literal. +RULE_SOURCES: tuple[str, ...] = ( + *(arm.source for arm in RULE_ARMS), PREFERENCE_SLOT_SOURCE, +) + + +@dataclass(frozen=True) +class RuleIO: + """The ranker and the two recorders an arm reports to.""" + + search: Callable[..., Any] + """`semantic_search_rules`, or a stand-in with its signature.""" + + record_retrieval: Callable[..., Any] + record_rule_surfaced: Callable[..., Any] + + +@dataclass(frozen=True) +class RuleMoment: + """What is happening: the query an arm searches with, and the session.""" + + user_id: int + query: str + project_id: int + where: str + """How a line names the moment: "here", "to this Bash call", …""" + + checkpoint_where: str = "" + exclude: frozenset[int] = frozenset() + """Rules the session has been shown — rendered as repeats, not hidden.""" + + held: frozenset[int] = frozenset() + """Rules the session has OPENED (#4100).""" + + +@dataclass +class RuleResult: + """What an arm put in front of the reader, and what it counted.""" + + lines: list[str] = field(default_factory=list) + rule_ids: list[int] = field(default_factory=list) + """The FRESH cut — telemetry's denominator, never a repeat.""" + + shown_rule_ids: list[int] = field(default_factory=list) + """Every rule LINE, repeats and the reserved slot included.""" + + checkpoint: dict = field(default_factory=dict) + + +# ── The stages ─────────────────────────────────────────────────────────── + + +def _telemetry_failed(source: str) -> None: + """A recorder raised. Telemetry observes the surface and must never take it + down — or take a rendered line away from the reader, which an exception + reaching the arm's own fail-open would do. So it costs only its row. + + The recorders are called DIRECTLY at each site, inside their own try, + rather than through a wrapper taking the recorder as an argument: the + registry test finds recorder call sites by their name, and a wrapper would + hide every site in this module from it (#3191's narrowing). + """ + logger.debug("retrieval telemetry failed for %s", source, exc_info=True) + + +async def _ranked( + io: RuleIO, moment: RuleMoment, *, source: str, floor: float, limit: int, + band: bool = False, kind: str | None = None, +) -> tuple[list, list, list]: + """Search, band, split fresh from repeats, and log the call — every call. + + Returns (hits, kept, fresh): what the ranker returned, what the band kept, + and the kept hits this session has not been shown. The row is written + HERE, before any caller can return early (#3497), with `results` the fresh + cut and `suppressed` counting both the band and the ledger, so a long + session's all-repeat zero reads apart from a bar that turned everything + away. + """ + t0 = time.perf_counter() + report: dict = {} + kwargs: dict = {"limit": limit, "threshold": floor, "report": report, + "project_id": moment.project_id or None} + if kind: + # KIND-FILTERED, so the query can only answer with what was asked + # for. Verifying the kind afterwards would be weaker: an unfiltered + # search that happened to return a rule would spend a preference's + # slot on it, indistinguishable from a line that earned its place. + kwargs["kind"] = kind + hits = await io.search(moment.user_id, moment.query, **kwargs) + kept = _rule_band(hits) if band else hits + fresh = [(score, rule) for score, rule in kept if rule.id not in moment.exclude] + try: + io.record_retrieval( + user_id=moment.user_id, source=source, query=moment.query, + threshold=floor, limit=limit, project_id=moment.project_id, + is_task=None, results=fresh, + duration_ms=(time.perf_counter() - t0) * 1000.0, + best_available=report.get("best_available_score"), + best_available_id=report.get("best_available_id"), + searched=bool(report.get("searched", True)), + suppressed=len(hits) - len(fresh), + ) + except Exception: # noqa: BLE001 - observation never breaks the observed + _telemetry_failed(source) + return hits, kept, fresh + + +async def _reserve_slot_for_preference( + io: RuleIO, moment: RuleMoment, hits: list, *, floor: float, +) -> tuple[list, int | None]: + """Guarantee a preference one slot, if one clears the bar (#3894). + + THE ASYMMETRY THIS EXISTS FOR. A rule and a preference are not equally + served by a shared score contest, because their losses are not equal: + + - a RULE crowded out here can still fire at the act arm. A `git push` + reaches `pre_tool_rule`, a file write reaches `write_path_rule`. The + prompt hit is a preview of a second chance. + - a PREFERENCE about how to answer has no second chance. There is no + later act — the response IS the act — so crowded out here it is never + delivered at all. + + A straight ranking therefore favours the record whose loss is recoverable + over the one whose loss is total, and it does so INVISIBLY: the rule that + won is a legitimate hit, the telemetry looks healthy, and the only symptom + is a preference that quietly never arrives. `reuse_slot` exists for the + same shape one corpus over (#2463), where snippets kept losing to project + records that merely resembled the query. + + THE SLOT BUYS POSITION, NOT A LOWER BAR — same as `reuse_slot`, which also + reserves at `cfg["threshold"]`. A weak preference cannot buy the slot, so + silence stays the default and the reserved line is never worse than the + ones it sits beside. If `preference_slot` later shows a stream of + near-misses, `best_available_id` (#3807) names which preference was + refused and a separate bar becomes an argument with evidence behind it + rather than a knob added on a guess. + + LEDGER REPEATS STILL COUNT AS REPRESENTED. A preference already on the + session's ledger occupies the slot rather than being skipped for a fresh + one: it is still rendered (#3750), just with the tail that says so, and a + preference is the kind of record where being reminded is the point. + + Returns the possibly-extended hit list, and the id the slot spent — the + caller needs that to keep each source's surfaced set matching its own log + row (#3668), since the slot logs under its own name. + """ + if any(rule.kind == "preference" for _s, rule in hits): + return hits, None + + # ITS OWN SOURCE, and both sides of the trade logged. #2463's own finding + # is the warning rather than the precedent here: the hit that slot pushed + # OUT was in retrieval_logs while the query that pushed it out was not, so + # the slot could never be judged against what it displaced. + found, _kept, _fresh = await _ranked( + io, moment, source=PREFERENCE_SLOT_SOURCE, floor=floor, limit=1, + kind="preference", + ) + seen = {rule.id for _s, rule in hits} + slot = [(s, r) for s, r in found + if r.kind == "preference" and r.id not in seen][:1] + if not slot: + return hits, None + + slot_id = int(slot[0][1].id) + if slot_id not in moment.exclude: + try: + io.record_rule_surfaced( + user_id=moment.user_id, rule_ids=[slot_id], + source=PREFERENCE_SLOT_SOURCE, + ) + except Exception: # noqa: BLE001 - observation never breaks the observed + _telemetry_failed(PREFERENCE_SLOT_SOURCE) + # IT EXTENDS, IT NEVER DISPLACES — and here it parts company with + # `reuse_slot`, which evicts its menu's weakest hit. The reason is the + # ledger rather than taste. A displaced hit was RETURNED by the general + # search and is sitting in that call's `retrieval_logs` row, but would not + # have been shown — so `prompt_rule`'s surfaced set would stop matching + # its own log row, and #3668's identity would break for a reason nothing + # in the data explains. That identity is the cheapest true statement + # available about this pair of tables, and milestone #379 is what it costs + # to lose it: five steps planned against a gap that was two counters + # disagreeing, not a write path dropping rows. + # + # The price is one extra line, only when the general search already filled + # the limit AND a preference cleared the bar without placing. Cheap, and + # it buys a surface whose two tables can always be checked against each + # other. + return hits + slot, slot_id + + +# ── The pipeline ───────────────────────────────────────────────────────── + + +async def run_rule_arm( + arm: RuleArm, moment: RuleMoment, *, floor: float, budget: int, io: RuleIO, + checkpoint_floor: float = 0.0, +) -> RuleResult: + """Run one rule arm through every stage it takes, in the one order. + + search → band → fresh/repeat split → call row → reserved slot → render → + surfacing rows → checkpoint. Fails open to an empty result. + """ + try: + _hits, kept, fresh = await _ranked( + io, moment, source=arm.source, floor=floor, limit=budget, + band=arm.band, + ) + shown = kept + if arm.preference_slot: + # BEFORE the bail-out, and the ordering is load-bearing. An empty + # general result is not proof that no preference qualifies: the + # general search overfetches by distance and then collapses, so a + # preference ranked below that window is invisible to it while a + # kind-filtered query finds it at once. + shown, _slot_id = await _reserve_slot_for_preference( + io, moment, shown, floor=floor, + ) + # `shown`, not `fresh` (#3750): a call whose only hit is a repeat + # still has something to say, it just says it differently. + if not shown: + return RuleResult() + + lines = [ + _rule_hint_line( + rule, where=moment.where, + seen=rule.id in moment.exclude, + held=rule.id in moment.held, + # Rank decides volume (#3851): the ranker's best guess gets + # the trigger, the rest get cited. + compact=arm.compact_tail and idx > 0, + ) + for idx, (_score, rule) in enumerate(shown) + ] + # FRESH-ONLY (#3752): a repeat is a rendering decision, not a + # retrieval outcome, and counting it would inflate the denominator + # pull_through is read from. It is also exactly what this call's own + # row recorded, so the two tables stay equal (#3668). RANKED, not + # ambient: the source is in `rule_usage.RANKED_SOURCES`. + rule_ids = [rule.id for _score, rule in fresh] + source = arm.source + if rule_ids: + try: + io.record_rule_surfaced( + user_id=moment.user_id, rule_ids=rule_ids, source=source, + ) + except Exception: # noqa: BLE001 - observation never breaks the observed + _telemetry_failed(source) + checkpoint = ( + checkpoint_for( + kept, held=set(moment.held), floor=checkpoint_floor, + where=moment.checkpoint_where, + ) + if arm.checkpoint else {} + ) + return RuleResult( + lines=lines, rule_ids=rule_ids, + shown_rule_ids=[rule.id for _score, rule in shown], + checkpoint=checkpoint, + ) + except Exception: # noqa: BLE001 - a recall aid never breaks its act + logger.debug("%s arm failed", arm.source, exc_info=True) + return RuleResult() diff --git a/src/scribe/services/retrieval_registry.py b/src/scribe/services/retrieval_registry.py index 2b5e9147..8dd2ef49 100644 --- a/src/scribe/services/retrieval_registry.py +++ b/src/scribe/services/retrieval_registry.py @@ -42,6 +42,8 @@ from __future__ import annotations from dataclasses import dataclass +from scribe.services.retrieval_pipeline import RULE_ARMS, RULE_SOURCES + # ── How a point is reached ──────────────────────────────────────────────── # # UNBIDDEN: fires on its own, against a query the agent did not write — a @@ -258,6 +260,16 @@ FAN_OUT_SITES: dict[str, tuple[str, ...]] = { "enter_project", "start_planning", "get_task", "get_project", "get_milestone", ), + # The one retrieval pipeline (milestone 456): every rule arm's call row and + # surfacing rows are written by the same two sites, so `source` arrives as + # a value. The values are the pipeline's own specs, read rather than + # restated, so an arm added there is declared here by construction. + "scribe/services/retrieval_pipeline.py::record_retrieval(source=source)": ( + RULE_SOURCES + ), + "scribe/services/retrieval_pipeline.py::record_rule_surfaced(source=source)": ( + tuple(arm.source for arm in RULE_ARMS) + ), } diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index a397abe3..32b61645 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -887,80 +887,76 @@ async def test_a_command_the_arm_never_searched_writes_no_row_at_all(): log.assert_not_called() -def test_neither_rule_arm_logs_its_call_behind_a_results_guard(): - """Structural, on top of the behavioural pair above, because the defect was - one level of indentation and it appeared INDEPENDENTLY in two places — the - pre-tool arm inherited it by being modelled on its sibling. The third arm - modelled on either of them is the one this catches. +def test_no_rule_arm_logs_its_call_behind_a_results_guard(): + """Structural, on top of the behavioural tests above, because the defect + was one level of indentation and it appeared INDEPENDENTLY in two arms — + the pre-tool arm inherited it by being modelled on its sibling (#3497). + + Since milestone 456 there is one implementation to guard rather than a + copy per arm, so the property is checked where it is written: the + pipeline's ranked-search stage writes the call row unconditionally, the + arm runner calls that stage before it can return, and the surfacing row + alone stays behind `if rule_ids:`. And no arm writes a rule-source row of + its own any more — a fourth copy would be the defect's way back in. """ - pc_src = Path("src/scribe/services/plugin_context.py").read_text() + from scribe.services import retrieval_pipeline as rp - # Write-path arm: what remains inside `if fresh:` is the SURFACING log only. - guarded = pc_src.split('source="write_path_rule", query=code or path')[1] - guarded = guarded.split("if fresh:")[1].split("except Exception:")[0] - assert "record_rule_surfaced" in guarded, "the surfacing log must stay guarded" - assert "record_retrieval" not in guarded, ( - "the call log is back inside the results guard — a call that found " - "nothing is the only evidence a threshold is set too high" + src = Path("src/scribe/services/retrieval_pipeline.py").read_text() + tree = ast.parse(src) + fns = {n.name: n for n in ast.walk(tree) if isinstance(n, ast.AsyncFunctionDef)} + + def _calls(fn, name): + return [ + n.lineno for n in ast.walk(fn) + if isinstance(n, ast.Call) + and (getattr(n.func, "attr", None) == name or getattr(n.func, "id", None) == name) + ] + + # 1. The stage logs, and nothing in it can return before it does. + ranked = fns["_ranked"] + logged_at = _calls(ranked, "record_retrieval") + assert logged_at, "the ranked-search stage no longer writes the call row" + early = [n.lineno for n in ast.walk(ranked) + if isinstance(n, ast.Return) and n.lineno < min(logged_at)] + assert not early, ( + f"the ranked-search stage can return at line {early[0]} before logging " + f"its call — a call that found nothing is the only evidence a bar is " + f"too high (#3497)" ) - # Pre-tool arm: the call log comes BEFORE the early return. - # - # WALKED, NOT SUBSTRING-MATCHED (rule 167). This assertion used to read - # `body.index("if not fresh:")`, which pinned the name of a local variable - # rather than the property. #3750 changed that guard to `if not hits:` — - # the arm still logs before returning, so the property held perfectly, and - # a name-matching assertion would have raised ValueError and reported the - # #3497 defect as back. A guard that cries regression when the thing it - # protects is intact is the failure mode rule 167 names. - # - # The property is positional: between the search and the first guard that - # can return early, the call row has already been written. - # The arm's BODY, which since #4633 lives in `_tool_rule_hint`; the public - # `build_tool_rule_hint` is a wrapper that adds the via-lesson step and - # searches nothing itself. Located by what it does — the function that - # calls the rule search under the pre_tool_rule source — so the next - # rename moves the guard with it instead of emptying it. - fn = next( - n for n in ast.walk(ast.parse(pc_src)) - if isinstance(n, ast.AsyncFunctionDef) - and any( - isinstance(c, ast.Call) and getattr(c.func, "id", None) == "semantic_search_rules" - for c in ast.walk(n) - ) - and any( - isinstance(k, ast.keyword) and k.arg == "source" - and isinstance(k.value, ast.Constant) and k.value.value == "pre_tool_rule" - for k in ast.walk(n) - ) - ) - search_at = min( - n.lineno for n in ast.walk(fn) - if isinstance(n, ast.Call) - and getattr(n.func, "id", None) == "semantic_search_rules" - ) - logged_at = min( - n.lineno for n in ast.walk(fn) - if isinstance(n, ast.Call) - and getattr(n.func, "id", None) == "record_retrieval" - ) - # Every `if : return ...` after the search — whatever it tests. + # 2. The runner reaches the stage before its first early return. + run = fns["run_rule_arm"] + staged_at = min(_calls(run, "_ranked")) bailouts = [ - n.lineno for n in ast.walk(fn) - if isinstance(n, ast.If) and n.lineno > search_at - and any(isinstance(b, ast.Return) for b in n.body) + n.lineno for n in ast.walk(run) + if isinstance(n, ast.If) and any(isinstance(b, ast.Return) for b in n.body) ] assert bailouts, ( - "no early return found after the search in the pre-tool arm — the " - "guard has nothing left to protect, which means this test is now " - "passing vacuously rather than the arm being correct" + "no early return found in run_rule_arm — this check has nothing left " + "to protect and would pass vacuously" ) - assert logged_at < min(bailouts), ( - f"the pre-tool arm returns at line {min(bailouts)} before logging its " - f"call at line {logged_at} — a surface with no rows at all cannot be " - f"told apart from a hook that never fired (#3497)" + assert staged_at < min(bailouts), ( + f"run_rule_arm returns at line {min(bailouts)} before the call row is " + f"written at line {staged_at}" ) + # 3. The surfacing row is the guarded one. + guards = [n for n in ast.walk(run) if isinstance(n, ast.If) + and isinstance(n.test, ast.Name) and n.test.id == "rule_ids"] + assert guards and any(_calls(g, "record_rule_surfaced") for g in guards), ( + "the surfacing row must stay behind `if rule_ids:` — nothing was shown, " + "so no surfacing occurred" + ) + + # 4. No arm keeps a private copy: outside the pipeline, nothing writes a + # call row under a rule arm's source. + pc_src = Path("src/scribe/services/plugin_context.py").read_text() + for arm in rp.RULE_ARMS: + assert f'source="{arm.source}"' not in pc_src, ( + f"plugin_context writes a {arm.source} row of its own again — the " + f"arm is meant to be a spec over the one pipeline" + ) + # ── Suppression: which zeros were the ranker, which were repeats (#3497) ── #