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/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_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" + ) 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." + )