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,25 +1553,31 @@ 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.
+        #
+        # 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",
             )

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 <cond>: 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."
+    )