From c4908f093f2ac1d402b868f1b8e14096e7e05cf9 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 21:48:50 -0400 Subject: [PATCH 1/2] feat(rules): a repeat is referenced, not withheld (#3750, #3752) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both arms used to drop a hit 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, leaving the rule absent from context AND unreachable for the rest of the session. #3749 closed the compaction half by clearing the ledger; this closes the ordinary half, where a session simply stops holding a line it read an hour ago. Only ONE CLAUSE of the existing line is false on a repeat — the claim that the rule is not in the session's loaded set — so only that clause changes. The fresh line is byte-identical to what it was. Both tails now come from one `_rule_hint_line`. The arms phrase their heads differently on purpose; everything after must not differ, and #3497's history is that the pre-tool arm inherited a defect by being modelled on its sibling rather than sharing with it. THE BUDGET DECISION, recorded at RULEHINT_LIMIT. A repeat competes for the single slot on rank alone: nothing is fetched behind it, and it never rides alongside as a second line. Promoting a fresh rule past a better-ranked repeat would reinstate the withholding one rank deeper, and a second line is the one thing the limit exists to forbid. The consequence is deliberate — a rule that keeps ranking first for a recurring situation keeps being referenced, and its decay belongs to exclusion ageing (#3751), not to a first-place rule being demoted for having won before. THE TELEMETRY, decided before shipping rather than after a number moved (#3752): nothing changes. A reference is a RENDERING decision, not a retrieval outcome. `results` stays `fresh`, `suppressed` stays len(hits) - len(fresh), and `record_rule_surfaced` still counts only what the arm freshly chose. This matters more than it reads: the naive implementation drops the `fresh` filter and 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 in the code and no longer working. A test asserts the counters as unmoved, because "nothing changed" is only worth something if it is checkable. Also corrects two comments that outlived #3702 — both arms still claimed CONDITIONAL ONLY while the module-level note above RULEHINT_LIMIT said the opposite, in the exact code this change edits. GUARDS - an already-held hit produces a rule line at all (the regression), asserted on `get_rule()` rather than a truthy context: the write-path arm fills its context from four other sources, so truthiness passes under the OLD behaviour and pins nothing there. - the two tails are distinguishable, each excludes the other, and neither injects the rule statement — the budget claim, both branches. - the counters are unmoved, per #3752. - test_a_rule_the_session_already_holds_is_not_re_offered asserted the old contract (`"161" not in context`). Repinned rather than deleted: the telemetry half of what it protected still holds. Repinned the #3497 log-placement guard on structure (rule 167). It read `body.index("if not fresh:")` — a local variable NAME, not the property. This change renames that guard to `if not hits:`, so the old assertion would have raised ValueError and reported #3497 as back while the arm was entirely correct. Now walks the AST for the first early return after the search and asserts the call row is written before it. Falsified both ways: it fails on the #3497 mutation, and it refuses to pass when no early return exists at all. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- src/scribe/services/plugin_context.py | 148 ++++++++++++---- tests/test_rule_usage_wiring.py | 235 +++++++++++++++++++++++++- 2 files changed, 347 insertions(+), 36 deletions(-) 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." + ) -- 2.54.0 From c1aa1d8e92b7f3e629e74d5311262343fca651cb Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 22:21:32 -0400 Subject: [PATCH 2/2] feat(plugin): rule exclusions age, so salience decays without a context event (#3751) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3749 clears the ledger when an EVENT destroys context — a compaction, a /clear. This is the case with no event at all: a long session where a rule was named two hundred turns ago and has simply fallen out of attention. It is #3702's argument at the tier level — present in context and salient at the moment are different properties — applied to time instead of to tier. FORMAT: `idepoch`, per entry. Not a whole-file mtime: that is one line of shell and wrong in exactly the session that needs it, since a single recent write keeps every stale id alive, and the ids that go stale first come from the rules that fire most. Not a turn counter, though it would be the truer model — an idle session does not forget. A hook has no turn number without keeping its own, which is a second piece of session state to write, read, clear on compaction and get wrong. Wall time costs a `date` call. The failure it accepts is a session left idle over lunch treating its rules as forgotten, worth one extra full line per rule and nothing else. TTL 2700s (45 minutes), reasoned rather than picked (rule 32). About one working stretch on a single task: long enough that a rule does not re-announce itself while you are still doing the thing it governs, short enough that a multi-hour session gets a refresh rather than one 9am mention. It leans short because since #3750 being wrong on the short side is the cheaper error — an expired entry costs one full line instead of one short one, and the exclusion re-arms the moment it is spent. There is no data on this yet; #3807's near-miss listing is what should revise it. BOTH READERS THROUGH ONE HELPER, in scribe_defs.sh. The two hooks share one ledger so a rule named by one arm is not re-offered by the other; a format only one of them understood would break that on the first read. The flat `tr '\n' ','` read would now send `1561789002860` as an exclude id — verified, which is why this is not a per-hook edit. THE LAST ENTRY FOR AN ID WINS. The file is append-only, so a rule that ages out, is surfaced fresh and is appended again has two lines. Reading the first leaves it permanently expired, and it then re-announces itself on every call for the rest of the session — the mechanism meant to quieten things becoming the loudest thing in the hint. A BARE ID IS LIVE. That is the pre-#3751 format, and every session in flight when this ships has a ledger full of them. Reading unknown as EXPIRED would make all of those sessions re-announce every rule they had already been told, at once — the exact noise this prevents, delivered by the feature on the day it ships. Unknown means "not measured", never "old", the same discipline the nullable retrieval_logs columns use. GUARDS (tests/test_rule_ledger_ageing.py, real shell, no credentials) - old ages out AND recent survives, in ONE assertion (rule 167): either half alone passes against a broken helper — "old is gone" passes against one returning nothing, "recent survives" passes against the flat read this replaces, i.e. against the defect itself. - the ping-pong case, which is the one that costs the most to get wrong. - a bare id is live, including beside a stale stamped one. - missing/empty ledger excludes nothing. - an id repeated in the ledger appears once, with no empty list element. - structural: neither hook reads the rule ledger flat again — pinned on the flat-read SHAPE, so a rename of the helper is not a failure and a hook that ages correctly some other way is not either. The TTL's VALUE is deliberately not asserted. The tests read the constant out of the shell and assert the property around it, so a later tuning change stays a tuning change instead of a red build. Dropped a boundary test (`ttl` vs `ttl + 1`) before committing: racy by construction, since a ledger written at T is read at T+n and the two cases swap. A one-second distinction on a 45-minute window is also not observable behaviour, so it pinned a flake rather than a property. Plugin version minted to 2026.09.10.0221 — this is entirely hook-side, so without the bump the cache would never pick it up and the merge would ship nothing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_defs.sh | 103 ++++++++++++++ plugin/hooks/scribe_prior_art.sh | 12 +- plugin/hooks/scribe_tool_rules.sh | 11 +- tests/test_rule_ledger_ageing.py | 216 ++++++++++++++++++++++++++++++ 5 files changed, 333 insertions(+), 11 deletions(-) create mode 100644 tests/test_rule_ledger_ageing.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index c3fe04c..59a9fdd 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "2026.09.09.0408", + "version": "2026.09.10.0221", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 93a2b39..9da0088 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -16,6 +16,9 @@ # scribe_reached STATE SID the server answered: the next outage speaks again # scribe_config sets `url` + `token` from the env, returns 0 # only if BOTH are usable (#2278) +# scribe_rules_live FILE live rule ids from the exclusion ledger, +# comma-joined; entries age out (#3751) +# scribe_rules_append FILE stdin ids -> the ledger, timestamped # # Sourced, not executed: `. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh"`. @@ -175,3 +178,103 @@ scribe_unreached() { scribe_reached() { rm -f "$1/$2.unreached" 2>/dev/null || true } + +# --------------------------------------------------------------------------- +# THE RULE EXCLUSION LEDGER, and how entries in it AGE (#3751). +# +# Two hooks write this file and two read it, which is the whole reason the +# parsing lives here: `scribe_prior_art.sh` and `scribe_tool_rules.sh` share +# one ledger so a rule named by one arm is not re-offered by the other, and a +# format only one of them understood would break that on the first read. +# +# WHAT PROBLEM AGEING SOLVES. #3749 clears the ledger when an EVENT destroys +# context — a compaction or a /clear. This is the case with no event at all: a +# long session where the rule was named two hundred turns ago and has simply +# fallen out of attention. It is #3702's argument at the tier level (present in +# context and salient at the moment are different properties) applied to time +# instead of to tier. +# +# PER-ENTRY TIMESTAMPS, NOT A FILE MTIME. Clearing the whole ledger when the +# file is old is one line of shell and wrong in exactly the session that needs +# it: a single recent write keeps every stale id alive, and the ids that go +# stale first are the ones from the rules that fire most. +# +# WALL TIME, NOT A TURN COUNT, and the trade is real rather than dismissed. A +# turn count is a truer model of salience — an idle session does not forget — +# but a hook has no turn number without keeping its own counter, which is a +# second piece of session state to write, read, clear on compaction and get +# wrong. Wall time is available from `date` and costs nothing. The failure mode +# it accepts is a session left idle over lunch treating its rules as forgotten, +# which produces one extra full line per rule and no other harm. +_SCRIBE_RULE_TTL=2700 + +# 45 MINUTES, and the reasoning rather than the number (rule 32). +# +# There is no data on this yet, so it is a judgement made to be revised — the +# telemetry that would settle it is the one #3807 just built, and a reading of +# how often an aged-out rule gets PULLED after it returns is what should move +# this. +# +# Too short and the exclusion stops existing and the repetition it prevents +# comes back. Too long and it never fires at all in a session short enough to +# matter. 45 minutes is about one working stretch on a single task: long enough +# that a rule does not re-announce itself while you are still doing the thing +# it governs, short enough that a multi-hour session gets a genuine refresh +# rather than one 9am mention. +# +# Being wrong on the short side is now the cheaper error, which is why this +# leans short. Since #3750 an excluded rule is REFERENCED rather than withheld, +# so the ledger is no longer the only thing standing between a session and a +# rule it has forgotten — an expired entry costs one full line instead of one +# short one, and the exclusion re-arms the moment it is spent. + +# Live ids from a ledger, comma-joined for `exclude_rule_ids`. Empty output for +# a missing, empty or fully-aged file — the callers already treat "" as "send +# no exclusions". +# +# THE LAST ENTRY FOR AN ID WINS, and this is what stops a rule ping-ponging. +# The file is append-only, so a rule that ages out, gets surfaced fresh and is +# appended again has TWO lines. Reading the first would leave it permanently +# expired and it would re-announce itself on every single call from then on — +# the loudest possible failure, from the mechanism meant to quieten things. +# Appends are chronological, so the last line for an id is its most recent. +# +# A BARE ID — no tab, no timestamp — IS LIVE. That is the pre-#3751 format, and +# a session in flight when this ships has a ledger full of them. Treating +# unknown as expired would make every one of those sessions re-announce every +# rule it had already been told, all at once, which is precisely the noise this +# exists to prevent. Unknown means "not measured" and never "old" — the same +# null discipline the retrieval_logs columns use. Those entries simply never +# age, which is bounded: the session ends. +scribe_rules_live() { + local f="$1" now + [ -n "$f" ] && [ -f "$f" ] || return 0 + now=$(date +%s 2>/dev/null) || now=0 + awk -F'\t' -v now="$now" -v ttl="$_SCRIBE_RULE_TTL" ' + { + id = $1 + gsub(/[^0-9]/, "", id) + if (id == "") next + if (!(id in seen)) { seen[id] = 1; seq[++n] = id } + stamp[id] = ($2 ~ /^[0-9]+$/) ? $2 : "" + } + END { + out = "" + for (i = 1; i <= n; i++) { + id = seq[i] + if (stamp[id] != "" && now > 0 && (now - stamp[id]) > ttl) continue + out = out (out == "" ? "" : ",") id + } + print out + } + ' "$f" 2>/dev/null || true +} + +# Append surfaced ids, stamped. Reads ids on stdin, one per line — the shape +# `jq -r '(.rule_ids // [])[]?'` already produces at both call sites. +scribe_rules_append() { + local f="$1" now + [ -n "$f" ] || return 0 + now=$(date +%s 2>/dev/null) || now=0 + awk -v ts="$now" 'NF { print $1 "\t" ts }' >> "$f" 2>/dev/null || true +} diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index 67242be..04f2cd7 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -204,10 +204,11 @@ if [ -n "$session_id" ]; then derive_seen=$(tr '\n' ',' < "$derivefile" 2>/dev/null | sed 's/,$//' | jq -sRr '@uri' 2>/dev/null) || derive_seen="" [ -n "$derive_seen" ] && derive_exclude_q="&exclude_derive=${derive_seen}" fi - if [ -f "$rulefile" ]; then - rule_seen=$(tr '\n' ',' < "$rulefile" 2>/dev/null | sed 's/,$//') - [ -n "$rule_seen" ] && rule_exclude_q="&exclude_rule_ids=${rule_seen}" - fi + # Ageing, not a flat read (#3751), and the ONLY ledger here that ages: the + # note channels above are a different question with a different answer, and + # this arm's sibling hook reads the same rule file through the same helper. + rule_seen=$(scribe_rules_live "$rulefile") + [ -n "$rule_seen" ] && rule_exclude_q="&exclude_rule_ids=${rule_seen}" fi # Not `|| exit 0`: an unreachable instance must not discard a local finding @@ -239,7 +240,8 @@ if [ -n "$body" ]; then printf '%s' "$body" | jq -r '(.sync_note_ids // [])[]?' 2>/dev/null >> "$syncfile" || true fi if [ -n "$rulefile" ]; then - printf '%s' "$body" | jq -r '(.rule_ids // [])[]?' 2>/dev/null >> "$rulefile" || true + printf '%s' "$body" | jq -r '(.rule_ids // [])[]?' 2>/dev/null \ + | scribe_rules_append "$rulefile" fi if [ -n "$derivefile" ]; then printf '%s' "$body" | jq -r '(.derive_keys // [])[]?' 2>/dev/null >> "$derivefile" || true diff --git a/plugin/hooks/scribe_tool_rules.sh b/plugin/hooks/scribe_tool_rules.sh index 6803a78..2d68977 100644 --- a/plugin/hooks/scribe_tool_rules.sh +++ b/plugin/hooks/scribe_tool_rules.sh @@ -86,10 +86,10 @@ rule_exclude_q="" if [ -n "$session_id" ]; then safe_sid=$(printf '%s' "$session_id" | tr -c 'A-Za-z0-9._-' '_') rulefile="$state_dir/${safe_sid}.rules.ids" - if [ -f "$rulefile" ]; then - rule_seen=$(tr '\n' ',' < "$rulefile" 2>/dev/null | sed 's/,$//') - [ -n "$rule_seen" ] && rule_exclude_q="&exclude_rule_ids=${rule_seen}" - fi + # Ageing, not a flat read (#3751): an id named two hours ago is not one the + # session is still holding. scribe_rules_live carries the reasoning. + rule_seen=$(scribe_rules_live "$rulefile") + [ -n "$rule_seen" ] && rule_exclude_q="&exclude_rule_ids=${rule_seen}" fi # `|| exit 0` here, unlike the prior-art hook: there is no local arm whose @@ -104,7 +104,8 @@ context=$(printf '%s' "$body" | jq -r '.context // empty' 2>/dev/null) || exit 0 # Remember what was named so it is not repeated this session. if [ -n "$rulefile" ]; then - printf '%s' "$body" | jq -r '(.rule_ids // [])[]?' 2>/dev/null >> "$rulefile" || true + printf '%s' "$body" | jq -r '(.rule_ids // [])[]?' 2>/dev/null \ + | scribe_rules_append "$rulefile" fi jq -cn --arg ctx "$context" '{ diff --git a/tests/test_rule_ledger_ageing.py b/tests/test_rule_ledger_ageing.py new file mode 100644 index 0000000..a0e3807 --- /dev/null +++ b/tests/test_rule_ledger_ageing.py @@ -0,0 +1,216 @@ +"""Entries in the rule exclusion ledger age out (#3751). + +WHY THIS EXISTS + +#3749 clears the ledger when an EVENT destroys context — a compaction, a +/clear. This covers the case with no event at all: a long session where a rule +was named two hundred turns ago and has simply fallen out of attention. Same +argument #3702 made at the tier level (present in context and salient at the +moment are different properties), applied to time instead of to tier. + +WHAT IS PINNED, AND WHAT DELIBERATELY IS NOT + +Not the TTL's value. 2700 is a judgement made to be revised once there is +telemetry to revise it with, and a test asserting `== 2700` would turn a +legitimate tuning change into a red build — which is how a number nobody may +touch gets one. The tests read the constant out of the shell and assert the +PROPERTY around it, so any TTL works and only a broken comparison fails. + +Runs the real shell, like `test_session_context_ledger` and the after-write +hook's tests, and deliberately with no SCRIBE_URL/SCRIBE_TOKEN: ageing is +local, keyless and networkless, and must work on an instance that is +unreachable or was never configured. +""" +from __future__ import annotations + +import os +import re +import shutil +import subprocess +import time +from pathlib import Path + +import pytest + +PLUGIN = Path(__file__).resolve().parents[1] / "plugin" / "hooks" +DEFS = PLUGIN / "scribe_defs.sh" + + +def _ttl() -> int: + """The TTL as the shell defines it — never a copy of the number.""" + m = re.search(r"^_SCRIBE_RULE_TTL=(\d+)", DEFS.read_text(), re.M) + assert m, "_SCRIBE_RULE_TTL is gone from scribe_defs.sh" + return int(m.group(1)) + + +def _live(ledger: Path) -> list[str]: + """`scribe_rules_live` against a real ledger, as the hooks call it.""" + for tool in ("bash", "awk"): + if shutil.which(tool) is None: + pytest.skip(f"hook runtime tool {tool!r} not installed") + proc = subprocess.run( + ["bash", "-c", f'. "{DEFS}"; scribe_rules_live "{ledger}"'], + capture_output=True, text=True, timeout=30, + env={"PATH": os.environ["PATH"]}, # no credentials — see the docstring + ) + assert proc.returncode == 0, proc.stderr + out = proc.stdout.strip() + return out.split(",") if out else [] + + +def _write_ledger(ledger: Path, entries: list[tuple[str, int | None]]) -> None: + """A ledger written in AGES rather than absolute stamps. + + Named for what it is: `tests/helpers._now` is a datetime for record + fixtures and this file wants the shell's epoch, so they are deliberately + not the same helper and deliberately not the same name. `time.time()` + rather than shelling out to `date` — both read the one system clock, so + the subprocess bought nothing. + """ + now = int(time.time()) + ledger.write_text("".join( + f"{rid}\n" if ago is None else f"{rid}\t{now - ago}\n" + for rid, ago in entries + )) + + +def test_an_old_entry_ages_out_and_a_recent_one_does_not(tmp_path): + """BOTH DIRECTIONS IN ONE ASSERTION (rule 167), because either half alone + passes against a broken implementation. + + A test that only checks the old id is gone passes against a helper that + returns nothing at all — which would drop every exclusion and bring back + the repetition the ledger exists to prevent. A test that only checks the + recent id survives passes against the flat `tr '\\n' ','` read this + replaces, i.e. against the defect itself. + """ + ttl = _ttl() + ledger = tmp_path / "s.rules.ids" + _write_ledger(ledger, [("156", ttl + 600), ("168", 60)]) + + assert _live(ledger) == ["168"], ( + f"expected the entry {ttl + 600}s old to age out and the 60s-old one " + f"to survive (TTL {ttl}s). An empty list means nothing is excluded any " + f"more; both ids means nothing ages." + ) + + +def test_a_repeatedly_surfaced_rule_appears_once_in_the_exclusion_list(tmp_path): + """List hygiene, and it is not cosmetic. + + The ledger is append-only, so a rule surfaced five times has five lines. + The output is spliced straight into `&exclude_rule_ids=` and parsed by the + server as a list; duplicates make that list grow without bound over a long + session, and a trailing or doubled comma is an empty element the parser has + to decide what to do with. Neither is visible from the hint. + + (There was a boundary test here — `ttl` vs `ttl + 1`. It was racy by + construction: a ledger written at T is read at T+n, so the entry ages by + however long the subprocess took and the two cases swap. A one-second + distinction on a 45-minute window is also not a behaviour anyone can + observe, so it was pinning a flake rather than a property.) + """ + ledger = tmp_path / "s.rules.ids" + _write_ledger(ledger, [("156", 60), ("168", 30), ("156", 10), ("156", 5)]) + + live = _live(ledger) + assert live == ["156", "168"], ( + f"got {live}: an id repeated in the ledger must appear once in the " + f"exclusion list, in first-seen order" + ) + raw = subprocess.run( + ["bash", "-c", f'. "{DEFS}"; scribe_rules_live "{ledger}"'], + capture_output=True, text=True, timeout=30, + env={"PATH": os.environ["PATH"]}, + ).stdout.strip() + assert ",," not in raw and not raw.endswith(","), ( + f"the exclusion list has an empty element: {raw!r}" + ) + + +def test_a_rule_that_ages_out_and_returns_does_not_ping_pong(tmp_path): + """THE FAILURE THIS COSTS THE MOST TO GET WRONG. + + The ledger is append-only, so a rule that ages out, gets surfaced fresh and + is appended again has TWO lines. An implementation reading the FIRST leaves + it permanently expired — and it then re-announces itself on every single + call for the rest of the session. The mechanism meant to quieten things + becomes the loudest thing in the hint. + """ + ttl = _ttl() + ledger = tmp_path / "s.rules.ids" + _write_ledger(ledger, [("156", ttl + 600)]) + + subprocess.run( + ["bash", "-c", f'. "{DEFS}"; printf \'156\\n\' | scribe_rules_append "{ledger}"'], + capture_output=True, text=True, timeout=30, + env={"PATH": os.environ["PATH"]}, + ) + + assert _live(ledger) == ["156"], ( + "a re-surfaced rule read as still expired; the most recent entry for " + "an id must win, or it re-announces itself on every call from here on" + ) + + +def test_a_bare_id_from_the_old_format_is_treated_as_live(tmp_path): + """The compatibility decision, asserted rather than described. + + Every session in flight when this ships has a ledger of bare ids. Reading + unknown as EXPIRED would make all of them re-announce every rule they had + already been told, all at once — the exact noise this feature exists to + prevent, delivered by the feature itself on the day it ships. + + Unknown means "not measured", never "old" — the same discipline the + nullable retrieval_logs columns use. Those entries never age, which is + bounded, because the session ends. + """ + ttl = _ttl() + ledger = tmp_path / "s.rules.ids" + ledger.write_text("156\n168\n") + assert _live(ledger) == ["156", "168"] + + # And a bare id keeps its own meaning next to a stale stamped one, rather + # than the file falling back to one format or the other wholesale. + _write_ledger(ledger, [("9", None), ("156", ttl + 600)]) + assert _live(ledger) == ["9"] + + +def test_a_missing_or_empty_ledger_excludes_nothing(tmp_path): + """Fail-open, like every other path in these hooks: no ledger means no + exclusions, not an error and not a stray comma the server would parse as + an id.""" + assert _live(tmp_path / "never-written.rules.ids") == [] + empty = tmp_path / "empty.rules.ids" + empty.write_text("") + assert _live(empty) == [] + + +def test_both_hooks_read_the_ledger_through_the_shared_helper(): + """Structural, because the defect this would reintroduce is invisible. + + Two hooks share one ledger so a rule named by one arm is not re-offered by + the other. If either goes back to reading the file flat, that hook stops + ageing while its sibling keeps ageing — the two disagree about which rules + are live, and nothing in the output says so. + + Pinned on the flat-read SHAPE rather than on a helper name: a hook that + stops calling the helper but still ages correctly some other way is not a + defect, and a rename of the helper is not one either. + """ + for name in ("scribe_prior_art.sh", "scribe_tool_rules.sh"): + src = (PLUGIN / name).read_text() + rule_reads = [ + ln for ln in src.splitlines() + if "rulefile" in ln and "tr " in ln and r"'\n' ','" in ln + ] + assert not rule_reads, ( + f"{name} reads the rule ledger flat again: {rule_reads}. That " + f"hook's exclusions would stop ageing while its sibling's keep " + f"ageing, and the two would disagree silently about which rules " + f"this session still holds." + ) + assert "scribe_rules_live" in src, ( + f"{name} no longer reads the rule ledger through the shared " + f"helper, so the two hooks can drift on the ledger format" + ) -- 2.54.0