diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 1e7d329..46a4f0b 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -129,34 +129,76 @@ WRITEPATH_DEFAULT_THRESHOLD = 0.68 RULEHINT_THRESHOLD_KEY = "kb_rulehint_threshold" RULEHINT_DEFAULT_THRESHOLD = 0.72 -# ONE rule per write, not two — and this is deliberately NOT a knob. +# A SET OF RULES PER ACT, NOT THE SINGLE BEST ONE (#3851). # -# With a corpus this small, top-k does as much damage as the threshold: k=2 -# over a few dozen candidates means the second line is almost always the -# second-best noise, arriving with the same confident framing as the first. -# Halving k halves that regardless of where the bar sits. +# This was 1, and the reasoning for that is kept below rather than deleted +# because it was correct for the world it was written in and the half of it +# that still holds is what shapes the replacement. # -# It also BOUNDS the blast radius of widening the pool (below): with k=1 a -# wider corpus can change WHICH rule surfaces and how often one does, but it -# can never make a single hint longer. The loudness of one hint and the -# eligibility of a rule are separate controls, and only one of them moved. +# THE OLD ARGUMENT. "With a corpus this small, top-k does as much damage as +# the threshold: k=2 over a few dozen candidates means the second line is +# almost always the second-best noise, arriving with the same confident +# framing as the first." True — and note the premise. Retrieval was then a +# SUPPLEMENT to a 33-rule resident set, so the arm's job was to add one +# salient rule beside everything the session already held. One was the right +# number for an accent. # -# It stays a constant because it is a decision about how LOUD one hint may be, -# not a per-install tuning question. The hint already carries prior art, shape -# signals and staleness; rules are the fourth voice in it, and a fourth voice -# that speaks twice is where a reader stops reading. Nothing suggests an -# operator wants this different, and a knob nobody turns is a knob that only -# adds a way to misconfigure the surface (rule 25 cuts both ways). -RULEHINT_LIMIT = 1 +# WHAT CHANGED. Milestone 394 removes residency, and then this arm is not the +# accent, it is the whole delivery. Moments genuinely need several rules at +# once: `git push origin dev` is governed by 1 (`dev` is home), 2 (never +# `main` unasked), 9 (poll CI) and 140 (let each action land) simultaneously, +# and each alone permits the mistake the others catch. One slot cannot serve +# that, and "the single best" is not a coherent answer when four rules bind. +# +# WHY A CAP PLUS A BAND, RATHER THAN A BIGGER CAP. The old argument's real +# content is that a fixed k invents lines — it fills slots whether or not +# anything deserves them. A band does not: it keeps what is close to the top +# and nothing else, so a moment with one clearly-relevant rule still shows +# one, and a moment with four shows four. The corpus decides, not a constant. +# The cap survives as a ceiling on the worst case, not as the usual answer. +RULEHINT_LIMIT = 5 -# AND A REPEAT COMPETES FOR THAT ONE SLOT ON RANK ALONE (#3750). +# 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 + +# AND A REPEAT COMPETES ON RANK ALONE (#3750), WHICH SURVIVES THE BAND. # # Since #3750 a hit already on the session's exclusion ledger is RENDERED # rather than dropped, which raises a question the old behaviour never had to # answer: when the top-ranked hit is one the session has already seen, does it -# take the slot, or step aside for a fresh rule behind it? +# take its place, or step aside for a fresh rule behind it? # -# It takes the slot, and nothing is fetched behind it. Two reasons. +# It keeps its place, and nothing is promoted past it. #3851 widened the arm +# from one slot to a banded set and did NOT reopen this: a repeat still ranks +# where it ranks, and the band is applied to scores with no regard for what +# the session has seen. The two are independent, exactly as `kind` and `seen` +# are independent in the renderer — rank answers "what is relevant now" and +# the ledger answers "have you been told", and neither is evidence about the +# other. Two reasons, both unchanged by the widening. # # RANK IS THE ANSWER TO "WHAT IS RELEVANT NOW". If the repeat scores 0.85 and # the best fresh candidate 0.73, the repeat is the better match for the action @@ -166,13 +208,21 @@ RULEHINT_LIMIT = 1 # match, and "you have seen this" is not the same claim as "you are holding # this". # -# AND A SECOND LINE IS THE ONE THING THE LIMIT ABOVE FORBIDS. Letting a repeat -# ride alongside a fresh rule means two hint lines, and the paragraph above is -# entirely about why a fourth voice that speaks twice is where a reader stops -# reading. A reference costs the same ~40 tokens as a first surfacing, so -# "it is only a short extra line" is not available as an argument: the budget -# is one line because of what a second line does to the whole hint, not -# because of what it costs. +# AND THE COST OF A SECOND LINE IS NOW PAID DIFFERENTLY, NOT WISHED AWAY. +# This paragraph used to read "a second line is the one thing the limit above +# forbids", on the strength of "a reference costs the same ~40 tokens as a +# first surfacing". Both halves are now wrong and the second was already +# wrong when written: a full line is ~143 tokens once the trigger is rendered, +# and #3855's rewrite of the corpus roughly tripled trigger length, so five +# full lines on a push probe measure ~646 tokens before EVERY Bash call. +# +# That number is why the widening pairs with a compact rendering rather than +# arriving alone (see `_rule_hint_line`). The old paragraph's instinct — that +# a fourth voice which speaks at full volume twice is where a reader stops +# reading — is the half worth keeping, and it is answered by making the +# later lines quieter rather than by refusing to have them. Top hit full, +# the rest as references: ~198 tokens, 1.4x the old single line, for four +# more rules. # # The consequence is deliberate and worth naming: a rule that keeps ranking # first for a recurring situation keeps being referenced, every time the @@ -1123,9 +1173,48 @@ async def get_writepath_config(user_id: int) -> dict: "rule_threshold": rule_threshold, } -def _rule_hint_line(rule, *, where: str, seen: bool) -> str: +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 _rule_hint_line(rule, *, where: str, seen: bool, 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. + + 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 ~646 tokens before every Bash + call. Top-full-plus-references costs ~198 — 1.4x 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 @@ -1192,6 +1281,14 @@ def _rule_hint_line(rule, *, where: str, seen: bool) -> str: f"Read it with get_rule({rule.id}) {reason}; it is not in this " "session's loaded set." ) + if compact: + # No trigger, and a shortened tail. What survives is everything a + # reader needs in order to DECIDE to pull it: the force (noun), the + # identity (title) and the pointer. What goes is the trigger, which is + # the expensive half and the half the top line already demonstrates. + return ( + f"Also \u2014 {noun.lower()} \u201c{rule.title}\u201d: get_rule({rule.id})." + ) return ( f"{noun} that may apply {where} \u2014 \u201c{rule.title}\u201d" + (f" ({trigger})" if trigger else "") @@ -1682,11 +1779,21 @@ async def build_write_path_hint( report=_rep_wpr, ) rule_ms = (time.perf_counter() - rule_t0) * 1000.0 - fresh = [(score, rule) for score, rule in hits if rule.id not in already] - # EVERY hit gets a line; `already` only changes the tail (#3750). - for _score, rule in hits: + # 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) + _rule_hint_line( + rule, where="here", seen=rule.id in already, + 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 @@ -1733,10 +1840,15 @@ async def build_write_path_hint( best_available=_rep_wpr.get("best_available_score"), best_available_id=_rep_wpr.get("best_available_id"), searched=bool(_rep_wpr.get("searched", True)), - # What the ranker found and this session had already been told. - # Without it a zero row cannot say whether the bar was too high or - # the reader was simply ahead of it — and only the first is a - # reason to move the threshold. + # 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: @@ -1824,7 +1936,10 @@ async def build_tool_rule_hint( 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] + # 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 @@ -1842,22 +1957,27 @@ async def build_tool_rule_hint( 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. 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. + # 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), ) - # `hits`, not `fresh` (#3750). A call whose only hit is a repeat still + # `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 hits: + if not kept: return out lines = [ _rule_hint_line( rule, where=f"to this {tool_name} call", seen=rule.id in already, + # Rank decides volume (#3851): the ranker's best guess gets the + # trigger, the rest get cited. + compact=idx > 0, ) - for _score, rule in hits + 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 diff --git a/tests/test_rule_hint_band.py b/tests/test_rule_hint_band.py new file mode 100644 index 0000000..af52253 --- /dev/null +++ b/tests/test_rule_hint_band.py @@ -0,0 +1,169 @@ +"""An act surfaces a SET of rules, and rank decides how loudly (#3851). + +WHY THIS EXISTS + +`RULEHINT_LIMIT` was 1. That was right while retrieval merely SUPPLEMENTED a +33-rule resident set — one salient rule beside everything already loaded. It +stops being right the moment milestone 394 removes residency, because then +this arm is the whole delivery, and `git push origin dev` is governed by +rules 1, 2, 9 and 140 at once, each of which alone permits the mistake the +others catch. + +Two instruments, and they answer different questions: + + - `_rule_band` decides HOW MANY. A fixed k fills its slots whether or not + anything deserves them; a band keeps only what scored close to the top, + so one clearly-relevant rule still shows one. + - `compact` decides HOW LOUD. Measured at #3851: a full line is ~143 tokens + once the trigger is rendered, so five of them cost ~646 before every Bash + call. Top-full-plus-references costs ~198. + +WHAT THIS PINS + +Structure, never wording — the lines are prose and will be rewritten: + + 1. The band keeps the top hit and everything within `_RULEHINT_BAND`, and + drops what falls outside. Falsified below from both sides: a hit just + inside survives, a hit just outside does not. + 2. Rank decides volume — the first line carries the trigger, later lines do + not, and every line names its rule's id so any of them can be pulled. + 3. The band reads SCORES ONLY. A top hit the session has already seen still + anchors the band, and its score still sets the cutoff. This is the axis + independence the renderer already keeps between `kind` and `seen`, and + the regression it prevents is subtle: letting the ledger reorder the + band would make "you were told this" change what counts as relevant. + +The band width itself is deliberately NOT pinned. It is a tuning value with +a comment recording the measurement behind it, and a test asserting 0.05 +would fail on every future retune while proving nothing about behaviour — +so the cases below express their scores as offsets from the constant. +""" +import pytest + +from scribe.services.plugin_context import ( + _RULEHINT_BAND, + _rule_band, + _rule_hint_line, +) +from tests.helpers import fake_rule + +_TRIGGER = "about to run git push with an earlier CI run still unread" + + +def _hit(score: float, rule_id: int): + return (score, fake_rule(id=rule_id, title=f"rule {rule_id}", + when_to_apply=_TRIGGER)) + + +def test_an_empty_result_stays_empty(): + """No hits is not a crash and not a phantom line.""" + assert _rule_band([]) == [] + + +def test_the_band_keeps_a_hit_just_inside_it(): + """The whole point: a close second rule reaches the agent.""" + top = 0.75 + hits = [_hit(top, 1), _hit(top - _RULEHINT_BAND + 0.01, 2)] + assert [r.id for _s, r in _rule_band(hits)] == [1, 2] + + +def test_the_band_drops_a_hit_just_outside_it(): + """And the band must actually BIND, or it is a fixed k wearing a hat.""" + top = 0.75 + hits = [_hit(top, 1), _hit(top - _RULEHINT_BAND - 0.01, 2)] + assert [r.id for _s, r in _rule_band(hits)] == [1] + + +def test_one_clearly_better_rule_still_surfaces_alone(): + """The behaviour the old limit of 1 got right, which must not regress. + + A moment with a single relevant rule shows one line, because the corpus + said so — not because a constant capped it. + """ + hits = [_hit(0.80, 1), _hit(0.55, 2), _hit(0.54, 3)] + assert [r.id for _s, r in _rule_band(hits)] == [1] + + +def test_a_flat_cluster_surfaces_together(): + """Measured shape of this corpus: adjacent rules sit ~0.02 apart. + + Four rules governing one act is the `git push` case the step exists for, + and at the measured spacing they must arrive together rather than the + ranker picking one of four near-ties. + """ + hits = [_hit(0.757, 2), _hit(0.735, 7), _hit(0.726, 1), _hit(0.711, 9)] + assert [r.id for _s, r in _rule_band(hits)] == [2, 7, 1, 9] + + +def test_the_band_is_computed_from_scores_not_from_the_ledger(): + """A seen top hit still anchors the band (#3750 x #3851). + + `_rule_band` never learns what the session has seen — dedup happens after + it, in the arms. Pinned here because the tempting "reorder so a fresh rule + leads" would silently change the cutoff, and the failure is invisible: the + arm would still emit lines, just the wrong set. + """ + hits = [_hit(0.80, 1), _hit(0.78, 2), _hit(0.60, 3)] + kept = _rule_band(hits) + # Independent of any `already` set, because it is not consulted. + assert [r.id for _s, r in kept] == [1, 2] + + +@pytest.mark.parametrize("seen", [False, True]) +def test_the_leading_line_carries_the_trigger(seen): + """Rank 0 gets the full rendering, on either tail.""" + line = _rule_hint_line( + fake_rule(id=4, title="dev is home", when_to_apply=_TRIGGER), + where="to this Bash call", seen=seen, compact=False, + ) + assert _TRIGGER in line + assert "get_rule(4)" in line + + +@pytest.mark.parametrize("seen", [False, True]) +def test_a_later_line_cites_its_rule_without_quoting_the_trigger(seen): + """Rank > 0 is a reference: identity and pointer, no trigger. + + The trigger is the expensive half — it is what made a full line ~143 + tokens — and the leading line has already demonstrated the shape. Both + assertions matter: dropping the trigger is the saving, and keeping + `get_rule(id)` is what makes the saving safe, because a cited rule the + reader cannot pull is just noise. + """ + line = _rule_hint_line( + fake_rule(id=4, title="dev is home", when_to_apply=_TRIGGER), + where="to this Bash call", seen=seen, compact=True, + ) + assert _TRIGGER not in line + assert "dev is home" in line + assert "get_rule(4)" in line + + +def test_a_compact_line_is_materially_shorter_than_a_full_one(): + """The cost claim, asserted rather than asserted-in-a-comment. + + Not a token count — that would pin the tokenizer. Half the characters is + the property that makes widening the arm affordable, and it is what fails + if a later edit reintroduces the trigger into the compact branch. + """ + rule = fake_rule(id=4, title="dev is home", when_to_apply=_TRIGGER) + full = _rule_hint_line(rule, where="here", seen=False, compact=False) + compact = _rule_hint_line(rule, where="here", seen=False, compact=True) + assert len(compact) * 2 < len(full) + + +def test_a_preference_keeps_its_noun_when_compact(): + """Force survives the shortening (#3849). + + `kind` and `compact` are independent axes. A preference rendered as a + reference must still not read as a rule — the noun is the whole of the + visual difference, so losing it in the compact branch would make every + cited preference bind. + """ + line = _rule_hint_line( + fake_rule(id=5, title="pace debugging", kind="preference", + when_to_apply=_TRIGGER), + where="here", seen=False, compact=True, + ) + assert "preference" in line.lower() + assert "standing rule" not in line.lower()