diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 3a21d4c..1b9d6d2 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -149,6 +149,37 @@ RULEHINT_DEFAULT_THRESHOLD = 0.72 # adds a way to misconfigure the surface (rule 25 cuts both ways). RULEHINT_LIMIT = 1 +# AND A REPEAT COMPETES FOR THAT ONE SLOT ON RANK ALONE (#3750). +# +# 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? +# +# It takes the slot, and nothing is fetched behind it. Two reasons. +# +# 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 +# actually being taken. Overfetching in order to promote the fresh one past it +# would reinstate exactly the withholding this milestone exists to remove, one +# rank deeper and harder to see — recency is not a reason to show a worse +# 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. +# +# The consequence is deliberate and worth naming: a rule that keeps ranking +# first for a recurring situation keeps being referenced, every time the +# situation recurs. That is the intended behaviour — the situation recurring +# IS the trigger — and its decay belongs to exclusion ageing (#3751), not to +# a rule that ranks first being quietly demoted for having won before. + # WHY THE ARMS NO LONGER FILTER TO ONE TIER (#3702). # # Both arms used to pass `tier="conditional"`, on the reasoning that an @@ -827,6 +858,52 @@ async def get_writepath_config(user_id: int) -> dict: "rule_threshold": rule_threshold, } +def _rule_hint_line(rule, *, where: str, seen: bool) -> str: + """One rule hint line — both arms, both tails (#3750). + + 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() + tail = ( + f"You saw it earlier this session; pull it with get_rule({rule.id}) " + "if you no longer hold it." + if seen else + f"Read it with get_rule({rule.id}) before deciding it does not " + "apply; it is not in this session's loaded set." + ) + return ( + f"Standing rule 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, @@ -1287,9 +1364,13 @@ async def build_write_path_hint( # resembles what is being written, noticed at the moment it is relevant # rather than by being resident in every session. # - # CONDITIONAL ONLY. An always-on rule is already in the session; repeating - # it here would be noise, and noise on a hint that fires on every write is - # how a hint gets ignored. + # EVERY TIER, since #3702 — see the note at RULEHINT_LIMIT. This comment + # used to read "CONDITIONAL ONLY: an always-on rule is already in the + # session, so repeating it here would be noise". That conflated being + # PRESENT in context with being SALIENT at the moment the action is taken, + # and it is the same conflation #3750 corrects one layer up: a rule the + # session was told about an hour ago is not a rule in front of the reader + # now. # # Fails open like every other arm: a rule hint must never break a write. rule_ids: list[int] = [] @@ -1308,15 +1389,18 @@ async def build_write_path_hint( ) rule_ms = (time.perf_counter() - rule_t0) * 1000.0 fresh = [(score, rule) for score, rule in hits if rule.id not in already] - for _score, rule in fresh: - trigger = (rule.when_to_apply or "").strip() + # EVERY hit gets a line; `already` only changes the tail (#3750). + for _score, rule in hits: lines.append( - f"Standing rule that may apply here — \u201c{rule.title}\u201d" - + (f" ({trigger})" if trigger else "") - + f". Read it with get_rule({rule.id}) before deciding it " - "does not apply; it is not in this session's loaded set." + _rule_hint_line(rule, where="here", seen=rule.id in already) ) - rule_ids.append(rule.id) + # `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) # 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 @@ -1412,10 +1496,10 @@ async def build_tool_rule_hint( which tools it watches, so widening the matcher is a `hooks.json` edit with no change here. - CONDITIONAL ONLY, exactly as the write-path arm — an always-on rule is - already resident and repeating it is noise. That filter is also the - transition this arm exists to enable: re-tier a rule to `conditional` and - it starts arriving here instead of in every session's preamble. + EVERY TIER, since #3702 — the tier filter this docstring used to describe + is gone from both arms, for the reason recorded at RULEHINT_LIMIT: present + in context and salient at the moment are different properties, and only the + second is what this arm is for. Fails open and returns an empty context on any error: a recall aid may never break the operator's action. @@ -1469,28 +1553,34 @@ async def build_tool_rule_hint( # and the threshold looks wrong when nothing about it is. suppressed=len(hits) - len(fresh), ) - if not fresh: + # `hits`, 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: return out - lines: list[str] = [] - rule_ids: list[int] = [] - for _score, rule in fresh: - trigger = (rule.when_to_apply or "").strip() - lines.append( - f"Standing rule that may apply to this {tool_name} call — " - f"“{rule.title}”" - + (f" ({trigger})" if trigger else "") - + f". Read it with get_rule({rule.id}) before deciding it " - "does not apply; it is not in this session's loaded set." + lines = [ + _rule_hint_line( + rule, where=f"to this {tool_name} call", + seen=rule.id in already, ) - rule_ids.append(rule.id) + for _score, rule in hits + ] + # 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. - record_rule_surfaced( - user_id=user_id, rule_ids=rule_ids, source="pre_tool_rule", - ) + # + # 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 except Exception: diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index 050ca89..462c113 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -10,6 +10,7 @@ end in two different doors, and the property under test is that they meet. Split across three module-shaped files, "both ends are wired" is a thing no single test asserts. """ +import ast from contextlib import ExitStack from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch @@ -472,14 +473,40 @@ async def test_the_tool_arm_is_a_ranked_source(): @pytest.mark.asyncio -async def test_a_rule_the_session_already_holds_is_not_re_offered(): +async def test_a_rule_the_session_already_holds_is_referenced_not_re_offered(): + """WAS `..._is_not_re_offered`, asserting `"161" not in out["context"]`. + + That assertion was the old contract and #3750 deliberately reverses half of + it: a rule already on the ledger now gets a line with a different tail + instead of being dropped in silence. Withholding was only ever right while + the session still HELD what it was told, and a compaction breaks exactly + that while leaving the id excluded. + + The half that survives is the half that was always about telemetry rather + than rendering: an already-held rule stays out of `rule_ids`, so it reaches + neither the exclusion ledger (where it already is) nor the surfacing count + (which a reference must not inflate). + """ rec = MagicMock() hits = [(0.71, fake_rule(id=161, title="Reach the forge through its MCP tools")), (0.70, fake_rule(id=12, title="Don't run a local stack unless asked"))] out = await _run_tool_arm(hits, rec, exclude_rule_ids=[161]) - assert out["rule_ids"] == [12] - assert "161" not in out["context"] + assert out["rule_ids"] == [12], ( + "a referenced rule was counted as surfaced; only the fresh one was " + "actually chosen by this arm" + ) + assert "get_rule(161)" in out["context"], ( + "the held rule vanished instead of being referenced (#3750)" + ) + assert "get_rule(12)" in out["context"], ( + "the fresh rule was lost while adding the reference — both belong in " + "the hint, and the reference must not displace the surfacing" + ) + assert _SEEN_TAIL in out["context"] and _FRESH_TAIL in out["context"], ( + "one call rendered two hits in different states and gave them the same " + "tail; the tails are the entire difference a reader can act on" + ) @pytest.mark.asyncio @@ -705,10 +732,46 @@ def test_neither_rule_arm_logs_its_call_behind_a_results_guard(): ) # Pre-tool arm: the call log comes BEFORE the early return. - body = pc_src.split("async def build_tool_rule_hint")[1] - assert body.index('source="pre_tool_rule"') < body.index("if not fresh:"), ( - "the pre-tool arm returns before logging its call — a surface with no " - "rows at all cannot be told apart from a hook that never fired" + # + # 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. + fn = next( + n for n in ast.walk(ast.parse(pc_src)) + if isinstance(n, ast.AsyncFunctionDef) and n.name == "build_tool_rule_hint" + ) + 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. + 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) + ] + assert bailouts, ( + "no early return found after the search in build_tool_rule_hint — the " + "guard has nothing left to protect, which means this test is now " + "passing vacuously rather than the arm being correct" + ) + 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)" ) @@ -866,3 +929,161 @@ async def test_both_recorders_report_the_same_rules_for_one_call( "the fixture stopped exercising what it claims to; check the exclusion " "filter still runs before both recorders" ) + + +# ── A repeat is REFERENCED, not withheld (#3750) ────────────────────────── +# +# Both arms used to drop a hit already on the session's exclusion ledger and +# emit nothing at all. That is right only while the session still HOLDS what it +# was told — and it stops being right the moment a compaction summarizes the +# earlier injection away while the id stays on the ledger, which leaves the +# rule absent from context AND unreachable for the rest of the session. +# +# The tests below pin the emitted LINE, not the prose describing it, and both +# arms are parametrized through one body so the two tails cannot drift apart — +# #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. + +_HELD = fake_rule( + id=156, + title="A wait with no deadline is a bug", + statement="Every wait on something that can fail to answer carries a deadline.", + when_to_apply="writing any call that crosses a process boundary", +) +_FRESH_TAIL = "not in this session's loaded set" +_SEEN_TAIL = "You saw it earlier this session" + + +@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"]) +@pytest.mark.asyncio +async def test_a_rule_already_on_the_ledger_still_produces_a_line(source, run): + """THE REGRESSION, stated as the thing that used to be absent. + + Falsified against the old behaviour: before #3750 both arms filtered to + `fresh` before rendering, so an all-excluded call returned an empty + context and this assertion fails on `context == ""`. + """ + out = await run([(0.81, _HELD)], MagicMock(), exclude_rule_ids=[156]) + + # ON `get_rule(156)` RATHER THAN A TRUTHY CONTEXT. The write-path arm's + # context also carries the prior-art menu, the staleness line and the shape + # signals, so `assert out["context"]` is TRUE under the old behaviour and + # would pin nothing on that arm while looking identical to a real check on + # the other. The rule line is the only part of the string this changes. + assert "get_rule(156)" in out["context"], ( + f"{source} emitted no rule line for a rule the session had already " + f"been shown. Silence is only correct while the session still holds " + f"the line — after a compaction it does not, and the id is still on " + f"the ledger, so the rule is unreachable for the rest of the session. " + f"Context was: {out['context']!r}" + ) + assert _HELD.title in out["context"], ( + "the reference names no rule; a pull pointer with nothing attached " + "gives a reader no way to judge whether it is worth pulling" + ) + + +@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"]) +@pytest.mark.asyncio +async def test_the_two_tails_are_distinguishable_and_say_the_true_one(source, run): + """One clause differs, and it is the clause that would otherwise be false. + + A repeat rendered with the FRESH tail would assert "it is not in this + session's loaded set" about a rule this session was handed twenty minutes + ago — a line that is wrong in the one way a reader cannot check. + """ + fresh_ctx = (await run([(0.81, _HELD)], MagicMock()))["context"] + seen_ctx = (await run([(0.81, _HELD)], MagicMock(), + exclude_rule_ids=[156]))["context"] + + assert _FRESH_TAIL in fresh_ctx and _SEEN_TAIL not in fresh_ctx, ( + f"{source} rendered a first surfacing with the repeat tail" + ) + assert _SEEN_TAIL in seen_ctx and _FRESH_TAIL not in seen_ctx, ( + f"{source} told the session a rule it has already been shown is not " + f"in its loaded set" + ) + assert fresh_ctx != seen_ctx, "the two tails collapsed into one" + + +@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"]) +@pytest.mark.asyncio +async def test_neither_tail_injects_the_rule_statement(source, run): + """The budget, pinned on both branches. + + A reference costs the same ~40 tokens as a first surfacing precisely + because neither carries the statement. If a repeat ever starts inlining the + body "since we are re-showing it anyway", this arm stops being cheap enough + to always emit — and always emitting is the whole mechanism. + """ + for excluded in ([], [156]): + ctx = (await run([(0.81, _HELD)], MagicMock(), + exclude_rule_ids=excluded))["context"] + assert _HELD.statement not in ctx, ( + f"{source} inlined the rule statement (excluded={excluded!r}); the " + f"line carries title, trigger and a pull pointer and nothing more" + ) + assert _HELD.title in ctx and "process boundary" in ctx, ( + "the line dropped the title or the trigger — those are what let a " + "reader decide whether to pull without pulling" + ) + + +# ── What a reference IS in the telemetry: nothing new (#3752) ───────────── +# +# THE RELATION, STATED BEFORE IT SHIPS. #3750 changes what is RENDERED and +# nothing about what is COUNTED: +# +# result_count counts fresh surfacings — unchanged +# suppressed_count counts repeats — unchanged +# rule_usage counts fresh surfacings — unchanged +# +# A reference is a rendering decision, not a retrieval outcome. That answer is +# not free: the naive implementation renders repeats by dropping the `fresh` +# filter, which takes `suppressed_count` to zero everywhere — and #3739's +# near-miss fix identifies repeat-caused zeros by `suppressed_count > 0`, so +# the contamination corrected on 2026-09-08 would return by a different route, +# in the same field, with the fix still sitting in the code not working. +# +# #3712 gave a reader `complete_from` for a counter that started late. Nothing +# tells a reader a counter's DEFINITION moved. This test is the cheap version +# of that guarantee: the claim "nothing moved" is only worth anything if it is +# checkable, so it is asserted rather than described. + + +@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"]) +@pytest.mark.asyncio +async def test_a_reference_is_rendered_but_not_counted(source, run): + """The counters must read exactly as they did before #3750.""" + log, rec = MagicMock(), MagicMock() + out = await run([(0.81, _HELD)], rec, retrieval_log=log, + exclude_rule_ids=[156]) + + row = next(c for c in log.call_args_list + if c.kwargs.get("source") == source).kwargs + assert row["results"] == [], ( + f"{source} counted a referenced rule as a result. `result_count` " + f"drives zero_result_calls and the whole threshold picture; a repeat " + f"is not evidence the bar is set correctly." + ) + assert row["suppressed"] == 1, ( + f"{source} stopped reporting the repeat as suppressed. #3739's " + f"near_misses predicate excludes declines with suppressed_count > 0 — " + f"if this reads 0, every repeat-caused zero is re-counted as a genuine " + f"ranker rejection and the near-miss contamination returns." + ) + assert rec.call_count == 0, ( + f"{source} recorded a surfacing for a rule it only referenced, which " + f"inflates pull_through's denominator with a choice the arm never made" + ) + assert out["rule_ids"] == [], ( + "a referenced id went back to the hook for the exclusion ledger; it is " + "already there by definition, and returning it conflates 'shown fresh' " + "with 'mentioned again'" + ) + assert "get_rule(156)" in out["context"], ( + "guard is passing vacuously — no rule line was rendered, so 'rendered " + "but not counted' is not what this run demonstrated. Truthiness of the " + "whole context will not do: the write-path arm fills it from four " + "other sources." + )