From c61f7301bc2a0a28c0608527bccaa5aa91b2630b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 16 Sep 2026 21:00:42 -0400 Subject: [PATCH 01/12] fix(plugin): a compaction clears every session ledger, not the two on the list (#4101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scribe_session_context.sh` cleared `.rules.ids` and `.opened.ids` by name and left `.ids`, `.sync.ids` and `.derive.ids` standing, under a comment asserting that was a decision. Reading the note arms says it was not: their exclusions go straight into `semantic_search_notes`, so a surfaced note leaves the result set rather than being rendered as a reference the way #3750 gave a repeated rule, and unlike the rules ledger they never age. Hard, permanent, never cleared — a note surfaced in a session's first minute is unreachable for the rest of it, which is milestone 386's own defect alive on the arms that fire most often. The list was the bug, so the fix is not a longer list. `scribe_clear_session_ ledgers` matches the naming convention instead — a per-session ledger is `[.].ids` — which covers all five and covers the sixth on the day it is written. `.unreached` is deliberately outside it: that records an outage, not held context, and #2932 needs it to survive. tests/test_session_ledger_clear.py runs the hook rather than grepping it for `rm -f`, since grepping for the names is the pattern being removed. It pins both directions — `compact`/`clear` take all five, `startup`/`resume` take none — plus the convention the glob rests on, checked against the hooks themselves so a ledger named outside it fails loudly instead of silently never clearing. Also drops a stale comment pointing at a rules-etag marker that milestone 394 retired. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_defs.sh | 30 +++++ plugin/hooks/scribe_session_context.sh | 51 +++++--- tests/test_rule_opened_ledger.py | 18 +-- tests/test_session_ledger_clear.py | 161 +++++++++++++++++++++++++ 5 files changed, 229 insertions(+), 33 deletions(-) create mode 100644 tests/test_session_ledger_clear.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 3aa5c75..d2b1158 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe for Claude Code: connects the scribe MCP server, adds the hooks that deliver live project state and relevant records at the right moment, ships the shared client-neutral Scribe skills (using-scribe, writing-plans, reporting-back, systematic-debugging, verification, brainstorming, reusing-code, shape-accounting), and syncs your saved Scribe Processes as skills (/scribe:sync).", - "version": "2026.09.16.2102", + "version": "2026.09.17.0100", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 0331abe..542824c 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -308,6 +308,36 @@ scribe_held_query() { return 0 } +# Drop EVERY per-session ledger, matched by convention rather than listed (#4101). +# +# A LIST IS THE BUG. Until now the compact/clear branch named its files one at +# a time, and it named two of the five: `.rules.ids` and `.opened.ids` were +# cleared while `.ids` (notes), `.sync.ids` (shape signals) and `.derive.ids` +# survived. So milestone 386's defect — "a compaction destroys the context but +# not the ledger, so the most applicable records become permanently +# unreachable mid-session" — was fixed for rules and left standing on the note +# and snippet surfaces, which are the ones that fire most often. +# +# Nobody decided that. The list was written when rules were the only ledger +# that mattered and was never revisited when the others arrived, which is what +# a hand-maintained list of "things to remember to clean up" does. Adding three +# more `rm` lines would rebuild the same trap for the sixth ledger. +# +# So the rule is the NAME: a per-session ledger is `[.].ids`, and +# everything matching that goes. A new ledger following the convention is +# covered the day it is written, by nobody. One that does not follow it is a +# deliberate exception and has to say so. +# +# Scoped to `.ids` rather than `.*` so a marker that is REWRITTEN on +# compact rather than discarded can still live in this directory without +# being swept away by a glob that was never told about it. +scribe_clear_session_ledgers() { + local dir="$1" sid="$2" + [ -n "$dir" ] && [ -n "$sid" ] || return 0 + rm -f "$dir/$sid"*.ids 2>/dev/null || true + return 0 +} + # --------------------------------------------------------------------------- # WHICH PROJECT IS THIS DIRECTORY'S? (#4085) # diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 5d0da25..2fa739d 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -77,9 +77,11 @@ source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source= # this state are the ones that fire most often, which is to say the ones that # apply most. # -# The session id survives a compaction — the etag marker further down is -# rewritten on `compact` and keyed by session_id, which is only meaningful if -# the id is stable — so the stale ledger is genuinely found again, not orphaned. +# The session id survives a compaction — the ledgers are keyed by it and are +# still found under the same name afterwards, so a stale ledger is genuinely +# reached again rather than orphaned. (This used to point at an etag marker as +# the evidence for that; milestone 394 removed the preload the etag described, +# and `plugin_context.py` records its retirement.) # # CLEARED ON THE SOURCES THAT DESTROY CONTEXT, AND ONLY THOSE: # @@ -93,25 +95,40 @@ source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source= # fork carries the conversation, so if it inherits the id the ledger # is accurate, and if it gets a new one the file is empty anyway. # -# ONLY the rules ledger. The same directory holds .ids / .sync.ids / -# .derive.ids for the note arms. Whether a surfaced NOTE should return after a -# compaction is a different question with a different answer, and leaving those -# alone is a decision rather than an oversight. +# EVERY LEDGER, AND THE NOTE ARMS NEEDED IT MOST (#4101). +# +# This used to clear the two rule ledgers by name and say, in a comment, that +# leaving `.ids` / `.sync.ids` / `.derive.ids` alone was "a decision rather than +# an oversight". Reading the note arms says otherwise, on two counts: +# +# - They are HARD exclusions. `exclude_ids` goes into `semantic_search_notes` +# itself, so a surfaced note is removed from the result set — it is not +# rendered as a reference the way #3750 made a repeated rule. There is no +# weaker form for it to fall back to. +# - They never AGE. #3751 gave the rules ledger a TTL precisely because +# salience decays without a context event; the note channels were left on a +# flat read. +# +# Hard plus permanent plus never cleared means a note surfaced in the first +# minute of a session is unreachable for the rest of it, through any number of +# compactions. That is milestone 386's original defect, alive on the arms that +# fire most often, and nothing about it was decided. +# +# THE LIST WAS THE BUG, so the fix is not a longer list. `scribe_clear_session_ +# ledgers` matches on the naming convention — a per-session ledger is +# `[.].ids` — which covers all five and covers the sixth on the day +# it is written. Best-effort, like every other filesystem touch in these hooks: +# a ledger that cannot be removed costs a repeated exclusion, never a session. +# +# NOT swept: `.unreached`, which records that the instance was unreachable +# rather than what the session holds, and survives on purpose. case "$source" in compact|clear) sid=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || sid="" if [ -n "$sid" ]; then safe_sid=$(printf '%s' "$sid" | tr -c 'A-Za-z0-9._-' '_') - # Best-effort, like every other filesystem touch in these hooks: a ledger - # that cannot be removed costs a repeated exclusion, never a session. - rm -f "${TMPDIR:-/tmp}/scribe-priorart/${safe_sid}.rules.ids" 2>/dev/null || true - # BOTH ledgers, for one reason (#4100). `.opened.ids` records what the - # session read; a compaction is exactly the event that takes it away - # again. Clearing the naming ledger while keeping this one would leave - # the surfacing arms telling a freshly-summarised session "you opened - # it earlier" about a rule that is no longer anywhere in its context — - # a more confident version of the claim this milestone removed. - rm -f "${TMPDIR:-/tmp}/scribe-priorart/${safe_sid}.opened.ids" 2>/dev/null || true + scribe_clear_session_ledgers \ + "${TMPDIR:-/tmp}/scribe-priorart" "$safe_sid" fi ;; esac diff --git a/tests/test_rule_opened_ledger.py b/tests/test_rule_opened_ledger.py index 3c7c765..72e23c8 100644 --- a/tests/test_rule_opened_ledger.py +++ b/tests/test_rule_opened_ledger.py @@ -135,21 +135,9 @@ def test_an_event_with_nothing_usable_records_nothing_and_still_exits_zero(event assert not led.exists() -# ── the two ledgers stay in step ─────────────────────────────────────────── - -def test_a_compaction_clears_both_ledgers(): - """The one that would be worst to get wrong. `.opened.ids` describes a - context the compaction just destroyed, so keeping it while clearing the - naming ledger would have the arms telling a freshly-summarised session - "you opened it earlier" about a rule now nowhere in its context. - """ - sh = (HOOKS / "scribe_session_context.sh").read_text() - block = sh.split("case \"$source\" in")[1].split("esac")[0] - assert "compact|clear)" in block - for led in (".rules.ids", ".opened.ids"): - assert re.search(rf"rm -f .*{re.escape(led)}", block), ( - f"{led} survives a compaction that destroyed what it describes" - ) +# What a compaction clears — including `.opened.ids`, whose claim is the one +# that would be worst to get wrong — is asserted against the running hook in +# tests/test_session_ledger_clear.py, so there is one home for it. def test_the_recorder_is_registered_on_the_get_rule_tool(): diff --git a/tests/test_session_ledger_clear.py b/tests/test_session_ledger_clear.py new file mode 100644 index 0000000..a1d1bf8 --- /dev/null +++ b/tests/test_session_ledger_clear.py @@ -0,0 +1,161 @@ +"""A compaction clears every session ledger, by convention not by list (#4101). + +WHY THIS EXISTS + +Milestone 386 established the claim: a ledger describes what a session HOLDS, +so the events that destroy context must destroy it too, or the records it names +become permanently unreachable mid-session. `scribe_session_context.sh` +implemented that — for the rules ledger, by name. + +Five ledgers live in that directory and two were on the list. The note arms' +three (`.ids`, `.sync.ids`, `.derive.ids`) were left, under a comment asserting +this was "a decision rather than an oversight". It was not a decision, and the +arms left out are the ones where it costs most: + + - their exclusions are HARD — `exclude_ids` is passed into + `semantic_search_notes` itself, so a surfaced note leaves the result set + entirely, with no weaker rendering to fall back to the way #3750 gave a + repeated rule one; + - and they never AGE — #3751's TTL was added to the rules ledger only. + +Hard, permanent and never cleared: a note surfaced in a session's first minute +is unreachable for the rest of it, through any number of compactions. + +WHAT THIS PINS + +The list was the bug, so the fix cannot be a longer list and neither can the +test. Both sides are asserted: + + 1. BEHAVIOUR — the hook is run on a real `compact` event with all five + ledgers on disk, and all five are gone afterwards. Run rather than + grepped, because grepping for the names is the hand-maintained pattern + this step removes. + 2. THE CONVENTION THE BEHAVIOUR RESTS ON — every per-session ledger any hook + builds is named `[.].ids`. That is what makes a sixth ledger + covered on the day it is written, and it is the assumption that would + rot silently, because a ledger named outside it simply never clears and + nothing says so. + +And the negative: `.unreached` is not a ledger of held context but a +record that the instance could not be reached, and a glob that swept it away +would make a hook forget an outage it is meant to report (#2932). +""" +import json +import os +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +HOOKS = Path(__file__).resolve().parents[1] / "plugin" / "hooks" +SESSION_START = HOOKS / "scribe_session_context.sh" + +# Every ledger the hooks write today. Named here so a failure READS as "this +# one survived", but never used to build the hook's own delete list — the +# convention test below is what keeps this roster honest as it grows. +LEDGERS = (".ids", ".rules.ids", ".opened.ids", ".sync.ids", ".derive.ids") + + +def _run_session_start(source: str, tmp: Path) -> Path: + """Run the SessionStart hook for real, with a populated ledger directory.""" + for tool in ("bash", "jq"): + if shutil.which(tool) is None: + pytest.skip(f"hook runtime tool {tool!r} not installed") + + state = tmp / "scribe-priorart" + state.mkdir(parents=True, exist_ok=True) + for suffix in LEDGERS: + (state / f"s1{suffix}").write_text("42\t1789600000\n") + # Not a ledger: an outage marker that must outlive the clear. + (state / "s1.unreached").write_text("1\n") + + env = {"PATH": os.environ["PATH"], "HOME": str(tmp), "TMPDIR": str(tmp)} + out = subprocess.run( + ["bash", str(SESSION_START)], + input=json.dumps({"session_id": "s1", "source": source}), + capture_output=True, text=True, env=env, timeout=60, + ) + assert out.returncode == 0, out.stderr + return state + + +@pytest.mark.parametrize("source", ["compact", "clear"]) +def test_a_context_destroying_source_clears_every_ledger(source, tmp_path): + """The step, stated as behaviour: all five, not the two that were listed.""" + state = _run_session_start(source, tmp_path) + survived = [s for s in LEDGERS if (state / f"s1{s}").exists()] + assert not survived, ( + f"{survived} survived a {source!r} that destroyed what they describe" + ) + + +@pytest.mark.parametrize("source", ["startup", "resume"]) +def test_a_source_that_kept_the_context_keeps_the_ledgers(source, tmp_path): + """The mirror error, and the more expensive one. + + `resume` genuinely restores the context, so the ledger still describes what + the session holds; clearing there would re-surface every record after a + restore that lost nothing. A blanket glob makes over-clearing cheap to + write, which is exactly why this direction needs a test of its own. + """ + state = _run_session_start(source, tmp_path) + for suffix in LEDGERS: + assert (state / f"s1{suffix}").exists(), ( + f"s1{suffix} was cleared on {source!r}, which lost no context" + ) + + +def test_the_outage_marker_is_not_swept_with_them(tmp_path): + """`.unreached` records that the instance was down, not what was surfaced. + + Different lifetime, different question. #2932's whole point is that "we + checked and found nothing" and "we never managed to check" must stay + distinguishable, and a clear that took this file out would quietly answer + the second with the first. + """ + state = _run_session_start("compact", tmp_path) + assert (state / "s1.unreached").exists() + + +def test_every_ledger_any_hook_builds_follows_the_naming_convention(): + """The assumption the glob rests on, asserted where it can actually fail. + + A ledger named outside `[.].ids` does not break loudly — it just + never clears, on the arm whose author had no reason to know a convention + existed. So the convention is checked against the hooks themselves rather + than trusted: every path any hook composes from the session-id stem has to + end in `.ids`, or be named below as a deliberate non-ledger. + """ + # Files built from the safe session id that are NOT per-session ledgers. + NOT_LEDGERS = {".unreached"} + + offenders = [] + for script in sorted(HOOKS.glob("*.sh")): + for suffix in re.findall(r'\$\{safe_sid\}([A-Za-z0-9_.]*)', + script.read_text()): + if suffix in NOT_LEDGERS or suffix.endswith(".ids"): + continue + offenders.append(f"{script.name}: ${{safe_sid}}{suffix}") + + assert not offenders, ( + "these session files clear on no compaction, because the clear matches " + f"on the `.ids` convention and they do not follow it: {offenders}" + ) + + +def test_the_clear_is_derived_and_not_a_list_of_names(): + """The regression that would look like a fix. + + Appending an `rm -f` per ledger passes every behavioural test above while + rebuilding the trap for the sixth one. The property worth keeping is that + the hook names no ledger at all. + """ + sh = SESSION_START.read_text() + block = sh.split('case "$source" in')[1].split("esac")[0] + assert "scribe_clear_session_ledgers" in block + for suffix in LEDGERS: + assert suffix not in block, ( + f"the clear names {suffix} again — a list, not a convention" + ) -- 2.54.0 From 5c64ea0b4fb57c605fb871b766693bdb475ae05e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 16 Sep 2026 21:08:10 -0400 Subject: [PATCH 02/12] feat(retrieval): a repeat on the note arms is a reference, not silence (#4101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3750 settled this for rules: a record the session was told about an hour ago is not a record in front of the reader now, so the second time it is the best answer it is rendered again with a tail saying so. The note and snippet arms never got that fix, and theirs was worse — the ledger went into `semantic_search_notes` as `exclude_ids`, so the repeat left the candidate set entirely. Three things followed: - the second time a note was the best answer the session got SILENCE, indistinguishable from "nothing matched", on the arms that fire most (`auto_inject` alone ran 598 calls in five days); - a compaction made that permanent, since the ledger outlived the context it described — fixed one layer down in c61f730; - and `best_available` was measured against a candidate set the caller had already edited, so the bar could be blamed for a record the caller withheld (#3739, from the side its fix never reached). The ledger is now a RENDERING fact. Every repeat is still ranked, still shown, and carries a `seen` marker; the band is computed over all hits, because letting the ledger move the cutoff would make "you were shown this" change what counts as relevant. The marker is one word and deliberately not the rule arms' phrasing — "before deciding it does not apply" is the voice of a record that binds, and a dev-log borrowing it would claim authority it does not have. Telemetry takes the rule arms' contract (#3752): `results` and `record_surfaced` both take fresh only, the repeat is counted in `suppressed`, so this source's surfaced set still matches its own log row (#3668). That makes a fact readable that could not be stated here before — `result_count == 0` with `suppressed_count > 0` is "everything that matched, the session has already seen", which is a different claim about the bar from "nothing cleared it". On the write path this also splits a variable that carried two claims. `seen` was the ledger plus everything the call had already rendered, and both were treated as reasons to withhold; `in_menu` keeps the same-call exclusion while the ledger becomes a marker. That narrows the `best_available` compromise at its old comment to the pulled-and-already-listed case, and retires the argument that a suppression count here would be partial — nothing is hidden inside the query any more. Deliberately unchanged: the write-path SYNC class still shows once. Its claim is about an edit in progress rather than a record's continuing relevance, and repeating it every write to the same file would be nagging. tests/test_ledger_references_not_silence.py pins both arms — the ledger never reaching the search, the repeat rendered and distinguishable, the telemetry split, the all-repeats call being readable, and the two exceptions (this call's own menu, and the sync class). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/plugin_context.py | 188 +++++++++++---- tests/test_ledger_references_not_silence.py | 240 ++++++++++++++++++++ 2 files changed, 386 insertions(+), 42 deletions(-) create mode 100644 tests/test_ledger_references_not_silence.py diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index b992354..506033a 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -644,7 +644,7 @@ async def _reserve_slot_for_reuse( cfg: dict, *, project_id: int | None, - exclude_ids: set[int], + already: set[int], ) -> list: """Guarantee the reuse-shaped kinds one slot, if one clears threshold (#2246). @@ -671,16 +671,23 @@ async def _reserve_slot_for_reuse( top_k = cfg["top_k"] _t0 = time.perf_counter() _rep: dict = {} + # THE LEDGER IS NOT AN EXCLUSION HERE EITHER (#4101) — but what is already + # in `kept` still is, and the two are different claims. A record sitting in + # this call's own menu must not be shown twice in it; a record shown in an + # EARLIER call is exactly what the slot should be allowed to spend itself + # on, because a snippet that was relevant then and is relevant now is the + # reuse case rather than a duplicate of it. reuse = await semantic_search_notes( user_id, query, limit=1, threshold=cfg["threshold"], project_id=project_id, - exclude_ids=exclude_ids | {int(n.id) for _s, n in kept}, + exclude_ids={int(n.id) for _s, n in kept}, note_type=_REUSE_KINDS, scope="browse", report=_rep, ) + fresh_reuse = [(s, n) for s, n in reuse if int(n.id) not in already] # A real semantic query competing for a menu slot — logged like the scored # arm it displaces. Before this, the hit it PUSHED OUT was in # retrieval_logs and the query that pushed it out was not, so the slot @@ -689,16 +696,17 @@ async def _reserve_slot_for_reuse( record_retrieval( user_id=user_id, source="reuse_slot", query=query, threshold=cfg["threshold"], limit=1, project_id=project_id, - is_task=None, results=reuse, + is_task=None, results=fresh_reuse, best_available=_rep.get("best_available_score"), best_available_id=_rep.get("best_available_id"), searched=bool(_rep.get("searched", True)), + suppressed=len(reuse) - len(fresh_reuse), duration_ms=(time.perf_counter() - _t0) * 1000.0, ) # Verify the kind rather than trusting the query that asked for it, and - # dedup on top of exclude_ids. This slot exists FOR reuse kinds — a slot - # silently spent on something else is worse than no slot, because the line - # is indistinguishable from one that earned its place on score. + # dedup against this call's own menu. This slot exists FOR reuse kinds — a + # slot silently spent on something else is worse than no slot, because the + # line is indistinguishable from one that earned its place on score. kept_ids = {int(n.id) for _s, n in kept} fresh = [ (s, n) for s, n in reuse @@ -726,7 +734,8 @@ async def build_autoinject_hint( The four anti-bloat gates (see the module + milestone-93 design): 1. high-confidence threshold (stricter than pull) — set per-user; 2. margin gate — keep only hits within _AUTOINJECT_BAND of the top score; - 3. session dedup — caller passes already-injected ids as `exclude_ids`; + 3. session marking — caller passes already-injected ids as `exclude_ids` + and they are rendered again with `[seen]`, never withheld (#4101); 4. title-first payload — id + kind + title + score only, never bodies. Disabled, blank-query, or nothing-clears-the-gates all return empty context, so most turns inject nothing. @@ -741,6 +750,24 @@ async def build_autoinject_hint( if not cfg["enabled"] or not q: return empty + # THE LEDGER LEAVES THE SEARCH (#4101). `exclude_ids` used to go into + # `semantic_search_notes` itself, so a record this session had already been + # shown was removed from the candidate set — which is #3750's defect, on the + # arm that fires most. Three things followed from it, none of them intended: + # + # - the second time a note was the best answer, the session got SILENCE, + # indistinguishable from "nothing matched"; + # - a compaction made that permanent, because the ledger outlived the + # context it described (fixed one layer down in this same step); + # - and the score the search reported was measured against a candidate set + # the caller had already edited, so `best_available` could name a bar + # that turned nothing away (#3739). + # + # Now the ledger is a RENDERING fact, not a retrieval one: every repeat is + # still ranked, still shown, and carries a marker saying it was surfaced + # before. A note line is a title and a score — the repeat costs about as + # much as the comma in this sentence — so there is nothing here to save. + already = {int(i) for i in (exclude_ids or [])} t0 = time.perf_counter() _rep_ai: dict = {} hits = await semantic_search_notes( @@ -748,7 +775,6 @@ async def build_autoinject_hint( limit=cfg["top_k"], threshold=cfg["threshold"], project_id=(project_id or None), - exclude_ids=set(exclude_ids or []), # Injection is the one retrieval nobody asked for, so it takes the BROWSE # scope: never a record shared one-to-one with the operator. What can # still appear is a collaborator's note inside a shared project — legible @@ -756,24 +782,37 @@ async def build_autoinject_hint( scope="browse", report=_rep_ai, ) + # `results=fresh` and `suppressed` together, on the rule arms' contract + # (#3752): a rendered repeat is not a new surfacing, so it stays out of the + # row's result set and is COUNTED instead. That keeps this source's surfaced + # set identical to its own log row (#3668) while making a zero-result call + # readable — `result_count == 0` with `suppressed_count > 0` is "everything + # that matched, this session has already seen", which is a different fact + # about the bar from "nothing cleared it" and used to be unreportable here. + fresh = [(s, n) for s, n in hits if int(n.id) not in already] record_retrieval( user_id=user_id, source="auto_inject", query=q, threshold=cfg["threshold"], limit=cfg["top_k"], - project_id=(project_id or None), is_task=None, results=hits, + project_id=(project_id or None), is_task=None, results=fresh, best_available=_rep_ai.get("best_available_score"), best_available_id=_rep_ai.get("best_available_id"), searched=bool(_rep_ai.get("searched", True)), + suppressed=len(hits) - len(fresh), duration_ms=(time.perf_counter() - t0) * 1000.0, ) if not hits: return empty - # Margin gate: keep only hits close to the strongest one. + # Margin gate: keep only hits close to the strongest one. Computed over ALL + # hits, repeats included — the band measures distance from the top SCORE, + # and letting the ledger move that cutoff would make "you were shown this" + # change what counts as relevant, which is the axis independence the rule + # band keeps for the same reason. top_score = hits[0][0] kept = [(s, n) for s, n in hits if s >= top_score - _AUTOINJECT_BAND] kept = await _reserve_slot_for_reuse( user_id, q, kept, cfg, project_id=(project_id or None), - exclude_ids=set(exclude_ids or []), + already=already, ) # A collaborator's note can reach this menu via a shared project, and the @@ -786,10 +825,15 @@ async def build_autoinject_hint( # "records", not "notes" — the menu can hold snippets, processes and tasks # too, and the kind marker on each line is only legible if the header doesn't # already claim they're all one thing. + # "injected once per session" was true and is not any more (#4101): a repeat + # is shown again with a marker rather than withheld, so the header must stop + # promising the old contract. It now says what the marker means instead, + # once, rather than each repeated line having to explain itself. lines = [ "> Possibly relevant from your Scribe records — open any in full with " "`get_note(id)`, or `get_snippet` / `get_process` for those kinds " - "(titles only; injected once per session):", + "(titles only; a line marked `seen` was surfaced earlier this session " + "and may no longer be in context):", ] # A superseded record is DEMOTED, not removed (#278) — so one can still reach # this menu, and when it does the reader has to be told. An agent handed @@ -802,6 +846,13 @@ async def build_autoinject_hint( note_ids.append(int(note.id)) title = (note.title or "(untitled)").replace("\n", " ").strip() line = f"> - #{note.id} [{_record_kind(note)}] \"{title}\" ({score:.2f})" + # ONE WORD, NOT A SENTENCE, and deliberately not the rule arms' phrasing. + # A rule line says "before deciding it does not apply", which is the + # voice of a record that BINDS; a note binds nothing, and borrowing that + # tone would tell the reader a dev-log has authority it does not have. + # The header carries the meaning, so the line carries only the flag. + if int(note.id) in already: + line += " [seen]" if int(note.id) in stale: line += " — SUPERSEDED, a later record covers this; check that first" if note.user_id != user_id: @@ -813,7 +864,17 @@ async def build_autoinject_hint( # menu the agent actually saw. retrieval_logs already holds the full # candidate set for threshold tuning; conflating the two would make # "surfaced" mean two different things depending on the surface (#2085). - record_surfaced(user_id=user_id, note_ids=note_ids, source="auto_inject") + # + # FRESH ONLY, which is the same cut the log row above takes (#4101). A + # repeat is rendered but is not a new surfacing, and counting it again would + # make this table disagree with `retrieval_logs` about the same call — + # #3668's identity, which is the cheapest true statement available about + # this pair of tables and is not worth a marker's convenience. + record_surfaced( + user_id=user_id, + note_ids=[i for i in note_ids if i not in already], + source="auto_inject", + ) return {"context": "\n".join(lines), "note_ids": note_ids, "config": cfg} @@ -1468,12 +1529,16 @@ async def build_write_path_hint( #2707): the record gets corrected in the session that has the context, at the moment of change. Nearby and semantic hits stay the REUSE menu. - The two classes dedup on SEPARATE channels — `exclude_ids` (reuse) and - `exclude_sync_ids` (sync) — because they answer different questions: a - title shown as "consider reusing this" twenty turns ago must not silence - "you are editing the recorded file right now" (#2708). + The two classes track the session on SEPARATE channels — `exclude_ids` + (reuse) and `exclude_sync_ids` (sync) — because they answer different + questions: a title shown as "consider reusing this" twenty turns ago must + not silence "you are editing the recorded file right now" (#2708). The two + channels also now ACT differently, which is #4101: a reuse repeat is + rendered again with a `seen` marker, while the sync class still shows + once, because its claim is about an edit in progress rather than about a + record's continuing relevance and repeating it would be nagging. - Carries auto-inject's anti-bloat gates (margin, session dedup, + Carries auto-inject's anti-bloat gates (margin, session marking, titles-never-bodies) plus the shared top-k cap across ALL arms — so a file with a lot of recorded history can't turn one edit into a wall of text. Two gates are its OWN, because code is not prose: a stricter similarity @@ -1592,16 +1657,28 @@ async def build_write_path_hint( query = concept_query(query) or query if remaining > 0 and query: t0 = time.perf_counter() - # Pulled-and-seen ids stay in the query (as evidence) but never in - # the menu — the dedup contract holds, the resemblance still lands. - pulled_seen = seen & set(pulled) + # `seen` CARRIES TWO DIFFERENT CLAIMS, and only one of them is a reason + # to withhold (#4101). It was built as the ledger plus everything this + # call has already rendered, and the arm treated both the same way: + # + # - already in THIS menu (the sync block, the place arm) — listing it + # twice in one hint is noise with no reader it could help; + # - on the LEDGER from an earlier call — a different claim entirely, + # and the one #3750 says must be rendered rather than dropped. + # + # Splitting them is the whole change here. `in_menu` keeps its + # exclusion; the ledger becomes a marker on the line. + in_menu = seen - excluded + # Pulled-and-already-listed ids stay in the query (as evidence for + # `resembles`) but never in the menu, so the limit has to cover them. + pulled_in_menu = in_menu & set(pulled) _rep_wp: dict = {} hits = await semantic_search_notes( user_id, query, - limit=remaining + len(pulled_seen), + limit=remaining + len(pulled_in_menu), threshold=cfg["threshold"], project_id=scope_project, - exclude_ids=seen - pulled_seen, + exclude_ids=in_menu - set(pulled), # Snippets AND recorded experience (#2246). This arm was # snippets-only, which is auto-inject's mistake inverted: an issue # saying "we tried this and it deadlocked", or a dev-log recording @@ -1625,41 +1702,50 @@ async def build_write_path_hint( int(note.id): float(score) for score, note in hits if int(note.id) in pulled } - shown = [(s, n) for s, n in hits if int(n.id) not in seen] + shown = [(s, n) for s, n in hits if int(n.id) not in in_menu] # WHAT THIS ARM WITHHELD AFTER THE SEARCH ANSWERED, and the reason # `best_available_score` cannot always be reported here (#3739 again, # from the side its fix did not reach). # # This arm is the one note arm that filters TWICE. `exclude_ids` takes - # `seen - pulled_seen` into the search, but the pulled-and-seen ids stay - # in the query deliberately — `resembles` above needs them — and are - # dropped in the line above instead. So the score the search reported is - # PRE that drop while the row's `result_count` is POST it, and a record - # the session had already been shown could be logged as something the - # BAR turned away. Live proof on the first read after #3739 shipped: - # write_path's near-miss max was 0.822 while the lowest score it ever - # RETURNED was 0.6857 — a "rejection" that beat every acceptance. - # - # The suppression column cannot rescue it the way it does for the rule - # arms: this arm's count would be PARTIAL, covering only the drops made - # here and not the ones `exclude_ids` made inside the search, and a - # partial number under a name that reads as complete is the substitution - # this whole milestone exists to stop. + # the already-listed ids into the search, but the PULLED ones among them + # stay in the query deliberately — `resembles` above needs them — and + # are dropped in the line above instead. So the score the search + # reported is PRE that drop while the row's `result_count` is POST it, + # and a record already listed in this same menu could be logged as + # something the BAR turned away. Live proof on the first read after + # #3739 shipped: write_path's near-miss max was 0.822 while the lowest + # score it ever RETURNED was 0.6857 — a "rejection" that beat every + # acceptance. # # So the honest answer is null — "not measured on this call" — whenever # this filter removed anything, because then the bar is not the only # thing that turned something away and the reported score may belong to # a record we withheld ourselves. Calls where nothing was dropped keep # reporting it, which is most of them. + # + # THE LEDGER IS NO LONGER PART OF THIS (#4101), and that is why the + # suppression column can now be filled in where it could not before. + # The old objection was that a count here would be PARTIAL — covering + # the drops made in this function but not the ones `exclude_ids` made + # inside the search — and a partial number under a name that reads as + # complete is the substitution this milestone exists to stop. That was + # right while the ledger was one of the things `exclude_ids` carried. + # It no longer is: every ledger repeat comes back from the search and is + # rendered, so `suppressed` counts all of them and none are hidden + # inside the query. What `exclude_ids` still removes is this call's own + # menu, which is not suppression at all — those records ARE being shown, + # one block further up. withheld_here = len(hits) - len(shown) hits = shown[:remaining] + fresh = [(s, n) for s, n in hits if int(n.id) not in excluded] record_retrieval( user_id=user_id, source="write_path", query=query, threshold=cfg["threshold"], limit=remaining, # is_task is None, not False: this arm now returns issues too, and # recording it as a notes-only retrieval would misdescribe the # candidate set the threshold is being tuned against. - project_id=scope_project, is_task=None, results=hits, + project_id=scope_project, is_task=None, results=fresh, best_available=( None if withheld_here else _rep_wp.get("best_available_score") ), @@ -1670,6 +1756,7 @@ async def build_write_path_hint( None if withheld_here else _rep_wp.get("best_available_id") ), searched=bool(_rep_wp.get("searched", True)), + suppressed=len(hits) - len(fresh), duration_ms=(time.perf_counter() - t0) * 1000.0, ) if hits: @@ -1683,9 +1770,19 @@ async def build_write_path_hint( # and an unlabelled line would be read as "here is code to # reuse", which is the opposite of what it says. kind = _record_kind(note) - scored.append(( + marker = ( f"similar {score:.2f}" if kind == "snippet" - else f"similar {score:.2f} · {kind}", + else f"similar {score:.2f} · {kind}" + ) + # The repeat marker rides the same dotted list as the kind, so a + # reference costs four characters and needs no second line + # (#4101). Same word as the auto-inject menu deliberately: a + # reader meeting `seen` on two different surfaces should not + # have to work out whether they mean the same thing. + if int(note.id) in excluded: + marker += " · seen" + scored.append(( + marker, { "id": int(note.id), "title": note.title, "user_id": note.user_id, # Carried so the line can disclose a cross-language hit @@ -1811,7 +1908,8 @@ async def build_write_path_hint( "`get_snippet(id)` for a snippet, `get_task(id)` for an issue, " "`get_note(id)` otherwise. Reuse a snippet rather than writing a fresh " "one-off; read an issue before repeating what it records " - "(titles only; shown once per session):" + "(titles only; a line marked `seen` was surfaced earlier this " + "session and may no longer be in context):" ) # Say what a language tag MEANS, and only when one is actually on the menu. # Without this the reader has to infer why "· python" is attached to a hit on @@ -1848,6 +1946,12 @@ async def build_write_path_hint( if sync_note_ids: by_arm["write_path_sync"] = list(sync_note_ids) for marker, item in menu: + # A rendered repeat is not a new surfacing (#4101) — same cut the log + # row takes, so this table and `retrieval_logs` keep agreeing about the + # same call (#3668). The place arm never reaches here with one: a nearby + # snippet on the ledger is skipped before it is ever placed. + if int(item["id"]) in excluded: + continue arm = "write_path_place" if marker == "nearby" else "write_path_semantic" by_arm.setdefault(arm, []).append(int(item["id"])) for arm, ids in by_arm.items(): diff --git a/tests/test_ledger_references_not_silence.py b/tests/test_ledger_references_not_silence.py new file mode 100644 index 0000000..798755d --- /dev/null +++ b/tests/test_ledger_references_not_silence.py @@ -0,0 +1,240 @@ +"""A repeat on a note arm is referenced, not withheld (#4101). + +WHY THIS EXISTS + +#3750 settled the argument for rules: a record the session was told about an +hour ago is not a record in front of the reader now, so the second time it is +the best answer the session gets the line again with a tail saying so — never +silence, which is indistinguishable from "nothing matched". + +The note and snippet arms never got that fix. Worse, their ledger went into +`semantic_search_notes` as `exclude_ids`, so the repeat left the candidate set +entirely, which had three consequences: + + - the session got silence the second time, on the arms that fire most; + - a compaction made it permanent, because the ledger outlived the context it + described (the same step fixes that one layer down); + - and `best_available` was measured against a candidate set the caller had + already edited, so the bar could be blamed for a record the caller withheld + (#3739, from the side its fix never reached). + +WHAT THIS PINS + + 1. The ledger does not reach the search. Asserted on the call's kwargs, + because this is the difference between a reference and silence and every + behavioural assertion below rests on it. + 2. The repeat is rendered, and marked. The wording is NOT the rule arms' — + "before deciding it does not apply" is the voice of a record that binds, + and a dev-log borrowing it would claim authority it does not have — so + what is pinned is that the line appears and is distinguishable, not its + prose. + 3. The telemetry splits the two (#3752 / #3668): the row's result set and the + surfaced table both take FRESH only, and the repeat is COUNTED in + `suppressed` instead. This is what makes a zero-result call readable — + `result_count == 0` with `suppressed_count > 0` says "everything that + matched, the session has already seen", which was unreportable here. + 4. The deliberate exception: the write-path SYNC class still shows once. Its + claim is about an edit in progress, not about a record's continuing + relevance, and repeating it would be nagging rather than reminding. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from tests.helpers import fake_note + +REAL_CODE = '''def debounce(fn, wait=0.25): + """Rate-limit a callback so it fires once after the last call.""" + timer = None + + def wrapped(*a, **kw): + nonlocal timer + if timer: + timer.cancel() + timer = threading.Timer(wait, fn, a, kw) + timer.start() + + return wrapped +''' + + +def _wp_cfg(**over): + base = {"enabled": True, "threshold": 0.68, "top_k": 3, "rule_threshold": 0.72} + base.update(over) + return base + + +def _snippet_item(nid, title, user_id=1): + return {"id": nid, "title": title, "user_id": user_id, "note_type": "snippet"} + + +# ── auto-inject ──────────────────────────────────────────────────────────── + +async def _autoinject(hits, exclude_ids, *, rec=None, surf=None): + from scribe.services import plugin_context as pc + search = AsyncMock(return_value=hits) + with patch.object(pc, "get_autoinject_config", AsyncMock( + return_value={"enabled": True, "threshold": 0.55, "top_k": 3})), \ + patch.object(pc, "semantic_search_notes", search), \ + patch.object(pc, "record_retrieval", rec or MagicMock()), \ + patch.object(pc, "record_surfaced", surf or MagicMock()): + out = await pc.build_autoinject_hint( + 1, "postgres pool", project_id=2, exclude_ids=exclude_ids) + return out, search + + +@pytest.mark.asyncio +async def test_the_ledger_never_reaches_the_search(): + """The load-bearing one. A ledger inside the query IS the silence.""" + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1))] + _out, search = await _autoinject(hits, [11]) + # The menu arm runs first; the reuse slot's own call may legitimately + # exclude this call's menu, which is a different claim (see below). + menu_call = search.call_args_list[0] + assert not menu_call.kwargs.get("exclude_ids"), ( + "the session ledger was passed into the search, so a repeat is " + "withheld rather than referenced" + ) + + +@pytest.mark.asyncio +async def test_a_repeat_is_shown_again_and_marked(): + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1)), + (0.78, fake_note(id=22, title="run_maintenance thresholds", user_id=1))] + out, _ = await _autoinject(hits, [11]) + assert "#11" in out["context"] and "#22" in out["context"] + # Distinguishable, without pinning the word's neighbours in the sentence. + line_11 = next(ln for ln in out["context"].splitlines() if "#11" in ln) + line_22 = next(ln for ln in out["context"].splitlines() if "#22" in ln) + assert "seen" in line_11 and "seen" not in line_22 + + +@pytest.mark.asyncio +async def test_the_repeat_is_counted_not_reported_as_a_result(): + rec = MagicMock() + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1)), + (0.78, fake_note(id=22, title="run_maintenance thresholds", user_id=1))] + await _autoinject(hits, [11], rec=rec) + row = next(c.kwargs for c in rec.call_args_list + if c.kwargs["source"] == "auto_inject") + assert [int(n.id) for _s, n in row["results"]] == [22] + assert row["suppressed"] == 1 + + +@pytest.mark.asyncio +async def test_a_call_where_everything_was_already_seen_is_readable(): + """The fact that could not be expressed before. + + Previously this call logged zero results with no suppression count, so it + was indistinguishable from a bar nothing cleared — and the operator tuning + that bar would have been reading the wrong number. + """ + rec = MagicMock() + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1))] + out, _ = await _autoinject(hits, [11], rec=rec) + row = next(c.kwargs for c in rec.call_args_list + if c.kwargs["source"] == "auto_inject") + assert row["results"] == [] and row["suppressed"] == 1 + # And the session still gets the line, which is the whole point. + assert "#11" in out["context"] + + +@pytest.mark.asyncio +async def test_a_repeat_is_not_recorded_as_a_fresh_surfacing(): + """#3668's identity: this table and the log row describe the same call.""" + surf = MagicMock() + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1)), + (0.78, fake_note(id=22, title="run_maintenance thresholds", user_id=1))] + await _autoinject(hits, [11], surf=surf) + rows = [c.kwargs for c in surf.call_args_list + if c.kwargs.get("source") == "auto_inject"] + assert rows and rows[0]["note_ids"] == [22] + + +@pytest.mark.asyncio +async def test_the_header_no_longer_promises_once_per_session(): + """A contract stated in the prose is a contract, and this one changed. + + Cheap to forget and invisible when wrong: the menu would carry a marker + the header had never explained, and a reader would have to guess. + """ + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1))] + out, _ = await _autoinject(hits, []) + header = out["context"].splitlines()[0] + assert "once per session" not in header + assert "seen" in header + + +# ── the write path ───────────────────────────────────────────────────────── + +async def _write_path(hits, exclude_ids, *, here=(), rec=None, sync_exclude=()): + from scribe.services import plugin_context as pc + search = AsyncMock(return_value=hits) + listing = AsyncMock(side_effect=[(list(here), len(here)), ([], 0)]) + with patch.object(pc, "get_writepath_config", AsyncMock(return_value=_wp_cfg())), \ + patch.object(pc.snippets_svc, "list_snippets", listing), \ + patch.object(pc, "semantic_search_notes", search), \ + patch.object(pc, "record_retrieval", rec or MagicMock()), \ + patch.object(pc, "record_surfaced", MagicMock()): + out = await pc.build_write_path_hint( + 1, "src/x.py", code=REAL_CODE, + exclude_ids=list(exclude_ids), + exclude_sync_ids=list(sync_exclude), + ) + return out, search + + +@pytest.mark.asyncio +async def test_the_write_path_ledger_does_not_reach_its_search_either(): + hits = [(0.83, fake_note(id=11, title="debounce helper", user_id=1))] + _out, search = await _write_path(hits, [11]) + assert 11 not in (search.call_args.kwargs.get("exclude_ids") or set()) + + +@pytest.mark.asyncio +async def test_a_write_path_repeat_is_rendered_with_a_marker(): + hits = [(0.83, fake_note(id=11, title="debounce helper", user_id=1))] + out, _ = await _write_path(hits, [11]) + assert "#11" in out["context"] + assert "seen" in next(ln for ln in out["context"].splitlines() if "#11" in ln) + + +@pytest.mark.asyncio +async def test_the_write_path_counts_its_repeat(): + rec = MagicMock() + hits = [(0.83, fake_note(id=11, title="debounce helper", user_id=1)), + (0.80, fake_note(id=22, title="throttle helper", user_id=1))] + await _write_path(hits, [11], rec=rec) + row = next(c.kwargs for c in rec.call_args_list + if c.kwargs["source"] == "write_path") + assert [int(n.id) for _s, n in row["results"]] == [22] + assert row["suppressed"] == 1 + + +@pytest.mark.asyncio +async def test_this_calls_own_menu_is_still_excluded_from_its_search(): + """The claim that stayed an exclusion, and must not be lost with the other. + + A snippet already listed by PLACE in this same hint has nothing to gain + from a second line in it. That is same-call duplication, not a repeat + across calls, and the two were the same variable until this step. + """ + hits = [] + _out, search = await _write_path( + hits, [], here=[_snippet_item(7, "records this file")]) + assert 7 in (search.call_args.kwargs.get("exclude_ids") or set()) + + +@pytest.mark.asyncio +async def test_the_sync_class_still_shows_only_once(): + """The deliberate exception, pinned so it reads as a decision. + + The sync nudge says "you are editing the file this record describes, so + updating it is part of the edit". Repeated every write to the same file it + is nagging, and unlike a reuse suggestion it is not a claim whose relevance + can return — it either got acted on or it did not. + """ + out, _ = await _write_path( + [], [], here=[_snippet_item(7, "records this file")], sync_exclude=[7]) + assert out["sync_note_ids"] == [] + assert "#7" not in out["context"] -- 2.54.0 From f1e63d207f6915a33214f59adc12608fda30e7af Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 16 Sep 2026 21:11:43 -0400 Subject: [PATCH 03/12] =?UTF-8?q?fix(plugin):=20the=20sweep=20missed=20the?= =?UTF-8?q?=20arm=20that=20fires=20most=20=E2=80=94=20two=20ledger=20direc?= =?UTF-8?q?tories=20(#4101)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught two things in c61f730, and the second is the one that mattered. 1. `scribe_autoinject.sh` keeps its note ledger in `${TMPDIR}/scribe-autoinject`, not `scribe-priorart`. Every ledger NAMED in the hooks was in the one directory the sweep visited, so it read as complete — and the arm that fires most (598 calls in five days) was the only one still carrying the bug. The clear ran, found nothing to remove, and exited 0. `SCRIBE_LEDGER_DIRS` in `scribe_defs.sh` is now the roster, and the guard reads that string rather than a copy of it, so a test can no longer agree with itself forever. 2. The convention guard over-reached: it flagged `.snap` and `.blocked`, which live in `scribe-afterwrite` and `scribe-reportcheck` and can never be touched by the sweep. It is now two tests keyed on the directory, because the two assumptions fail differently — one lets a ledger sit where nothing sweeps, the other lets it sit in the right place under a name the sweep does not match. `test_only_the_rule_ledger_is_cleared_and_the_note_ledgers_are_left` also went red, correctly: the note arms' exemption was a real decision, recorded in a test, and my commit message said nothing about it had been decided. That was wrong and the test was right to stop me. The decision is reversed rather than ignored, and the reasoning is that #4101 removed its premise: it rested on a note repeat being WITHHELD, so clearing the ledger meant re-injecting whole menu lines the session already had. Now a repeat is rendered with a `seen` marker, so the ledger only decides whether that marker is true — and across a compaction it is false, telling a freshly-summarised session it has already seen a record that is nowhere in its context. The test keeps its name and records the reversal with the reason, rather than being deleted. Verified by running the hook directly: all six ledgers across both directories gone on `compact`, `.unreached` left standing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_defs.sh | 24 +++- plugin/hooks/scribe_session_context.sh | 8 +- tests/test_session_context_ledger.py | 36 ++++-- tests/test_session_ledger_clear.py | 145 ++++++++++++++++++------- 5 files changed, 156 insertions(+), 59 deletions(-) diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index d2b1158..5600365 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe for Claude Code: connects the scribe MCP server, adds the hooks that deliver live project state and relevant records at the right moment, ships the shared client-neutral Scribe skills (using-scribe, writing-plans, reporting-back, systematic-debugging, verification, brainstorming, reusing-code, shape-accounting), and syncs your saved Scribe Processes as skills (/scribe:sync).", - "version": "2026.09.17.0100", + "version": "2026.09.17.0111", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 542824c..76e6370 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -329,12 +329,26 @@ scribe_held_query() { # deliberate exception and has to say so. # # Scoped to `.ids` rather than `.*` so a marker that is REWRITTEN on -# compact rather than discarded can still live in this directory without -# being swept away by a glob that was never told about it. +# compact rather than discarded can still live in these directories without +# being swept away by a glob that was never told about it. `.unreached` +# is the live example: it records that the instance could not be reached, not +# what the session holds, and #2932 needs it to outlive a compaction. +# +# TWO DIRECTORIES, WHICH IS ITS OWN LESSON. The first cut of this swept only +# `scribe-priorart` — every ledger named in the hooks was there, so the list +# looked complete. `scribe_autoinject.sh` keeps its note ledger in +# `scribe-autoinject`, so the arm that fires most (598 calls in five days) was +# the one the clear could not reach, and the fix read as finished. Hence the +# roster here rather than a path at the call site: one place to add to, and +# the tests read THIS string rather than a copy of it. +SCRIBE_LEDGER_DIRS="scribe-priorart scribe-autoinject" + scribe_clear_session_ledgers() { - local dir="$1" sid="$2" - [ -n "$dir" ] && [ -n "$sid" ] || return 0 - rm -f "$dir/$sid"*.ids 2>/dev/null || true + local sid="$1" dir + [ -n "$sid" ] || return 0 + for dir in $SCRIBE_LEDGER_DIRS; do + rm -f "${TMPDIR:-/tmp}/$dir/$sid"*.ids 2>/dev/null || true + done return 0 } diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 2fa739d..b0dacaa 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -121,14 +121,16 @@ source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source= # a ledger that cannot be removed costs a repeated exclusion, never a session. # # NOT swept: `.unreached`, which records that the instance was unreachable -# rather than what the session holds, and survives on purpose. +# rather than what the session holds, and survives on purpose. The directories +# swept are `scribe_defs.sh`'s `SCRIBE_LEDGER_DIRS` — plural, because the +# auto-inject arm keeps its ledger somewhere else and a single-directory sweep +# missed exactly the arm that fires most. case "$source" in compact|clear) sid=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || sid="" if [ -n "$sid" ]; then safe_sid=$(printf '%s' "$sid" | tr -c 'A-Za-z0-9._-' '_') - scribe_clear_session_ledgers \ - "${TMPDIR:-/tmp}/scribe-priorart" "$safe_sid" + scribe_clear_session_ledgers "$safe_sid" fi ;; esac diff --git a/tests/test_session_context_ledger.py b/tests/test_session_context_ledger.py index 5546d51..b0e28b1 100644 --- a/tests/test_session_context_ledger.py +++ b/tests/test_session_context_ledger.py @@ -108,14 +108,29 @@ def test_the_rules_ledger_survives_exactly_when_the_context_does(tmp_path): ) -def test_only_the_rule_ledger_is_cleared_and_the_note_ledgers_are_left(tmp_path): - """Scope, asserted rather than described. +def test_the_note_ledgers_are_cleared_too_now_that_a_repeat_is_shown(tmp_path): + """The decision this test used to pin, reversed — and why it could be. - The same directory holds `.ids`, `.sync.ids` and `.derive.ids` for the note - arms. Whether a surfaced NOTE should come back after a compaction is a - different question with a different answer, and it is not being answered. - A `rm` glob over `.*` would pass every assertion in the test above - while silently deciding it. + It read: the note arms were deliberately left out of #3749, whether a + surfaced NOTE should come back after a compaction is a different question, + and a glob over `.*` would decide it by accident. That was right when + it was written, and the reason was the note arms' behaviour rather than the + note arms' nature: a note repeat was WITHHELD, so clearing their ledger + meant re-injecting whole menu lines the session had already been given. + Against that, leaving them was the cheaper mistake. + + #4101 removed the premise. A note repeat is now rendered again with a + `seen` marker instead of dropped from the search, so the ledger no longer + decides whether a record is reachable — only whether its line says the + session has met it before. Across a compaction that marker becomes a false + statement: it tells a freshly-summarised session it has already seen a + record that is nowhere in its context. Keeping the ledger buys nothing and + asserts something untrue. + + Kept as a named test rather than deleted, because the roster it guards + moved to tests/test_session_ledger_clear.py and what is worth keeping HERE + is the record that the answer changed, with the reason attached — a + deleted test takes the old decision's reasoning with it. """ env = _env(tmp_path) sid = "sess-scope" @@ -127,10 +142,9 @@ def test_only_the_rule_ledger_is_cleared_and_the_note_ledgers_are_left(tmp_path) _fire("compact", sid, env) assert not rules.exists(), "the rule ledger should have been cleared" - assert notes.exists() and sync.exists() and derive.exists(), ( - "a note ledger was cleared too. The note arms were deliberately left " - "out of #3749 — clearing them is a decision about a different surface, " - "and a glob that takes them along makes it by accident." + assert not (notes.exists() or sync.exists() or derive.exists()), ( + "a note ledger outlived the context it describes, so its records are " + "marked `seen` to a session that can no longer see them" ) diff --git a/tests/test_session_ledger_clear.py b/tests/test_session_ledger_clear.py index a1d1bf8..372c77d 100644 --- a/tests/test_session_ledger_clear.py +++ b/tests/test_session_ledger_clear.py @@ -51,25 +51,43 @@ import pytest HOOKS = Path(__file__).resolve().parents[1] / "plugin" / "hooks" SESSION_START = HOOKS / "scribe_session_context.sh" +DEFS = HOOKS / "scribe_defs.sh" -# Every ledger the hooks write today. Named here so a failure READS as "this -# one survived", but never used to build the hook's own delete list — the -# convention test below is what keeps this roster honest as it grows. -LEDGERS = (".ids", ".rules.ids", ".opened.ids", ".sync.ids", ".derive.ids") +# Every ledger the hooks write today, and where. Named here so a failure READS +# as "this one survived", but never used to build the hook's own delete list — +# the convention tests below are what keep this roster honest as it grows. +LEDGERS = { + "scribe-priorart": (".ids", ".rules.ids", ".opened.ids", ".sync.ids", + ".derive.ids"), + "scribe-autoinject": (".ids",), +} + + +def _swept_dirs() -> set[str]: + """The directories the SHIPPED hook sweeps, read from the hook itself. + + Read rather than restated: a test carrying its own copy of the roster would + agree with itself forever, which is precisely the failure that let the + auto-inject ledger sit in a directory nothing cleared. + """ + line = re.search(r'SCRIBE_LEDGER_DIRS="([^"]*)"', DEFS.read_text()) + assert line, "SCRIBE_LEDGER_DIRS is gone; the clear has no roster" + return set(line.group(1).split()) def _run_session_start(source: str, tmp: Path) -> Path: - """Run the SessionStart hook for real, with a populated ledger directory.""" + """Run the SessionStart hook for real, with the ledger directories filled.""" for tool in ("bash", "jq"): if shutil.which(tool) is None: pytest.skip(f"hook runtime tool {tool!r} not installed") - state = tmp / "scribe-priorart" - state.mkdir(parents=True, exist_ok=True) - for suffix in LEDGERS: - (state / f"s1{suffix}").write_text("42\t1789600000\n") + for dirname, suffixes in LEDGERS.items(): + state = tmp / dirname + state.mkdir(parents=True, exist_ok=True) + for suffix in suffixes: + (state / f"s1{suffix}").write_text("42\t1789600000\n") # Not a ledger: an outage marker that must outlive the clear. - (state / "s1.unreached").write_text("1\n") + (tmp / "scribe-priorart" / "s1.unreached").write_text("1\n") env = {"PATH": os.environ["PATH"], "HOME": str(tmp), "TMPDIR": str(tmp)} out = subprocess.run( @@ -78,14 +96,15 @@ def _run_session_start(source: str, tmp: Path) -> Path: capture_output=True, text=True, env=env, timeout=60, ) assert out.returncode == 0, out.stderr - return state + return tmp @pytest.mark.parametrize("source", ["compact", "clear"]) def test_a_context_destroying_source_clears_every_ledger(source, tmp_path): - """The step, stated as behaviour: all five, not the two that were listed.""" - state = _run_session_start(source, tmp_path) - survived = [s for s in LEDGERS if (state / f"s1{s}").exists()] + """The step, stated as behaviour: all of them, in both directories.""" + root = _run_session_start(source, tmp_path) + survived = [f"{d}/s1{s}" for d, suffixes in LEDGERS.items() + for s in suffixes if (root / d / f"s1{s}").exists()] assert not survived, ( f"{survived} survived a {source!r} that destroyed what they describe" ) @@ -100,11 +119,13 @@ def test_a_source_that_kept_the_context_keeps_the_ledgers(source, tmp_path): restore that lost nothing. A blanket glob makes over-clearing cheap to write, which is exactly why this direction needs a test of its own. """ - state = _run_session_start(source, tmp_path) - for suffix in LEDGERS: - assert (state / f"s1{suffix}").exists(), ( - f"s1{suffix} was cleared on {source!r}, which lost no context" - ) + root = _run_session_start(source, tmp_path) + for dirname, suffixes in LEDGERS.items(): + for suffix in suffixes: + assert (root / dirname / f"s1{suffix}").exists(), ( + f"{dirname}/s1{suffix} was cleared on {source!r}, which lost " + "no context" + ) def test_the_outage_marker_is_not_swept_with_them(tmp_path): @@ -115,33 +136,79 @@ def test_the_outage_marker_is_not_swept_with_them(tmp_path): distinguishable, and a clear that took this file out would quietly answer the second with the first. """ - state = _run_session_start("compact", tmp_path) - assert (state / "s1.unreached").exists() + root = _run_session_start("compact", tmp_path) + assert (root / "scribe-priorart" / "s1.unreached").exists() -def test_every_ledger_any_hook_builds_follows_the_naming_convention(): - """The assumption the glob rests on, asserted where it can actually fail. +# ── the two assumptions the sweep rests on ───────────────────────────────── +# +# Both are checked against the hooks rather than trusted, because neither fails +# loudly: a ledger outside them simply never clears, on the arm whose author had +# no reason to know a convention existed. - A ledger named outside `[.].ids` does not break loudly — it just - never clears, on the arm whose author had no reason to know a convention - existed. So the convention is checked against the hooks themselves rather - than trusted: every path any hook composes from the session-id stem has to - end in `.ids`, or be named below as a deliberate non-ledger. +def _dir_vars(text: str) -> dict[str, str]: + """`var="${TMPDIR:-/tmp}/"` → {var: name}, per script.""" + return dict(re.findall( + r'(\w+)="\$\{TMPDIR:-/tmp\}/([A-Za-z0-9_-]+)"', text)) + + +def _session_paths(text: str): + """Every `$/${safe_sid}` a script composes.""" + return re.findall(r'\$(\w+)"?/\$\{safe_sid\}([A-Za-z0-9_.]*)', text) + + +def test_every_directory_holding_a_ledger_is_one_the_clear_visits(): + """The failure that shipped once already, in the same step that fixed it. + + The first cut swept `scribe-priorart` alone. Every ledger NAMED in the + hooks was there, so it read as complete — and `scribe_autoinject.sh` keeps + its note ledger in `scribe-autoinject`, which meant the arm that fires most + was the only one still carrying the bug. Nothing said so: the clear ran, + found nothing to remove, and exited 0. """ - # Files built from the safe session id that are NOT per-session ledgers. - NOT_LEDGERS = {".unreached"} - + swept = _swept_dirs() offenders = [] for script in sorted(HOOKS.glob("*.sh")): - for suffix in re.findall(r'\$\{safe_sid\}([A-Za-z0-9_.]*)', - script.read_text()): - if suffix in NOT_LEDGERS or suffix.endswith(".ids"): + text = script.read_text() + dirs = _dir_vars(text) + for var, suffix in _session_paths(text): + if not suffix.endswith(".ids"): continue - offenders.append(f"{script.name}: ${{safe_sid}}{suffix}") + where = dirs.get(var) + if where not in swept: + offenders.append(f"{script.name}: ${var} -> {where or '?'}") assert not offenders, ( - "these session files clear on no compaction, because the clear matches " - f"on the `.ids` convention and they do not follow it: {offenders}" + "these ledgers live in directories the compaction clear never visits, " + f"so they outlive the context they describe: {offenders}. Add the " + "directory to SCRIBE_LEDGER_DIRS in scribe_defs.sh." + ) + + +def test_every_session_file_in_a_swept_directory_follows_the_convention(): + """The other half: the sweep matches `.ids`, so a ledger must be named it. + + Stated as its own test because the two assumptions fail differently — this + one lets a ledger sit in the right directory and still never clear. + """ + # Session files in a swept directory that are NOT per-session ledgers. + NOT_LEDGERS = {".unreached"} + + swept = _swept_dirs() + offenders = [] + for script in sorted(HOOKS.glob("*.sh")): + text = script.read_text() + dirs = _dir_vars(text) + for var, suffix in _session_paths(text): + if dirs.get(var) not in swept: + continue # a different directory, a different question + if suffix in NOT_LEDGERS or suffix.endswith(".ids"): + continue + offenders.append(f"{script.name}: ${var}/${{safe_sid}}{suffix}") + + assert not offenders, ( + "these session files sit in a swept directory but clear on no " + f"compaction, because the sweep matches `.ids`: {offenders}" ) @@ -149,13 +216,13 @@ def test_the_clear_is_derived_and_not_a_list_of_names(): """The regression that would look like a fix. Appending an `rm -f` per ledger passes every behavioural test above while - rebuilding the trap for the sixth one. The property worth keeping is that + rebuilding the trap for the next one. The property worth keeping is that the hook names no ledger at all. """ sh = SESSION_START.read_text() block = sh.split('case "$source" in')[1].split("esac")[0] assert "scribe_clear_session_ledgers" in block - for suffix in LEDGERS: + for suffix in {s for suffixes in LEDGERS.values() for s in suffixes}: assert suffix not in block, ( f"the clear names {suffix} again — a list, not a convention" ) -- 2.54.0 From 825491d8591c3fee751497c44d1d5152d9ffbaa6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 16 Sep 2026 21:16:10 -0400 Subject: [PATCH 04/12] fix(retrieval): the place arm marks its repeats too, and three tests meet the new contract (#4101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI on 5c64ea0 surfaced a half-measure I had shipped and three tests pinning the semantics it replaces. **The half-measure.** Only the semantic arm was marking repeats; the write path's PLACE arm still dropped a nearby snippet that was on the ledger. One menu, two rules, decided by which arm happened to find a record — and the marker would then read as a complete account of what the session has met while covering half the lines. `seen` now means this call's own menu and nothing else, so both reuse arms mark and neither withholds. **test_session_dedup_excludes_ids_from_the_reuse_arms** asserted the drop and the ledger going into the query. Both are the defect; it is renamed and asserts the marker instead. **test_a_pulled_snippet_already_seen_is_evidence_not_menu** asserted #7 was evidence and NOT menu, with the limit widened by one to pay for the drop. The premise was the hard exclusion; #7 is now both, and the widening is no longer needed for it. **test_a_record_this_arm_withheld_itself_is_not_a_near_miss** is the interesting one. Its property still holds and its setup no longer reaches it: a ledger repeat comes back from the search and is rendered, so nothing is withheld and `best_available` describes the bar. The one post-search filter left is a PULLED record this same call already listed by place — kept in the query because `resembles` needs its score, dropped from the menu because it is already on it — so the test is rebuilt on that, and a sibling test pins the direction that changed, so nulling the score whenever the ledger matches cannot pass both. Also stubs `superseded_ids` and `owner_names_for` in the new test file, which were reaching for Postgres in a unit job. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/plugin_context.py | 36 +++--- tests/test_ledger_references_not_silence.py | 7 +- tests/test_write_path_trigger.py | 115 +++++++++++++++++--- 3 files changed, 123 insertions(+), 35 deletions(-) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 506033a..3c63f98 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -1611,7 +1611,13 @@ async def build_write_path_hint( # stay eligible here, which is the whole point of the split. Either way # they join `seen`, so the reuse arms (where the directory query would # surface them again) never re-list a record the sync block owns. - seen: set[int] = set(excluded) + # + # `seen` IS THIS CALL'S OWN MENU, and nothing else (#4101). It used to start + # from `excluded` — the session ledger — which folded two different claims + # into one variable: "already listed a few lines above" and "shown at some + # earlier point in the session". Only the first is a reason to stay quiet. + # The ledger is now a marker instead, on both reuse arms. + seen: set[int] = set() synced: list[dict] = [] for item in here: nid = int(item["id"]) @@ -1625,7 +1631,12 @@ async def build_write_path_hint( if nid in seen: continue seen.add(nid) - placed.append(("nearby", item)) + # MARKED HERE TOO, not just on the semantic arm, because these are one + # menu. A hint where some repeats carry `seen` and others are silently + # dropped — decided by which arm happened to find them — is worse than + # either rule applied consistently: the marker would read as a complete + # account of what the session has met before, and it would not be one. + placed.append(("nearby · seen" if nid in excluded else "nearby", item)) # The stamping feed's "actually pulled it" half (#2791). Read once, before # the semantic arm, because the arm's query doubles as the resemblance @@ -1657,20 +1668,11 @@ async def build_write_path_hint( query = concept_query(query) or query if remaining > 0 and query: t0 = time.perf_counter() - # `seen` CARRIES TWO DIFFERENT CLAIMS, and only one of them is a reason - # to withhold (#4101). It was built as the ledger plus everything this - # call has already rendered, and the arm treated both the same way: - # - # - already in THIS menu (the sync block, the place arm) — listing it - # twice in one hint is noise with no reader it could help; - # - on the LEDGER from an earlier call — a different claim entirely, - # and the one #3750 says must be rendered rather than dropped. - # - # Splitting them is the whole change here. `in_menu` keeps its - # exclusion; the ledger becomes a marker on the line. - in_menu = seen - excluded # Pulled-and-already-listed ids stay in the query (as evidence for # `resembles`) but never in the menu, so the limit has to cover them. + # `seen` is this call's own menu now, which is the only thing left that + # is a reason to withhold (#4101). + in_menu = seen pulled_in_menu = in_menu & set(pulled) _rep_wp: dict = {} hits = await semantic_search_notes( @@ -1948,11 +1950,11 @@ async def build_write_path_hint( for marker, item in menu: # A rendered repeat is not a new surfacing (#4101) — same cut the log # row takes, so this table and `retrieval_logs` keep agreeing about the - # same call (#3668). The place arm never reaches here with one: a nearby - # snippet on the ledger is skipped before it is ever placed. + # same call (#3668). if int(item["id"]) in excluded: continue - arm = "write_path_place" if marker == "nearby" else "write_path_semantic" + arm = ("write_path_place" if marker.startswith("nearby") + else "write_path_semantic") by_arm.setdefault(arm, []).append(int(item["id"])) for arm, ids in by_arm.items(): record_surfaced(user_id=user_id, note_ids=ids, source=arm) diff --git a/tests/test_ledger_references_not_silence.py b/tests/test_ledger_references_not_silence.py index 798755d..be1f161 100644 --- a/tests/test_ledger_references_not_silence.py +++ b/tests/test_ledger_references_not_silence.py @@ -77,7 +77,9 @@ async def _autoinject(hits, exclude_ids, *, rec=None, surf=None): return_value={"enabled": True, "threshold": 0.55, "top_k": 3})), \ patch.object(pc, "semantic_search_notes", search), \ patch.object(pc, "record_retrieval", rec or MagicMock()), \ - patch.object(pc, "record_surfaced", surf or MagicMock()): + patch.object(pc, "record_surfaced", surf or MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "superseded_ids", AsyncMock(return_value=set())): out = await pc.build_autoinject_hint( 1, "postgres pool", project_id=2, exclude_ids=exclude_ids) return out, search @@ -175,7 +177,8 @@ async def _write_path(hits, exclude_ids, *, here=(), rec=None, sync_exclude=()): patch.object(pc.snippets_svc, "list_snippets", listing), \ patch.object(pc, "semantic_search_notes", search), \ patch.object(pc, "record_retrieval", rec or MagicMock()), \ - patch.object(pc, "record_surfaced", MagicMock()): + patch.object(pc, "record_surfaced", MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})): out = await pc.build_write_path_hint( 1, "src/x.py", code=REAL_CODE, exclude_ids=list(exclude_ids), diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index a9e8044..6a4575f 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -310,9 +310,23 @@ async def test_semantic_arm_only_asks_for_the_budget_the_place_arm_left(): @pytest.mark.asyncio -async def test_session_dedup_excludes_ids_from_the_reuse_arms(): +async def test_session_dedup_marks_the_reuse_arms_rather_than_silencing_them(): """exclude_ids governs the REUSE classes — nearby and semantic. (The sync - class has its own channel; see the tests above.)""" + class has its own channel; see the tests above.) + + WHAT CHANGED AND WHY (#4101). This used to assert the opposite: the nearby + hit was dropped and its id pushed into the search's `exclude_ids`. That was + #3750's defect on the note arms — the second time a record was the best + answer, the session got silence, which reads exactly like "nothing is + recorded here". Now the repeat is rendered with a `seen` marker, and the + ledger never reaches the query, so the score the search reports describes + the bar rather than a candidate set the caller had already edited. + + Marked on BOTH reuse arms, which is what this asserts: one menu with two + rules — `seen` on the semantic hits, silence on the nearby ones — would + make the marker read as a complete account of what the session has met, + when it would only cover half the lines. + """ from scribe.services import plugin_context as pc async def _listing(uid, **kw): @@ -327,9 +341,11 @@ async def test_session_dedup_excludes_ids_from_the_reuse_arms(): patch.object(pc, "semantic_search_notes", search), \ patch.object(pc, "record_retrieval", MagicMock()): out = await pc.build_write_path_hint(1, "src/x.py", code=REAL_CODE, exclude_ids=[12]) - # The nearby hit was already surfaced this session → dropped, not repeated. - assert out["note_ids"] == [] - assert 12 in search.await_args.kwargs["exclude_ids"] + assert out["note_ids"] == [12] + assert "seen" in next( + line for line in out["context"].splitlines() if "#12" in line + ) + assert 12 not in (search.await_args.kwargs.get("exclude_ids") or set()) # --- attribution + telemetry ------------------------------------------------- @@ -1264,8 +1280,16 @@ async def test_stamping_needs_named_shapes_and_a_recent_pull(): async def test_a_pulled_snippet_already_seen_is_evidence_not_menu(): """The pulled-then-written flow IS the dedup-excluded flow: the hint offered #7 earlier (so it sits in exclude_ids), the session pulled it, and now - writes code resembling it. #7 must be scored for this payload — and handed - to the stamp as resemblance — without being re-listed in the menu.""" + writes code resembling it. #7 must be scored for this payload and handed to + the stamp as resemblance. + + THE MENU HALF INVERTED AT #4101. This asserted that #7 was evidence and NOT + menu — scored for the stamp, kept out of the lines. That followed from the + ledger being a hard exclusion; it is now a marker, so #7 is both, and the + `limit` no longer needs widening for it because nothing is dropped after + the search. What survives unchanged is the part the stamp depends on: the + pulled id stays in the query and its score reaches `resembles`. + """ from scribe.services import plugin_context as pc search = AsyncMock(return_value=[(0.91, fake_note(id=7, title="pulled", user_id=1, note_type="snippet")), (0.80, fake_note(id=8, title="fresh", user_id=1, note_type="snippet"))]) stamp = AsyncMock(return_value=[{ @@ -1283,15 +1307,15 @@ async def test_a_pulled_snippet_already_seen_is_evidence_not_menu(): 1, "src/x.py", code=REAL_CODE, project_id=4, exclude_ids=[7], stamp_shapes=[("sym", "debounce")], repo_key="git.example.com/a/b", ) - # The query kept #7 eligible (and widened the budget by one for it)... + # The query kept #7 eligible, and needs no extra budget for it now that + # nothing is dropped between the search and the menu. kw = search.call_args.kwargs assert 7 not in kw["exclude_ids"] - assert kw["limit"] == 4 - # ...but the menu still honours the session dedup. - assert out["note_ids"] == [8] - assert "#7" not in "\n".join( - line for line in out["context"].splitlines() if "[similar" in line - ) + assert kw["limit"] == 3 + # ...and the menu carries it, marked. + assert out["note_ids"] == [7, 8] + seven = next(line for line in out["context"].splitlines() if "#7" in line) + assert "seen" in seven # The stamp saw the pull and the resemblance score for this payload. skw = stamp.call_args.kwargs assert skw["pulled"] == {7: skw["pulled"][7]} @@ -1670,9 +1694,47 @@ async def _write_path_row(rec, **kwargs): async def test_a_record_this_arm_withheld_itself_is_not_a_near_miss(): """The defect: the row's count is POST this arm's filter and the score was captured PRE it, so a withheld record is indistinguishable from one the bar - rejected — while scoring higher than anything the bar ever let through.""" + rejected — while scoring higher than anything the bar ever let through. + + REACHED DIFFERENTLY SINCE #4101, and that is the news. The ledger used to + produce this case and no longer can: a repeat comes back from the search + and is rendered, so nothing is dropped and `best_available` describes the + bar alone. What still produces it is the one post-search filter left — a + PULLED record that this same call already listed by place. It stays in the + query because `resembles` needs its score, and is dropped from the menu + because it is already on it, which is exactly the pre/post split. + + The property is unchanged and worth as much as it ever was; only the setup + that exhibits it moved. Written this way rather than deleted, because the + filter is still there and an arm that reported a score for it would be + making the same false claim about the bar. + """ + from scribe.services import plugin_context as pc rec = MagicMock() - row = await _write_path_row(rec, exclude_ids=[7]) + # Recorded at a sibling file → listed by PLACE in this same call, and + # pulled this session → kept in the query as resemblance evidence. + async def _listing(uid, **kw): + if kw["path"] == "src": + return ([_snippet_item(7, "already listed by place")], 1) + return ([], 0) + + with patch.object(pc, "get_writepath_config", AsyncMock(return_value=_cfg())), \ + patch.object(pc.snippets_svc, "list_snippets", _listing), \ + patch.object(pc, "record_retrieval", rec), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc.shape_ledger_svc, "recent_pulls", + AsyncMock(return_value={7: _ts()})), \ + patch.object(pc.shape_ledger_svc, "stamp_write_path_instances", + AsyncMock(return_value=[])), \ + patch.object(pc, "semantic_search_notes", + _search_reporting(0.9, fake_note( + id=7, title="scored", user_id=1, note_type="snippet"))): + await pc.build_write_path_hint( + 1, "src/x.py", code=REAL_CODE, + stamp_shapes=[("sym", "debounce")], + ) + row = next(c for c in rec.call_args_list + if c.kwargs["source"] == "write_path") assert row.kwargs["results"] == [], "the hit was withheld, so nothing shown" assert row.kwargs["best_available"] is None, ( @@ -1682,6 +1744,27 @@ async def test_a_record_this_arm_withheld_itself_is_not_a_near_miss(): ) +@pytest.mark.asyncio +async def test_a_ledger_repeat_no_longer_produces_that_case_at_all(): + """The other direction, and the reason the setup above had to move. + + A record on the session ledger now clears the search, is rendered with a + marker, and is counted in `suppressed` — so nothing is withheld and the + reported score describes the bar. Pinned because the tempting way to keep + the old test passing would have been to null the score whenever the ledger + matched, which would delete the measurement #3670 was built for on exactly + the calls where the bar is most worth reading. + """ + rec = MagicMock() + row = await _write_path_row(rec, exclude_ids=[7]) + + assert row.kwargs["results"] == [], "a repeat is not a fresh result" + assert row.kwargs["suppressed"] == 1, "and it is counted rather than lost" + assert row.kwargs["best_available"] == 0.9, ( + "nothing was withheld, so the reported score describes the bar" + ) + + @pytest.mark.asyncio async def test_a_call_that_withheld_nothing_still_reports_what_the_bar_refused(): """The other half, and what stops the fix being 'never report it'. -- 2.54.0 From 5d47342fbc70c67252b2e42f3aa8311297d6b189 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 16 Sep 2026 21:19:05 -0400 Subject: [PATCH 05/12] fix(tests): two expectations that the new contract corrected (#4101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were mine, and both show the change behaving as designed. `test_session_dedup_marks_the_reuse_arms_rather_than_silencing_them` asserted #12 stays out of the search's `exclude_ids`. It does not, and should not: the place arm has just listed it, so the semantic arm must not list it again. That is the same-call rule, not the ledger. The claim I meant — the LEDGER never reaches the query — is asserted on its own against a record the call has not otherwise rendered. `test_a_pulled_snippet_already_seen_is_evidence_not_menu` expected #7 and #8. It gets #7 alone, because #7 (0.91) now anchors the margin band at 0.81 and #8 scores 0.80. Withholding #7 used to promote a materially weaker hit into a slot it had not earned, with nothing in the output saying so — the band measures distance from the best answer, and letting the ledger decide which answer that is was the axis confusion the rule band was written to avoid. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_write_path_trigger.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 6a4575f..7fd3bcd 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -345,7 +345,12 @@ async def test_session_dedup_marks_the_reuse_arms_rather_than_silencing_them(): assert "seen" in next( line for line in out["context"].splitlines() if "#12" in line ) - assert 12 not in (search.await_args.kwargs.get("exclude_ids") or set()) + # #12 IS in this call's `exclude_ids`, and that is the other rule working: + # the place arm has just listed it, so the semantic arm must not list it + # again. What must never reach the query is the LEDGER, which is asserted + # on its own in tests/test_ledger_references_not_silence.py against a + # record this call has not otherwise rendered. + assert 12 in search.await_args.kwargs["exclude_ids"] # --- attribution + telemetry ------------------------------------------------- @@ -1313,9 +1318,14 @@ async def test_a_pulled_snippet_already_seen_is_evidence_not_menu(): assert 7 not in kw["exclude_ids"] assert kw["limit"] == 3 # ...and the menu carries it, marked. - assert out["note_ids"] == [7, 8] seven = next(line for line in out["context"].splitlines() if "#7" in line) assert "seen" in seven + # #8 (0.80) now falls OUTSIDE the margin band, because #7 (0.91) anchors it + # at 0.81 instead of being filtered out first. That is the point rather than + # a casualty: the band measures distance from the best answer, and the best + # answer here is #7 — suppressing it used to promote a materially weaker hit + # into a slot it had not earned, with nothing in the output saying so. + assert out["note_ids"] == [7] # The stamp saw the pull and the resemblance score for this payload. skw = stamp.call_args.kwargs assert skw["pulled"] == {7: skw["pulled"][7]} -- 2.54.0 From 09b48457ffdd90bc7dc73d77790c023448be6c48 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 11:17:23 -0400 Subject: [PATCH 06/12] refactor(retrieval): one registry for every surface's floor and budget (#4102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Groundwork for the step's real change. The operator's decision is that the floor is chosen and adjusted by the model using it, not shipped as a value somebody has to defend: "we need a model consistent surface for the adjustment of these floor values. the user should be able to touch it but the model should be the thing handling it 9 times out of 10." A tuning surface cannot be consistent across six arms that each spell their configuration differently, so the arms stop owning their numbers. `retrieval_surfaces.SURFACES` names each one, its floor key and default, its budget key and default, and — because they are rendered by the tuning tool and the Settings UI — what it asks, over what corpus, and how often it fires. A floor cannot be moved responsibly by anyone who does not know those three. Three things fall out: - **`k` becomes a real budget everywhere.** Only auto-inject had a configurable one; `RULEHINT_LIMIT`, `PROMPTRULE_LIMIT` and `reply_preferences.LIMIT` were constants. `k` is what binds under a low floor, so it has to be settable per surface — and per surface is the point, since `pre_tool_rule` fires before every Bash call while `prompt_rule` fires once a turn. - **`write_path` gets its own budget, inherited not reset.** It shared auto-inject's outright on the argument that "how many titles at once" means the same thing on both. It does not, for the same reason. Unset, it still reads auto-inject's key, so an install that tuned the shared knob does not silently drop to a new default. - **The duplicated read-and-clamp goes.** That shape is canon #2860 across 295 of 372 judged siblings. Survivable while the numbers were constants; not once they are meant to move. The long measurement comments stay exactly where they are — #2223's noise-floor probe, #3853's command-vs-code split, #3851's band measurement. The constants they annotate now alias the registry, so there is one value and the reasoning still sits beside it. Tests build the write-path config from the registry (`helpers.writepath_cfg`) instead of from hand-written dicts. That is not tidiness: the rule arms read their numbers inside a fail-open `except`, so a dict missing one key does not raise where a reader would see it — the arm silently becomes a no-op that reads exactly like "fired and found nothing". Ten hand-written dicts each looked complete on the day they were typed. tests/test_retrieval_surfaces.py pins the identity everything rests on: a surface's name IS its telemetry source. Nothing in the type system says so — `record_retrieval(source="pre_tool_rule")` is a literal in another file — and renaming one without the other yields an arm that can be tuned and not measured, or measured and not tuned, with no symptom either way. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/plugin_context.py | 167 +++++++------- src/scribe/services/reply_preferences.py | 29 +-- src/scribe/services/retrieval_surfaces.py | 243 ++++++++++++++++++++ tests/helpers.py | 29 +++ tests/test_ledger_references_not_silence.py | 6 +- tests/test_note_usage.py | 5 +- tests/test_retrieval_surfaces.py | 187 +++++++++++++++ tests/test_rule_usage_wiring.py | 70 +++--- tests/test_services_plugin_context.py | 22 +- tests/test_write_path_trigger.py | 58 +++-- 10 files changed, 644 insertions(+), 172 deletions(-) create mode 100644 src/scribe/services/retrieval_surfaces.py create mode 100644 tests/test_retrieval_surfaces.py diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 3c63f98..3a6b449 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -31,6 +31,12 @@ from scribe.services.embeddings import semantic_search_notes, semantic_search_ru from scribe.services.note_usage import record_surfaced from scribe.services.rule_usage import record_rule_surfaced from scribe.services.supersession import superseded_ids +from scribe.services.retrieval_surfaces import ( + MAX_BUDGET, + SURFACES, + budget_for, + floor_for, +) from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.settings import get_setting @@ -54,12 +60,16 @@ _GOAL_CHARS = 200 # and are meant to be tuned from retrieval_logs (source='auto_inject') once data # accrues — they're exposed in the Settings UI, no restart needed. AUTOINJECT_ENABLED_KEY = "kb_autoinject_enabled" -AUTOINJECT_THRESHOLD_KEY = "kb_autoinject_threshold" -AUTOINJECT_TOP_K_KEY = "kb_autoinject_top_k" +# The key and the value both live in the registry now (#4102); these names +# survive because the comments above them are where each number's measurement +# is recorded, and because the Settings UI agreement test pairs them with the +# Vue refs by name. One definition, two readable places. +AUTOINJECT_THRESHOLD_KEY = SURFACES["auto_inject"].floor_key +AUTOINJECT_TOP_K_KEY = SURFACES["auto_inject"].budget_key AUTOINJECT_DEFAULT_ENABLED = True -AUTOINJECT_DEFAULT_THRESHOLD = 0.55 -AUTOINJECT_DEFAULT_TOP_K = 3 +AUTOINJECT_DEFAULT_THRESHOLD = SURFACES["auto_inject"].floor_default +AUTOINJECT_DEFAULT_TOP_K = SURFACES["auto_inject"].budget_default # The write-path trigger (#2082) gets its own on/off switch, its own threshold, # and shares only top-k. It originally shared the threshold too, on the argument @@ -83,9 +93,9 @@ AUTOINJECT_DEFAULT_TOP_K = 3 # pull-through (#2085) once a real corpus accrues; a cross-encoder rerank # (#1038) would subsume this bump. WRITEPATH_ENABLED_KEY = "kb_writepath_enabled" -WRITEPATH_THRESHOLD_KEY = "kb_writepath_threshold" +WRITEPATH_THRESHOLD_KEY = SURFACES["write_path"].floor_key WRITEPATH_DEFAULT_ENABLED = True -WRITEPATH_DEFAULT_THRESHOLD = 0.68 +WRITEPATH_DEFAULT_THRESHOLD = SURFACES["write_path"].floor_default # The standing-rule arm (milestone 307) gets its own bar — the split #2223 made # one surface down, now made for the THIRD corpus. It inherited 0.68 above, and @@ -133,8 +143,8 @@ WRITEPATH_DEFAULT_THRESHOLD = 0.68 # case 0.72 was calibrated on, and the telemetry says it is working — the # write-path rule arm speaks on 37% of its calls and its refused mass sits at # p50 0.6989, comfortably under the bar rather than piled against it. -RULEHINT_THRESHOLD_KEY = "kb_rulehint_threshold" -RULEHINT_DEFAULT_THRESHOLD = 0.72 +RULEHINT_THRESHOLD_KEY = SURFACES["write_path_rule"].floor_key +RULEHINT_DEFAULT_THRESHOLD = SURFACES["write_path_rule"].floor_default # THE COMMAND ARM'S OWN BAR, AND WHY IT IS NOT THE WRITE PATH'S (#3853). # @@ -193,8 +203,8 @@ RULEHINT_DEFAULT_THRESHOLD = 0.72 # eligible. Retrieval is ownership-scoped, not project-scoped. Scoping it # would drop that ceiling and widen the 0.0115, which is the larger fix and # the reason to settle project scoping before tuning this number twice. -TOOLRULE_THRESHOLD_KEY = "kb_toolrule_threshold" -TOOLRULE_DEFAULT_THRESHOLD = 0.68 +TOOLRULE_THRESHOLD_KEY = SURFACES["pre_tool_rule"].floor_key +TOOLRULE_DEFAULT_THRESHOLD = SURFACES["pre_tool_rule"].floor_default # A SET OF RULES PER ACT, NOT THE SINGLE BEST ONE (#3851). # @@ -223,7 +233,10 @@ TOOLRULE_DEFAULT_THRESHOLD = 0.68 # and nothing else, so a moment with one clearly-relevant rule still shows # one, and a moment with four shows four. The corpus decides, not a constant. # The cap survives as a ceiling on the worst case, not as the usual answer. -RULEHINT_LIMIT = 5 +# NOW A DEFAULT RATHER THAN A CAP (#4102): both rule act-arms read their own +# budget from the registry, so this is the value an install starts at and not +# the value it is stuck with. The reasoning below is why 5 is where it starts. +RULEHINT_LIMIT = SURFACES["write_path_rule"].budget_default # MEASURED, NOT REASONED — and the reasoning it replaced was wrong (#3851). # @@ -433,7 +446,11 @@ _CONCEPT_MIN_CHARS = 16 _AUTOINJECT_BAND = 0.10 # Hard ceiling on top-k regardless of the user's setting — this is an # awareness menu (titles only), never a content dump. -_AUTOINJECT_MAX_TOP_K = 10 +# The budget ceiling, now shared by every surface rather than owned by this +# one (#4102). It bounds the same thing everywhere — how many lines a single +# unsolicited injection may occupy — so one surface having a private ceiling +# was an accident of which arm got a configurable budget first. +_AUTOINJECT_MAX_TOP_K = MAX_BUDGET # --- the prompt-boundary rule arm (#3852) ------------------------------------ # @@ -446,7 +463,7 @@ _AUTOINJECT_MAX_TOP_K = 10 # The operator's message is the only query that exists before one is composed, # and this arm is what runs against it. Until now that hook searched notes # alone, so no rule had ever been retrieved against a thing the operator said. -PROMPTRULE_THRESHOLD_KEY = "kb_promptrule_threshold" +PROMPTRULE_THRESHOLD_KEY = SURFACES["prompt_rule"].floor_key # INHERITED FROM THE ACT ARMS, AND NOT YET EARNED HERE. 0.72 was tuned against # code and shell commands. An operator's prose is a different query shape # against the same documents, and nothing yet says the two distributions line @@ -458,7 +475,7 @@ PROMPTRULE_THRESHOLD_KEY = "kb_promptrule_threshold" # front of a corpus that binds. Every call is logged under `prompt_rule` from # the first deploy, so a few days of real traffic settles it — read # `near_miss_samples` (#3807) before moving this, not the percentile alone. -PROMPTRULE_DEFAULT_THRESHOLD = 0.72 +PROMPTRULE_DEFAULT_THRESHOLD = SURFACES["prompt_rule"].floor_default # MORE THAN THE ACT ARMS' SINGLE SLOT, anchored on this hook's budget rather # than theirs. RULEHINT_LIMIT is 1 because that arm fires before EVERY Bash @@ -469,7 +486,7 @@ PROMPTRULE_DEFAULT_THRESHOLD = 0.72 # And a prompt genuinely contains more than one act. "Merge to main and then # start on X" is two, governed by different rules; k=1 cannot serve that case # at all, where the act arms never face it because a command is one thing. -PROMPTRULE_LIMIT = 3 +PROMPTRULE_LIMIT = SURFACES["prompt_rule"].budget_default # THE COMPLETION-REPORT ARM'S OWN BAR (services/reply_preferences.py). # @@ -487,8 +504,8 @@ PROMPTRULE_LIMIT = 3 # Kept at the prose arm's starting value rather than tuned: the split is what # makes the two independently movable, and a default is a product decision # that this install's corpus cannot settle (rule 115). -REPORTPREF_THRESHOLD_KEY = "kb_reportpref_threshold" -REPORTPREF_DEFAULT_THRESHOLD = 0.72 +REPORTPREF_THRESHOLD_KEY = SURFACES["report_preference"].floor_key +REPORTPREF_DEFAULT_THRESHOLD = SURFACES["report_preference"].floor_default def _slugify(text: str) -> str: @@ -592,8 +609,18 @@ async def build_process_manifest(user_id: int) -> dict: async def get_autoinject_config(user_id: int) -> dict: """Resolve a user's auto-inject settings, falling back to the defaults. - Returns {"enabled": bool, "threshold": float, "top_k": int}, clamped to - sane ranges (threshold to [0,1]; top_k to [1, _AUTOINJECT_MAX_TOP_K]). + Returns {"enabled": bool, "threshold": float, "top_k": int}. + + THE TWO NUMBERS COME FROM THE REGISTRY NOW (#4102). They used to be read and + clamped here, and identically again in `get_writepath_config`, and again in + three rule arms, and once more in `reply_preferences` — the duplication the + shape ledger counts as canon #2860 across 295 of 372 siblings. That was + tolerable while the values were shipped constants. It stops being tolerable + once a tool is expected to MOVE them, because a tuning surface cannot be + consistent across arms that each spell their configuration differently. + + `enabled` stays here: it is this surface's own switch, not a tunable number, + and the registry deliberately holds only the pair a floor-tuner touches. """ enabled_raw = await get_setting( user_id, AUTOINJECT_ENABLED_KEY, @@ -601,21 +628,11 @@ async def get_autoinject_config(user_id: int) -> dict: ) enabled = enabled_raw.strip().lower() in ("true", "1", "yes", "on") - try: - threshold = float(await get_setting( - user_id, AUTOINJECT_THRESHOLD_KEY, str(AUTOINJECT_DEFAULT_THRESHOLD))) - except (TypeError, ValueError): - threshold = AUTOINJECT_DEFAULT_THRESHOLD - threshold = min(1.0, max(0.0, threshold)) - - try: - top_k = int(float(await get_setting( - user_id, AUTOINJECT_TOP_K_KEY, str(AUTOINJECT_DEFAULT_TOP_K)))) - except (TypeError, ValueError): - top_k = AUTOINJECT_DEFAULT_TOP_K - top_k = min(_AUTOINJECT_MAX_TOP_K, max(1, top_k)) - - return {"enabled": enabled, "threshold": threshold, "top_k": top_k} + return { + "enabled": enabled, + "threshold": await floor_for(user_id, "auto_inject"), + "top_k": await budget_for(user_id, "auto_inject"), + } def _record_kind(note) -> str: @@ -1024,13 +1041,8 @@ async def build_prompt_rule_hint( return out try: - try: - threshold = float(await get_setting( - user_id, PROMPTRULE_THRESHOLD_KEY, - str(PROMPTRULE_DEFAULT_THRESHOLD))) - except (TypeError, ValueError): - threshold = PROMPTRULE_DEFAULT_THRESHOLD - threshold = min(1.0, max(0.0, threshold)) + threshold = await floor_for(user_id, "prompt_rule") + limit = await budget_for(user_id, "prompt_rule") t0 = time.perf_counter() _rep: dict = {} @@ -1041,7 +1053,7 @@ async def build_prompt_rule_hint( # sessions — this surface speaks unasked, and a whole-rulebook answer # is only right for someone who asked the whole rulebook. hits = await semantic_search_rules( - user_id, q, limit=PROMPTRULE_LIMIT, threshold=threshold, + user_id, q, limit=limit, threshold=threshold, report=_rep, project_id=project_id or None, ) duration_ms = (time.perf_counter() - t0) * 1000.0 @@ -1057,7 +1069,7 @@ async def build_prompt_rule_hint( # and unverified for this corpus, so the zero rows are the point. record_retrieval( user_id=user_id, source="prompt_rule", query=q, - threshold=threshold, limit=PROMPTRULE_LIMIT, + threshold=threshold, limit=limit, project_id=project_id, is_task=None, results=fresh, duration_ms=duration_ms, best_available=_rep.get("best_available_score"), @@ -1294,52 +1306,37 @@ def concept_query(code: str) -> str: async def get_writepath_config(user_id: int) -> dict: - """Write-path trigger settings: its own `enabled` and `threshold`, auto-inject's top_k. + """Write-path trigger settings — and the two rule arms' numbers alongside. - The threshold OVERRIDES the inherited auto-inject value — code embeddings - have a much higher similarity floor than prose, so the two surfaces need - different bars. See WRITEPATH_DEFAULT_THRESHOLD for the measurements (#2223). - top_k is still shared: "how many titles at once" means the same thing on - both surfaces, and nothing suggests they want different ceilings. + Three surfaces' worth of configuration arrives in one call because one hook + request drives all three arms. They are still three SURFACES with three + independent pairs, resolved from the registry (#4102). + + The floors were split apart one at a time, each on its own measurement, and + those measurements are recorded where the defaults are: WRITEPATH (#2223 — + code embeddings sit on a much higher similarity floor than prose), RULEHINT + (a third corpus again), TOOLRULE (#3853 — a shell command is a different + query shape from a code payload and scores lower for the same relevance). + + THE BUDGET IS NOW SPLIT TOO. `top_k` used to be auto-inject's outright, on + the argument that "how many titles at once" means the same thing on both + surfaces. It does not: this arm fires before every Write and Edit while + auto-inject fires once a turn, so the same number buys wildly different + amounts of attention. `write_path` inherits auto-inject's value when it has + none of its own, so no install that tuned the shared knob loses it. """ - cfg = await get_autoinject_config(user_id) enabled_raw = await get_setting( user_id, WRITEPATH_ENABLED_KEY, "true" if WRITEPATH_DEFAULT_ENABLED else "false", ) - - try: - threshold = float(await get_setting( - user_id, WRITEPATH_THRESHOLD_KEY, str(WRITEPATH_DEFAULT_THRESHOLD))) - except (TypeError, ValueError): - threshold = WRITEPATH_DEFAULT_THRESHOLD - threshold = min(1.0, max(0.0, threshold)) - - try: - rule_threshold = float(await get_setting( - user_id, RULEHINT_THRESHOLD_KEY, str(RULEHINT_DEFAULT_THRESHOLD))) - except (TypeError, ValueError): - rule_threshold = RULEHINT_DEFAULT_THRESHOLD - rule_threshold = min(1.0, max(0.0, rule_threshold)) - - try: - tool_rule_threshold = float(await get_setting( - user_id, TOOLRULE_THRESHOLD_KEY, str(TOOLRULE_DEFAULT_THRESHOLD))) - except (TypeError, ValueError): - tool_rule_threshold = TOOLRULE_DEFAULT_THRESHOLD - tool_rule_threshold = min(1.0, max(0.0, tool_rule_threshold)) - return { - **cfg, "enabled": enabled_raw.strip().lower() in ("true", "1", "yes", "on"), - "threshold": threshold, - # Its own bar, for a third corpus — see RULEHINT_DEFAULT_THRESHOLD. - "rule_threshold": rule_threshold, - # And the COMMAND arm's own bar again, for the same reason one level - # down: a shell command is a different query shape from a code payload - # and scores lower for the same relevance (#3853). Separate keys, so an - # install can move one without the other — which is the whole finding. - "tool_rule_threshold": tool_rule_threshold, + "threshold": await floor_for(user_id, "write_path"), + "top_k": await budget_for(user_id, "write_path"), + "rule_threshold": await floor_for(user_id, "write_path_rule"), + "rule_top_k": await budget_for(user_id, "write_path_rule"), + "tool_rule_threshold": await floor_for(user_id, "pre_tool_rule"), + "tool_rule_top_k": await budget_for(user_id, "pre_tool_rule"), } def _rule_band(hits: list) -> list: @@ -1988,7 +1985,7 @@ async def build_write_path_hint( rule_t0 = time.perf_counter() _rep_wpr: dict = {} hits = await semantic_search_rules( - user_id, code or path, limit=RULEHINT_LIMIT, + user_id, code or path, limit=cfg["rule_top_k"], threshold=cfg["rule_threshold"], report=_rep_wpr, project_id=project_id or None, ) @@ -2049,7 +2046,7 @@ async def build_write_path_hint( # the same readout. record_retrieval( user_id=user_id, source="write_path_rule", query=code or path, - threshold=cfg["rule_threshold"], limit=RULEHINT_LIMIT, + threshold=cfg["rule_threshold"], limit=cfg["rule_top_k"], project_id=project_id, is_task=None, results=fresh, duration_ms=rule_ms, best_available=_rep_wpr.get("best_available_score"), @@ -2145,7 +2142,7 @@ async def build_tool_rule_hint( t0 = time.perf_counter() _rep_ptr: dict = {} hits = await semantic_search_rules( - user_id, query, limit=RULEHINT_LIMIT, + user_id, query, limit=cfg["tool_rule_top_k"], threshold=cfg["tool_rule_threshold"], report=_rep_ptr, project_id=project_id or None, ) @@ -2168,7 +2165,7 @@ async def build_tool_rule_hint( # failure the arm was built to stop. record_retrieval( user_id=user_id, source="pre_tool_rule", query=query, - threshold=cfg["tool_rule_threshold"], limit=RULEHINT_LIMIT, + threshold=cfg["tool_rule_threshold"], limit=cfg["tool_rule_top_k"], project_id=project_id, is_task=None, results=fresh, duration_ms=duration_ms, best_available=_rep_ptr.get("best_available_score"), diff --git a/src/scribe/services/reply_preferences.py b/src/scribe/services/reply_preferences.py index fd04b5d..40bb968 100644 --- a/src/scribe/services/reply_preferences.py +++ b/src/scribe/services/reply_preferences.py @@ -61,13 +61,9 @@ import logging import time from scribe.services.embeddings import semantic_search_rules -from scribe.services.plugin_context import ( - REPORTPREF_DEFAULT_THRESHOLD, - REPORTPREF_THRESHOLD_KEY, -) +from scribe.services.retrieval_surfaces import SURFACES, budget_for, floor_for from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.rule_usage import record_rule_surfaced -from scribe.services.settings import get_setting logger = logging.getLogger(__name__) @@ -85,16 +81,20 @@ COMPLETION_QUERY = ( # A handful, not a menu. More than a few shape preferences for ONE kind of # reply would contradict each other before they helped; the limit is here to # keep one noisy corpus from turning a status change into a wall of text. -LIMIT = 3 +# The STARTING budget, not the budget (#4102). Both numbers this arm runs on +# now come from the surface registry, so the model that reads this arm's +# telemetry can move either — which matters more here than anywhere else, +# because a fixed query makes this arm's score a constant and a floor a hair +# above it produces a dead arm no amount of traffic will ever reveal. +LIMIT = SURFACES["report_preference"].budget_default async def _threshold(user_id: int) -> float: - try: - value = float(await get_setting( - user_id, REPORTPREF_THRESHOLD_KEY, str(REPORTPREF_DEFAULT_THRESHOLD))) - except (TypeError, ValueError): - value = REPORTPREF_DEFAULT_THRESHOLD - return min(1.0, max(0.0, value)) + return await floor_for(user_id, SOURCE) + + +async def _limit(user_id: int) -> int: + return await budget_for(user_id, SOURCE) async def completion_preferences(user_id: int, *, project_id: int | None = None) -> list[dict]: @@ -115,16 +115,17 @@ async def completion_preferences(user_id: int, *, project_id: int | None = None) """ try: threshold = await _threshold(user_id) + limit = await _limit(user_id) report: dict = {} t0 = time.perf_counter() hits = await semantic_search_rules( - user_id, COMPLETION_QUERY, limit=LIMIT, threshold=threshold, + user_id, COMPLETION_QUERY, limit=limit, threshold=threshold, kind="preference", report=report, project_id=project_id, ) hits = [(score, rule) for score, rule in hits if rule.kind == "preference"] record_retrieval( user_id=user_id, source=SOURCE, query=COMPLETION_QUERY, - threshold=threshold, limit=LIMIT, project_id=project_id, + threshold=threshold, limit=limit, project_id=project_id, is_task=None, results=hits, best_available=report.get("best_available_score"), best_available_id=report.get("best_available_id"), diff --git a/src/scribe/services/retrieval_surfaces.py b/src/scribe/services/retrieval_surfaces.py new file mode 100644 index 0000000..2fcdbd8 --- /dev/null +++ b/src/scribe/services/retrieval_surfaces.py @@ -0,0 +1,243 @@ +"""One registry of the retrieval surfaces, and the two numbers each one has (#4102). + +WHY THIS EXISTS + +Six push arms each carried their own loose copy of the same shape: a settings +key, a default, and a limit that was usually a module constant nobody could +change. `_threshold()` was duplicated so widely that the shape ledger counts it +as canon across 295 of 372 judged siblings (snippet #2860). That was survivable +while the numbers were shipped constants an operator occasionally edited. + +It stops being survivable once the numbers are meant to MOVE. The operator's +decision for this step: + + "the floor should be chosen and adjusted by the model using it. we've come + back to something either fails or has to be looked at by the user we need a + model consistent surface for the adjustment of these floor values. the user + should be able to touch it but the model should be the thing handling it 9 + times out of 10." + +A tuning surface cannot be "model consistent" if every arm spells its own +configuration differently. So the arms stop owning their numbers and read them +from here instead, and the tuning tool, the routes and the Settings UI all +enumerate THIS table rather than hard-coding six special cases. + +WHAT A FLOOR IS NOW, AND WHAT IT IS NOT + +It is not a relevance judgement. Relevance is decided by the reader, which is +the only participant that can read a trigger against a situation — that is the +milestone's whole argument, and the injected line has always said so out loud +("read it before deciding it does not apply"). + +A floor answers the cheaper question: *is this worth ranking at all*. `k` is +what binds, and `k` is a BUDGET — how much of this surface's attention a +candidate list may spend. An arm that fires before every Bash call cannot +afford what an arm that fires once a turn can. + +WHY THE DEFAULTS BELOW ARE STARTING POINTS AND NOT ANSWERS + +A cosine score is a distance in BAAI/bge-small-en-v1.5's vector space, measured +against THIS corpus. It cannot transfer to an install with different records, +and rule 115 forbids defending a shipped default from this instance's +telemetry — which is what made the old design unbuildable: every number was a +guess everywhere except here, and there was no mechanism that could ever +improve it. + +The mechanism is the fix. Scribe ships a starting point and the means to +correct it, so the values below carry the measurement that motivated them (see +the long comments in `plugin_context.py`, which are kept where they are because +they record how each number was first arrived at) without claiming to be right +for anybody else. + +HOW A FLOOR SHOULD ACTUALLY BE MOVED + +By reading the records the floor refused — `retrieval_telemetry( +near_miss_samples=N)` returns them by id — and never by the percentile alone. +That is not a style preference; it is the one case where the two disagreed and +was checked. `report_preference` logged 69 consecutive declines with the +refused score 0.0006 under the bar, and every percentile said "lower it". +Reading the refused record showed it was rule 77 "Extract intent from loose +phrasing", a false positive, so lowering the bar would have delivered that rule +on every completion report ever written. The statistic and the correct action +pointed in opposite directions, and only opening the record could tell. +""" +from __future__ import annotations + +from dataclasses import dataclass + +from scribe.services.settings import get_setting + +# A budget nobody should be able to set past. Not a tuning value — a guard on +# the worst case, so a mistyped setting cannot turn a menu into a wall of text. +# Shared by every surface because it bounds the same thing everywhere: how many +# lines a single unsolicited injection may occupy. +MAX_BUDGET = 10 + + +@dataclass(frozen=True) +class Surface: + """One push arm's tunable pair, plus enough prose to tune it responsibly. + + `asks` / `over` / `fires` are not documentation for this file — they are + rendered by the tuning tool and the Settings UI. A floor cannot be moved + sensibly by anyone, model or human, who does not know what the query is, what + corpus it runs against, or how often it costs something. Those three facts + are exactly what separates these arms from each other, and they were + previously recoverable only by reading `plugin_context.py`. + """ + + name: str + """The telemetry `source` value, and the join key. + + MUST equal the string this arm passes to `record_retrieval`. Everything + useful about tuning depends on that identity: the tool that moves a floor + and the table that says what the floor did have to be talking about the same + arm. A test asserts it rather than a comment asking nicely. + """ + + floor_key: str + floor_default: float + budget_key: str + budget_default: int + asks: str + over: str + fires: str + budget_falls_back_to: str = "" + """A budget key to inherit when this surface has none of its own set. + + Only `write_path` uses it, and only because it USED to share auto-inject's + `top_k` outright. Giving it a key without this would silently reset the + budget of every install that had tuned the shared one — a behaviour change + delivered as a default, which is the shape of regression nobody reports + because nothing looks broken. + """ + + +SURFACES: dict[str, Surface] = { + "auto_inject": Surface( + name="auto_inject", + floor_key="kb_autoinject_threshold", + floor_default=0.55, + budget_key="kb_autoinject_top_k", + budget_default=3, + asks="the operator's message, as they typed it", + over="notes, snippets, processes and issues", + fires="once per operator turn", + ), + "write_path": Surface( + name="write_path", + floor_key="kb_writepath_threshold", + floor_default=0.68, + budget_key="kb_writepath_top_k", + budget_default=3, + budget_falls_back_to="kb_autoinject_top_k", + asks="the code being written, rewritten as a concept query", + over="snippets and recorded issues", + fires="before every Write and Edit", + ), + "write_path_rule": Surface( + name="write_path_rule", + floor_key="kb_rulehint_threshold", + floor_default=0.72, + budget_key="kb_rulehint_top_k", + budget_default=5, + asks="the code being written, against rule triggers", + over="global rules plus the bound project's own", + fires="before every Write and Edit", + ), + "pre_tool_rule": Surface( + name="pre_tool_rule", + floor_key="kb_toolrule_threshold", + floor_default=0.68, + budget_key="kb_toolrule_top_k", + budget_default=5, + asks="the command about to run, against rule triggers", + over="global rules plus the bound project's own", + fires="before every Bash call — the busiest arm there is", + ), + "prompt_rule": Surface( + name="prompt_rule", + floor_key="kb_promptrule_threshold", + floor_default=0.72, + budget_key="kb_promptrule_top_k", + budget_default=3, + asks="the operator's message, against rule triggers", + over="global rules plus the bound project's own", + fires="once per operator turn", + ), + "report_preference": Surface( + name="report_preference", + floor_key="kb_reportpref_threshold", + floor_default=0.72, + budget_key="kb_reportpref_top_k", + budget_default=3, + # THE ONE FIXED QUERY, and the reason this arm behaves unlike the rest. + # The others score something that varies per call; this one scores a + # constant string, so its top score for a given corpus is also a + # constant. A floor a hair above that constant is not a quiet arm, it is + # a dead one, and no amount of traffic will ever reveal it — which is + # precisely how this arm spent 69 calls declining the same record. + asks="a fixed question about how to lay out a completion report", + over="preferences", + fires="when a task finishes", + ), +} + +# Reserved slots are deliberately absent. `preference_slot` and `reuse_slot` +# borrow their parent arm's floor and are hard-limited to one hit each, because +# their entire purpose is to guarantee a single line to a kind of record that +# keeps losing a general score contest (#2246, #3894). A budget of "1" is the +# feature; exposing it as tunable would invite setting it to 0 and silently +# removing the guarantee. + + +def surface_names() -> list[str]: + """Every tunable surface, in a stable order for menus and listings.""" + return list(SURFACES) + + +def get_surface(name: str) -> Surface: + """Look one up, refusing an unknown name loudly. + + A typo'd surface must not be writable. Settings keys are free-form strings + in a generic table, so a tuning call naming `pretool_rule` would otherwise + write a key nothing ever reads — a change that appears to succeed, reports a + new value, and alters nothing. + """ + try: + return SURFACES[name] + except KeyError: + raise ValueError( + f"unknown retrieval surface {name!r}. Tunable surfaces are: " + + ", ".join(surface_names()) + ) from None + + +async def floor_for(user_id: int, name: str) -> float: + """This install's current floor for a surface, clamped to [0, 1].""" + s = get_surface(name) + try: + value = float(await get_setting(user_id, s.floor_key, str(s.floor_default))) + except (TypeError, ValueError): + value = s.floor_default + return min(1.0, max(0.0, value)) + + +async def budget_for(user_id: int, name: str) -> int: + """This install's current budget for a surface, clamped to [1, MAX_BUDGET]. + + The lower clamp is 1, never 0: a surface turned off is turned off by its + `enabled` switch, which says so. A budget of zero would be an arm that runs + a search, logs a retrieval, and renders nothing — indistinguishable in the + telemetry from a bar nothing cleared, which is the exact confusion this + milestone exists to remove. + """ + s = get_surface(name) + raw = await get_setting(user_id, s.budget_key, "") + if not raw and s.budget_falls_back_to: + raw = await get_setting(user_id, s.budget_falls_back_to, "") + try: + value = int(float(raw)) if raw else s.budget_default + except (TypeError, ValueError): + value = s.budget_default + return min(MAX_BUDGET, max(1, value)) diff --git a/tests/helpers.py b/tests/helpers.py index 73ce17e..0dcd784 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -322,3 +322,32 @@ def http_sink(reply: bytes = b'{"context":"","note_ids":[]}'): finally: server.shutdown() server.server_close() + + +def writepath_cfg(**over): + """A complete `get_writepath_config` stand-in, built from the registry (#4102). + + DERIVED, NOT LITERAL, and the reason is a failure mode this file already + warned about in prose without being able to prevent: the write-path hint + drives three arms, each of which reads its numbers out of the config dict + inside a fail-open `except`. A dict missing one key does not raise where a + reader would see it — the arm silently becomes a no-op, which is + indistinguishable from the arm working and finding nothing. + + So the keys come from `retrieval_surfaces.SURFACES`. A seventh surface, or a + rename, changes this helper for free and cannot quietly disable an arm in + ten hand-written dicts that each looked complete on the day they were typed. + """ + from scribe.services.retrieval_surfaces import SURFACES + + cfg = { + "enabled": True, + "threshold": SURFACES["write_path"].floor_default, + "top_k": SURFACES["write_path"].budget_default, + "rule_threshold": SURFACES["write_path_rule"].floor_default, + "rule_top_k": SURFACES["write_path_rule"].budget_default, + "tool_rule_threshold": SURFACES["pre_tool_rule"].floor_default, + "tool_rule_top_k": SURFACES["pre_tool_rule"].budget_default, + } + cfg.update(over) + return cfg diff --git a/tests/test_ledger_references_not_silence.py b/tests/test_ledger_references_not_silence.py index be1f161..a190a19 100644 --- a/tests/test_ledger_references_not_silence.py +++ b/tests/test_ledger_references_not_silence.py @@ -41,7 +41,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note +from tests.helpers import fake_note, writepath_cfg REAL_CODE = '''def debounce(fn, wait=0.25): """Rate-limit a callback so it fires once after the last call.""" @@ -59,9 +59,7 @@ REAL_CODE = '''def debounce(fn, wait=0.25): def _wp_cfg(**over): - base = {"enabled": True, "threshold": 0.68, "top_k": 3, "rule_threshold": 0.72} - base.update(over) - return base + return writepath_cfg(**over) def _snippet_item(nid, title, user_id=1): diff --git a/tests/test_note_usage.py b/tests/test_note_usage.py index 26a2fc0..5ab69cf 100644 --- a/tests/test_note_usage.py +++ b/tests/test_note_usage.py @@ -20,7 +20,7 @@ from scribe.services.note_usage import ( record_surfaced, usage_for_notes, ) -from tests.helpers import fake_note +from tests.helpers import fake_note, writepath_cfg # --- recording ------------------------------------------------------------ @@ -154,8 +154,7 @@ async def test_unscored_location_arms_are_recorded(lookups, expected_source): patch.object( plugin_context, "get_writepath_config", - AsyncMock(return_value={"enabled": True, "threshold": 0.55, "top_k": 3, - "rule_threshold": 0.72}), + AsyncMock(return_value=writepath_cfg(threshold=0.55)), ), patch.object( plugin_context.snippets_svc, diff --git a/tests/test_retrieval_surfaces.py b/tests/test_retrieval_surfaces.py new file mode 100644 index 0000000..1f6a720 --- /dev/null +++ b/tests/test_retrieval_surfaces.py @@ -0,0 +1,187 @@ +"""The surface registry, and the identity the whole tuning story rests on (#4102). + +WHY THIS EXISTS + +Six arms used to own their own numbers: a settings key, a default, and a limit +that was usually a module constant nobody could change. That survived while the +values were shipped constants. It stops surviving once the operator's decision +is that the numbers MOVE: + + "the floor should be chosen and adjusted by the model using it… the user + should be able to touch it but the model should be the thing handling it 9 + times out of 10." + +A tuning surface cannot be model-consistent across arms that each spell their +configuration differently, so the arms now read `{floor, budget}` from one +registry. + +WHAT THIS PINS + + 1. **A surface's name IS its telemetry source.** This is the load-bearing + one. The tool that moves a floor and the table that reports what the floor + did have to be naming the same arm, and nothing in the type system says so + — `record_retrieval(source="pre_tool_rule")` is a string literal in a + different file. Renaming one without the other produces a surface that can + be tuned and cannot be measured, or measured and not tuned, and both fail + silently. + 2. **Keys are unique.** Two surfaces sharing a settings key is how + `report_preference` spent its first release moving whenever the prompt arm + was tuned (#3860) — one dial wearing two labels. + 3. **An unknown surface is refused.** Settings keys are free-form strings in + a generic table, so a typo'd name would write a key nothing reads: a + change that appears to succeed, reports a new value, and alters nothing. + 4. **The budget clamps at 1, never 0.** A zero-budget arm searches, logs a + retrieval and renders nothing — indistinguishable in the telemetry from a + bar nothing cleared, which is the exact confusion this milestone exists to + remove. "Off" is what the `enabled` switch is for. + 5. **`write_path` inherits auto-inject's budget when it has none.** It used + to share that key outright; giving it its own without a fallback would + silently reset the budget on every install that had tuned the shared one. +""" +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.services import retrieval_surfaces as rs + +SERVICES = ("plugin_context", "reply_preferences") + + +def _service_source(name: str) -> str: + from pathlib import Path + root = Path(__file__).resolve().parents[1] / "src" / "scribe" / "services" + return (root / f"{name}.py").read_text() + + +def test_every_surface_name_is_a_real_telemetry_source(): + """The join key, checked against the arms that emit it. + + Asserted on the source text rather than by calling the arms, because what + can rot here is the literal: a surface renamed in the registry and not in + the `record_retrieval(source=…)` call still runs, still logs, and still + tunes — just against two different names, so the readout an operator uses + to justify a change describes a different arm from the one the change hits. + """ + blob = "\n".join(_service_source(n) for n in SERVICES) + # `report_preference` passes its name through a module constant rather than + # a literal, so that one name is satisfied by the constant holding it. + from scribe.services.reply_preferences import SOURCE + + missing = [ + s.name for s in rs.SURFACES.values() + if f'source="{s.name}"' not in blob and s.name != SOURCE + ] + assert not missing, ( + f"these surfaces can be tuned but never measured: {missing}. The " + "registry name must match the string the arm passes to record_retrieval." + ) + + +def test_no_two_surfaces_share_a_settings_key(): + """One dial, one label. #3860 is what the other way costs.""" + floors = [s.floor_key for s in rs.SURFACES.values()] + budgets = [s.budget_key for s in rs.SURFACES.values()] + assert len(set(floors)) == len(floors), f"duplicate floor key in {floors}" + assert len(set(budgets)) == len(budgets), f"duplicate budget key in {budgets}" + assert not (set(floors) & set(budgets)), "a floor key doubles as a budget key" + + +def test_every_surface_says_what_it_asks_over_what_and_how_often(): + """The prose is rendered, not decorative. + + A floor cannot be moved responsibly by anyone — model or human — who does + not know the query shape, the corpus, or how often the arm costs something. + An empty string here reaches the tuning tool and the Settings UI as a blank. + """ + for s in rs.SURFACES.values(): + for field in ("asks", "over", "fires"): + assert getattr(s, field).strip(), f"{s.name}.{field} is empty" + + +def test_an_unknown_surface_is_refused_rather_than_written(): + with pytest.raises(ValueError) as e: + rs.get_surface("pretool_rule") # a real typo for pre_tool_rule + # The message has to name the alternatives, or the caller's next move is a + # second guess. + assert "pre_tool_rule" in str(e.value) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("stored, expected", [ + ("0.8", 0.8), + ("5", 1.0), # clamped, not believed + ("-3", 0.0), + ("banana", 0.55), # unparseable falls back to the surface's default + ("", 0.55), +]) +async def test_a_floor_is_clamped_to_the_unit_interval(stored, expected): + with patch.object(rs, "get_setting", AsyncMock(return_value=stored)): + assert await rs.floor_for(1, "auto_inject") == expected + + +@pytest.mark.asyncio +@pytest.mark.parametrize("stored, expected", [ + ("4", 4), + ("999", rs.MAX_BUDGET), + ("0", 1), + ("-2", 1), + ("banana", 3), +]) +async def test_a_budget_is_clamped_with_a_floor_of_one(stored, expected): + """Zero is the value that must not get through. + + An arm with a budget of 0 runs its search, writes a `retrieval_logs` row + with `result_count == 0`, and renders nothing — which reads in the telemetry + exactly like a bar nothing cleared. Turning a surface off is the `enabled` + switch's job, and that one says so. + """ + with patch.object(rs, "get_setting", AsyncMock(return_value=stored)): + assert await rs.budget_for(1, "auto_inject") == expected + + +@pytest.mark.asyncio +async def test_the_write_path_budget_falls_back_to_auto_injects(): + """The migration this fallback exists for. + + `write_path` had no budget key of its own — it read auto-inject's. An + install that had tuned that shared knob to 6 must not silently drop to the + new key's default the day this ships; nothing would look broken and the + operator would never know to look. + """ + async def _setting(_uid, key, default=""): + return {"kb_writepath_top_k": "", "kb_autoinject_top_k": "6"}.get(key, default) + + with patch.object(rs, "get_setting", AsyncMock(side_effect=_setting)): + assert await rs.budget_for(1, "write_path") == 6 + + +@pytest.mark.asyncio +async def test_its_own_budget_wins_once_it_is_set(): + """And the fallback must not become a permanent override.""" + async def _setting(_uid, key, default=""): + return {"kb_writepath_top_k": "2", "kb_autoinject_top_k": "6"}.get(key, default) + + with patch.object(rs, "get_setting", AsyncMock(side_effect=_setting)): + assert await rs.budget_for(1, "write_path") == 2 + + +@pytest.mark.asyncio +async def test_the_write_path_config_carries_every_arm_it_drives(): + """One hook request drives three arms, and each reads its pair out of this. + + A missing key does not raise where a reader would see it — the rule arms + fail open, so a KeyError becomes an empty hint, and the arm reads as "fired + and found nothing". That is the failure this asserts against, and it is the + reason tests build the dict from the registry rather than by hand. + """ + from scribe.services import plugin_context as pc + + # Both modules read settings: `pc` for the enabled switch, `rs` for the + # pairs. Patching one and not the other reaches a real database. + with patch.object(pc, "get_setting", AsyncMock(return_value="")), \ + patch.object(rs, "get_setting", AsyncMock(return_value="")): + cfg = await pc.get_writepath_config(1) + + for key in ("threshold", "top_k", "rule_threshold", "rule_top_k", + "tool_rule_threshold", "tool_rule_top_k"): + assert key in cfg, f"{key} missing — its arm will silently no-op" diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index 6b383d6..554f2af 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -17,7 +17,8 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note, fake_rule +from scribe.services import retrieval_surfaces as rs +from tests.helpers import fake_note, fake_rule, writepath_cfg # The MCP tool layer reads its caller from a ContextVar the HTTP transport sets # per request; a unit test has no request, so it binds the caller itself. The @@ -59,16 +60,15 @@ def _arm_patches(pc, hits, recorder, prior_art=None, cfg=None, rule_search=None, """ return ( patch.object(pc, "get_writepath_config", - AsyncMock(return_value=cfg or { - "enabled": True, "threshold": 0.6, - "top_k": 3, "rule_threshold": 0.6, - # The command arm reads its OWN bar since #3853, and - # a stub missing this key does not fail where a - # reader would see it: the arm fails open, so the - # KeyError becomes an empty hint and every case in - # _ARMS reports the arm went silent instead. - "tool_rule_threshold": 0.6, - })), + # Every key, derived from the surface registry (#4102). + # A stub missing one does not fail where a reader would + # see it: the arm fails open, so the KeyError becomes an + # empty hint and every case in _ARMS reports the arm went + # silent instead. + AsyncMock(return_value=cfg or writepath_cfg( + threshold=0.6, rule_threshold=0.6, + tool_rule_threshold=0.6, + ))), patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), patch.object(pc, "semantic_search_notes", AsyncMock(return_value=_PRIOR_ART if prior_art is None @@ -156,8 +156,7 @@ async def test_the_arm_searches_on_its_OWN_bar_not_the_code_one(): with ExitStack() as stack: for ctx in _arm_patches( pc, [], MagicMock(), rule_search=search, - cfg={"enabled": True, "threshold": 0.60, - "top_k": 3, "rule_threshold": 0.81}, + cfg=writepath_cfg(threshold=0.60, top_k=3, rule_threshold=0.81), ): stack.enter_context(ctx) await pc.build_write_path_hint( @@ -392,16 +391,15 @@ def test_every_rules_payload_caller_names_itself(): def _tool_patches(pc, hits, recorder, cfg=None, retrieval_log=None): return ( patch.object(pc, "get_writepath_config", - AsyncMock(return_value=cfg or { - "enabled": True, "threshold": 0.6, - "top_k": 3, "rule_threshold": 0.6, - # The command arm reads its OWN bar since #3853, and - # a stub missing this key does not fail where a - # reader would see it: the arm fails open, so the - # KeyError becomes an empty hint and every case in - # _ARMS reports the arm went silent instead. - "tool_rule_threshold": 0.6, - })), + # Every key, derived from the surface registry (#4102). + # A stub missing one does not fail where a reader would + # see it: the arm fails open, so the KeyError becomes an + # empty hint and every case in _ARMS reports the arm went + # silent instead. + AsyncMock(return_value=cfg or writepath_cfg( + threshold=0.6, rule_threshold=0.6, + tool_rule_threshold=0.6, + ))), patch.object(pc, "semantic_search_rules", AsyncMock(return_value=hits)), patch.object(pc, "record_retrieval", retrieval_log or MagicMock()), patch.object(pc, "record_rule_surfaced", recorder), @@ -433,9 +431,10 @@ async def _run_tool_arm(hits, recorder, command="curl -s https://git.example/api def _prompt_patches(pc, hits, recorder, retrieval_log=None): return ( - # The arm reads its own threshold key rather than a shared config - # object — a third corpus with a bar nothing has yet tuned for it. - patch.object(pc, "get_setting", AsyncMock(return_value="0.6")), + # The arm reads its own floor and budget from the surface registry + # rather than a shared config object (#4102) — a third corpus, with a + # pair nothing has yet tuned for it. + patch.object(rs, "get_setting", AsyncMock(return_value="0.6")), patch.object(pc, "semantic_search_rules", AsyncMock(return_value=hits)), patch.object(pc, "record_retrieval", retrieval_log or MagicMock()), patch.object(pc, "record_rule_surfaced", recorder), @@ -469,7 +468,7 @@ async def test_the_prompt_arm_retrieves_against_what_the_operator_SAID(): ))]) from scribe.services import plugin_context as pc with ExitStack() as stack: - stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6"))) + stack.enter_context(patch.object(rs, "get_setting", AsyncMock(return_value="0.6"))) stack.enter_context(patch.object(pc, "semantic_search_rules", search)) stack.enter_context(patch.object(pc, "record_retrieval", MagicMock())) stack.enter_context(patch.object(pc, "record_rule_surfaced", rec)) @@ -503,7 +502,7 @@ async def test_the_prompt_arm_says_nothing_when_asked_nothing(): log = MagicMock() from scribe.services import plugin_context as pc with ExitStack() as stack: - stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6"))) + stack.enter_context(patch.object(rs, "get_setting", AsyncMock(return_value="0.6"))) stack.enter_context(patch.object(pc, "semantic_search_rules", search)) stack.enter_context(patch.object(pc, "record_retrieval", log)) stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock())) @@ -1416,7 +1415,7 @@ async def _run_slot(general, preference, recorder=None, retrieval_log=None, **kw from scribe.services import plugin_context as pc rec = recorder or MagicMock() with ExitStack() as stack: - stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6"))) + stack.enter_context(patch.object(rs, "get_setting", AsyncMock(return_value="0.6"))) stack.enter_context(patch.object( pc, "semantic_search_rules", _search_by_kind(general, preference))) stack.enter_context(patch.object( @@ -1480,7 +1479,7 @@ async def test_the_slot_query_can_only_answer_with_a_preference(): from scribe.services import plugin_context as pc search = _search_by_kind(_RULES_FILLING_THE_LIMIT, _PREF_HIT) with ExitStack() as stack: - stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6"))) + stack.enter_context(patch.object(rs, "get_setting", AsyncMock(return_value="0.6"))) stack.enter_context(patch.object(pc, "semantic_search_rules", search)) stack.enter_context(patch.object(pc, "record_retrieval", MagicMock())) stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock())) @@ -1617,8 +1616,7 @@ async def test_each_act_arm_searches_at_its_own_bar(): """The split, where it actually takes effect.""" from scribe.services import plugin_context as pc - cfg = {"enabled": True, "threshold": 0.6, "top_k": 3, - "rule_threshold": 0.77, "tool_rule_threshold": 0.61} + cfg = writepath_cfg(threshold=0.6, top_k=3, rule_threshold=0.77, tool_rule_threshold=0.61) search = AsyncMock(return_value=list(_THREE_HITS)) with ExitStack() as stack: @@ -1646,8 +1644,7 @@ async def test_an_act_arm_reports_the_bar_it_actually_searched_at(): """ from scribe.services import plugin_context as pc - cfg = {"enabled": True, "threshold": 0.6, "top_k": 3, - "rule_threshold": 0.77, "tool_rule_threshold": 0.61} + cfg = writepath_cfg(threshold=0.6, top_k=3, rule_threshold=0.77, tool_rule_threshold=0.61) search = AsyncMock(return_value=list(_THREE_HITS)) log = MagicMock() @@ -1712,8 +1709,7 @@ async def test_the_act_arms_scope_their_search_to_the_bound_project(bound, scope global rules only, which the search spells as `project_id=None`.""" from scribe.services import plugin_context as pc - cfg = {"enabled": True, "threshold": 0.6, "top_k": 3, - "rule_threshold": 0.6, "tool_rule_threshold": 0.6} + cfg = writepath_cfg(threshold=0.6, top_k=3, rule_threshold=0.6, tool_rule_threshold=0.6) tool_search = AsyncMock(return_value=[]) with ExitStack() as stack: stack.enter_context(patch.object( @@ -1726,7 +1722,7 @@ async def test_the_act_arms_scope_their_search_to_the_bound_project(bound, scope prompt_search = AsyncMock(return_value=[]) with ExitStack() as stack: - stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6"))) + stack.enter_context(patch.object(rs, "get_setting", AsyncMock(return_value="0.6"))) stack.enter_context(patch.object(pc, "semantic_search_rules", prompt_search)) stack.enter_context(patch.object(pc, "record_retrieval", MagicMock())) stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock())) diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index fff5f62..9e159ae 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -1,7 +1,8 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note +from scribe.services import retrieval_surfaces as rs +from tests.helpers import fake_note, writepath_cfg pytestmark = pytest.mark.usefixtures("_no_supersession") @@ -17,7 +18,8 @@ async def test_get_autoinject_config_defaults_and_clamps(): from scribe.services import plugin_context as pc # No settings stored → defaults. - with patch.object(pc, "get_setting", AsyncMock(side_effect=lambda uid, k, d: d)): + with patch.object(pc, "get_setting", AsyncMock(side_effect=lambda uid, k, d: d)), \ + patch.object(rs, "get_setting", AsyncMock(side_effect=lambda uid, k, d="": d)): cfg = await pc.get_autoinject_config(1) assert cfg == { "enabled": pc.AUTOINJECT_DEFAULT_ENABLED, @@ -31,8 +33,12 @@ async def test_get_autoinject_config_defaults_and_clamps(): pc.AUTOINJECT_THRESHOLD_KEY: "5", pc.AUTOINJECT_TOP_K_KEY: "999", } - with patch.object(pc, "get_setting", - AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))): + # The switch comes from plugin_context, the two numbers from the registry. + def _side(uid, k, d=""): + return stored.get(k, d) + + with patch.object(pc, "get_setting", AsyncMock(side_effect=_side)), \ + patch.object(rs, "get_setting", AsyncMock(side_effect=_side)): cfg = await pc.get_autoinject_config(1) assert cfg["enabled"] is False assert cfg["threshold"] == 1.0 @@ -437,9 +443,7 @@ async def test_write_path_semantic_arm_asks_for_experience_not_just_snippets(): search = AsyncMock(return_value=hits) rec = MagicMock() with patch.object(pc, "get_writepath_config", - AsyncMock(return_value={"enabled": True, "threshold": 0.6, - "top_k": 3, - "rule_threshold": 0.72})), \ + AsyncMock(return_value=writepath_cfg(threshold=0.6))), \ patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), \ patch.object(pc, "semantic_search_notes", search), \ @@ -470,9 +474,7 @@ async def test_write_path_labels_a_non_snippet_hit_with_its_kind(): hits = [(0.72, fake_note(id=9, title="debounce helper", user_id=1, note_type="snippet")), (0.71, fake_note(id=7, title="Debounce dropped the trailing call", user_id=1, is_task=True, task_kind="issue"))] with patch.object(pc, "get_writepath_config", - AsyncMock(return_value={"enabled": True, "threshold": 0.6, - "top_k": 3, - "rule_threshold": 0.72})), \ + AsyncMock(return_value=writepath_cfg(threshold=0.6))), \ patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), \ patch.object(pc, "semantic_search_notes", AsyncMock(return_value=hits)), \ diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 7fd3bcd..31a1632 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -6,6 +6,7 @@ source, and the two ways this must stay silent. Plus the plugin hook contract a PreToolUse hook that returns a permission decision would be able to block the operator's edit, which this feature must never do. """ +import contextlib import json import re import subprocess @@ -13,7 +14,30 @@ from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note, http_sink +from tests.helpers import fake_note, http_sink, writepath_cfg + + +@contextlib.contextmanager +def _stored_settings(stored): + """Patch BOTH readers, because one call now uses two (#4102). + + `get_writepath_config` reads its `enabled` switch through + `plugin_context.get_setting` and all six tunable numbers through + `retrieval_surfaces.get_setting`. Patching only the first leaves the numbers + talking to a real database — which in a unit job is a connection error, and + in an integration job is worse: the test would pass or fail on whatever the + instance happened to have stored. + """ + from scribe.services import plugin_context as pc + from scribe.services import retrieval_surfaces as rs + + def _side(uid, k, d=""): + return stored.get(k, d) + + with patch.object(pc, "get_setting", AsyncMock(side_effect=_side)), \ + patch.object(rs, "get_setting", AsyncMock(side_effect=_side)): + yield + PLUGIN = Path(__file__).resolve().parents[1] / "plugin" HOOK = PLUGIN / "hooks" / "scribe_prior_art.sh" @@ -31,9 +55,7 @@ def _cfg(**over): # missing key raises inside its fail-open except and turns the arm into a # silent no-op — which is indistinguishable from it working and finding # nothing. - base = {"enabled": True, "threshold": 0.68, "top_k": 3, "rule_threshold": 0.72} - base.update(over) - return base + return writepath_cfg(**over) # The semantic arm ignores payloads carrying less than WRITEPATH_MIN_CODE_CHARS @@ -423,11 +445,10 @@ async def test_sync_surfacing_is_measured_under_its_own_usage_source(): @pytest.mark.asyncio -async def test_config_has_its_own_switch_and_threshold_but_shares_top_k(): +async def test_config_has_its_own_switch_threshold_and_inherited_budget(): from scribe.services import plugin_context as pc stored = {pc.WRITEPATH_ENABLED_KEY: "false"} - with patch.object(pc, "get_setting", - AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))): + with _stored_settings(stored): cfg = await pc.get_writepath_config(1) # Its own switch is off while auto-inject stays on... assert cfg["enabled"] is False @@ -436,8 +457,12 @@ async def test_config_has_its_own_switch_and_threshold_but_shares_top_k(): # made unrelated code — including `x = 1` at 0.58 — clear the bar. assert cfg["threshold"] == pc.WRITEPATH_DEFAULT_THRESHOLD assert cfg["threshold"] > pc.AUTOINJECT_DEFAULT_THRESHOLD - # ...and top_k is still shared: "how many titles at once" means the same - # thing on both surfaces. + # ...and the budget is INHERITED rather than shared (#4102). This arm has + # its own key now, because it fires before every Write and Edit while + # auto-inject fires once a turn, so the same number buys very different + # amounts of attention. Unset, it still reads auto-inject's — which is what + # stops the split from silently resetting an install that had tuned the + # knob when it was shared. assert cfg["top_k"] == pc.AUTOINJECT_DEFAULT_TOP_K @@ -449,8 +474,7 @@ async def test_writepath_threshold_is_operator_tunable_and_clamped(): async def _cfg_with(raw): stored = {pc.WRITEPATH_THRESHOLD_KEY: raw} - with patch.object(pc, "get_setting", - AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))): + with _stored_settings(stored): return await pc.get_writepath_config(1) assert (await _cfg_with("0.9"))["threshold"] == 0.9 @@ -474,8 +498,7 @@ async def test_the_rule_arm_has_its_own_tunable_bar(): async def _cfg_with(raw): stored = {pc.RULEHINT_THRESHOLD_KEY: raw} - with patch.object(pc, "get_setting", - AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))): + with _stored_settings(stored): return await pc.get_writepath_config(1) assert (await _cfg_with("0.8"))["rule_threshold"] == 0.8 @@ -494,8 +517,7 @@ async def test_the_two_write_path_bars_are_independent(): from scribe.services import plugin_context as pc stored = {pc.WRITEPATH_THRESHOLD_KEY: "0.90", pc.RULEHINT_THRESHOLD_KEY: "0.61"} - with patch.object(pc, "get_setting", - AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))): + with _stored_settings(stored): cfg = await pc.get_writepath_config(1) assert cfg["threshold"] == 0.90 @@ -516,8 +538,7 @@ async def test_the_two_act_arms_read_independent_rule_bars(): from scribe.services import plugin_context as pc stored = {pc.RULEHINT_THRESHOLD_KEY: "0.75", pc.TOOLRULE_THRESHOLD_KEY: "0.61"} - with patch.object(pc, "get_setting", - AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))): + with _stored_settings(stored): cfg = await pc.get_writepath_config(1) assert cfg["rule_threshold"] == 0.75 @@ -536,8 +557,7 @@ async def test_a_garbage_command_bar_falls_back_to_its_own_default(): from scribe.services import plugin_context as pc stored = {pc.TOOLRULE_THRESHOLD_KEY: "banana"} - with patch.object(pc, "get_setting", - AsyncMock(side_effect=lambda uid, k, d: stored.get(k, d))): + with _stored_settings(stored): cfg = await pc.get_writepath_config(1) assert cfg["tool_rule_threshold"] == pc.TOOLRULE_DEFAULT_THRESHOLD -- 2.54.0 From 003bfd7a0af341180a9969fb0f7248b2804344ee Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 11:20:47 -0400 Subject: [PATCH 07/12] fix(tests): two readers moved, and the settings guard now checks the registry (#4102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI on 09b4845 caught both, and the second is an improvement rather than a repair. `test_services_reply_preferences` patched `reply_preferences.get_setting`, which the registry refactor removed — its floor and budget are resolved through `retrieval_surfaces` now. Its key-independence test also asserted the arm asks for exactly one key; it asks for two, because both of its numbers are its own since this step, so the assertion names both and adds a check that the registry and the module constant still agree about the floor key. They writing different keys is the failure where the Settings form saves one string and the arm reads another. `test_settings_defaults_agree` parsed `plugin_context.py` for a bare module-level float, and those constants now alias the registry. Rather than teach the regex about aliases, the six retrieval floors are keyed on their SURFACE NAME and read from the registry directly — which is strictly better for this guard: a surface name is also its telemetry source, so a row names the same arm the readout does, and a floor cannot be checked against a stale constant that happened to keep its old value. `PLAN_MATCH_DEFAULT_THRESHOLD` is not a push surface and keeps the older shape, with a note saying why. Added while there: `test_every_tunable_surface_has_a_control`, derived from the registry, so a seventh surface arrives as a failing test rather than as a number only the model can reach (rules 25, 27). Also lands the audit trail this step needs — `retrieval_tuning_events` (model + migration 0103) and `services/retrieval_tuning.py`. The tool layer on top is the next commit; the table is here because `reason` being REQUIRED is the whole guardrail, and the schema is where that starts. A number moved silently leaves nothing for the operator to review or disagree with, and the operator's decision is that the model moves these "9 times out of 10". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- .../versions/0103_retrieval_tuning_events.py | 75 +++++++ src/scribe/models/__init__.py | 1 + src/scribe/models/retrieval_tuning.py | 97 ++++++++ src/scribe/services/retrieval_tuning.py | 208 ++++++++++++++++++ tests/test_services_reply_preferences.py | 23 +- tests/test_settings_defaults_agree.py | 88 ++++++-- 6 files changed, 463 insertions(+), 29 deletions(-) create mode 100644 alembic/versions/0103_retrieval_tuning_events.py create mode 100644 src/scribe/models/retrieval_tuning.py create mode 100644 src/scribe/services/retrieval_tuning.py diff --git a/alembic/versions/0103_retrieval_tuning_events.py b/alembic/versions/0103_retrieval_tuning_events.py new file mode 100644 index 0000000..7dc69ac --- /dev/null +++ b/alembic/versions/0103_retrieval_tuning_events.py @@ -0,0 +1,75 @@ +"""retrieval_tuning_events — why a floor is where it is (#4102) + +Revision ID: 0103 +Revises: 0102 +Create Date: 2026-09-17 + +Milestone 416 stops shipping similarity thresholds as values somebody has to +defend, and hands the adjustment to the model that reads the surface's own +telemetry. The operator's decision: + + "the floor should be chosen and adjusted by the model using it… the user + should be able to touch it but the model should be the thing handling it 9 + times out of 10." + +The number itself already has a home — the generic settings table. What has no +home is the ARGUMENT, and once the values move on their own the argument is the +part an operator needs: what changed, from what to what, who moved it, and on +what evidence. This table is that trail, and it is what makes the delegation +reviewable rather than merely automatic. + +Nothing is backfilled. A surface with no rows here is sitting on its shipped +starting point, which is a true and useful thing for the history to say. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0103" +down_revision = "0102" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "retrieval_tuning_events", + sa.Column("id", sa.Integer(), primary_key=True), + sa.Column( + "created_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + # FK-free, like retrieval_logs and app_logs: the record of why a number + # is where it is must outlive the account that moved it. + sa.Column("user_id", sa.Integer(), nullable=True), + # Also the surface's `retrieval_logs.source`, so a change can be read + # next to what the change did. + sa.Column("surface", sa.Text(), nullable=False), + sa.Column("dial", sa.Text(), nullable=False), + # Nullable: the first change to a surface has no stored predecessor. It + # moved off the shipped starting point, which is a different event from + # moving off a value somebody chose. + sa.Column("old_value", sa.Float(), nullable=True), + sa.Column("new_value", sa.Float(), nullable=False), + sa.Column( + "actor", sa.Text(), nullable=False, server_default=sa.text("'model'") + ), + # Non-null here; non-BLANK is enforced at the service boundary, because + # a column that merely forbids NULL is satisfied by "" and a required + # field that accepts "" is a formality. + sa.Column("reason", sa.Text(), nullable=False), + ) + # The only read this table has: one surface's history, newest first. + op.create_index( + "ix_retrieval_tuning_surface_created", + "retrieval_tuning_events", + ["surface", sa.text("created_at DESC")], + ) + + +def downgrade() -> None: + op.drop_index( + "ix_retrieval_tuning_surface_created", table_name="retrieval_tuning_events" + ) + op.drop_table("retrieval_tuning_events") diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index f522011..aa91587 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -27,6 +27,7 @@ from scribe.models.password_reset import PasswordResetToken # noqa: E402, F401 from scribe.models.invitation import InvitationToken # noqa: E402, F401 from scribe.models.embedding import MilestoneEmbedding, NoteEmbedding, RuleEmbedding # noqa: E402, F401 from scribe.models.retrieval_log import RetrievalLog # noqa: E402, F401 +from scribe.models.retrieval_tuning import RetrievalTuningEvent # noqa: E402, F401 from scribe.models.note_usage import NoteUsageEvent # noqa: E402, F401 from scribe.models.rule_usage import RuleUsageEvent # noqa: E402, F401 from scribe.models.project import Project # noqa: E402, F401 diff --git a/src/scribe/models/retrieval_tuning.py b/src/scribe/models/retrieval_tuning.py new file mode 100644 index 0000000..b08d2c7 --- /dev/null +++ b/src/scribe/models/retrieval_tuning.py @@ -0,0 +1,97 @@ +from datetime import datetime, timezone + +from sqlalchemy import DateTime, Float, Index, Integer, Text +from sqlalchemy.orm import Mapped, mapped_column + +from scribe.models import Base + + +class RetrievalTuningEvent(Base): + """One row per change to a retrieval surface's floor or budget (#4102). + + WHY A TABLE AND NOT JUST THE SETTING + + The setting holds the number. It cannot hold the argument, and from this + step onward the argument is the interesting part: the operator's decision is + that the model moves these values, *"9 times out of 10"*, without being + asked each time. Delegation like that is only safe if it leaves a trail the + operator can read afterwards — what changed, from what to what, and on what + evidence — and can disagree with. + + So `reason` is not decoration and not optional. It is the guardrail: a model + that has to state why is a model that has to have looked, and a reason that + turns out to be wrong is a reason someone can point at. A bare number in a + settings row records that something moved and nothing about whether it + should have. + + WHY `actor` IS NOT JUST `user_id` + + Both a person and a model act as the same user — the MCP tools run under the + operator's own id — so the id cannot distinguish them. That distinction is + exactly what an operator scanning this table wants first: "did I do this, or + did the session?" A change the operator made themselves needs no review; one + made on their behalf might. + + FK-FREE ON user_id, like `RetrievalLog` and `AppLog` above it: the history of + how a surface was tuned should outlive the account that tuned it, and a + deleted user must not cascade away the record of why a number is where it is. + """ + + __tablename__ = "retrieval_tuning_events" + + id: Mapped[int] = mapped_column(primary_key=True) + # Declared here rather than via CreatedAtMixin because the index below + # orders on `created_at.desc()`, which needs the column object in this class + # body — a mixin's column is not in scope there. + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) + ) + user_id: Mapped[int | None] = mapped_column(Integer, nullable=True) + + # The surface's registry name, which is also its `retrieval_logs.source`. + # That identity is what lets a reader put a change next to what the change + # did; `tests/test_retrieval_surfaces.py` asserts it rather than trusting it. + surface: Mapped[str] = mapped_column(Text, nullable=False) + + # "floor" or "budget". Text rather than an enum because rule 36 makes a new + # CHECK value a migration, and this is a young surface that may well grow a + # third tunable — a constraint bought nothing here except a future migration. + dial: Mapped[str] = mapped_column(Text, nullable=False) + + # Both stored as float so one pair of columns serves both dials; a budget is + # a whole number and reads back as one. `old_value` is nullable because the + # first change to a surface has no stored predecessor — it moved off the + # shipped starting point, which is a different event from moving off a value + # somebody chose, and worth being able to tell apart. + old_value: Mapped[float | None] = mapped_column(Float, nullable=True) + new_value: Mapped[float] = mapped_column(Float, nullable=False) + + # "model" | "human". See the class docstring: the user id cannot tell these + # apart, and the difference is the first thing a reviewer wants. + actor: Mapped[str] = mapped_column(Text, nullable=False, default="model") + + # REQUIRED at the service boundary, not merely non-null here. A column that + # is technically non-null is satisfied by "", which is how a required field + # becomes a formality; the service refuses a blank one. + reason: Mapped[str] = mapped_column(Text, nullable=False) + + __table_args__ = ( + # The only read this table has: one surface's history, newest first. + Index( + "ix_retrieval_tuning_surface_created", + "surface", + created_at.desc(), + ), + ) + + def to_dict(self) -> dict: + return { + "id": self.id, + "created_at": self.created_at.isoformat() if self.created_at else None, + "surface": self.surface, + "dial": self.dial, + "old_value": self.old_value, + "new_value": self.new_value, + "actor": self.actor, + "reason": self.reason, + } diff --git a/src/scribe/services/retrieval_tuning.py b/src/scribe/services/retrieval_tuning.py new file mode 100644 index 0000000..5e92033 --- /dev/null +++ b/src/scribe/services/retrieval_tuning.py @@ -0,0 +1,208 @@ +"""Moving a retrieval surface's floor or budget, with the argument attached (#4102). + +WHY THIS EXISTS + +The operator's decision for milestone 416 step 4: + + "the floor should be chosen and adjusted by the model using it. we've come + back to something either fails or has to be looked at by the user we need a + model consistent surface for the adjustment of these floor values. the user + should be able to touch it but the model should be the thing handling it 9 + times out of 10." + +`retrieval_surfaces` made the six arms describe their numbers the same way. +This module is the write half: one call that moves one dial on one surface, +records what it was, what it became, who moved it and why, and refuses to do +any of that without a reason. + +WHY A REASON IS REQUIRED + +Because the alternative was tried and measured. The milestone originally listed +self-tuning as a non-goal on the strength of one case: `report_preference` +logged 69 consecutive declines with the refused record 0.0006 under the bar, and +every percentile in the readout said "lower it". Reading the refused record +showed it was rule 77 "Extract intent from loose phrasing" — a false positive — +so lowering the bar would have attached that rule to every completion report +ever written. The statistic and the correct action pointed in opposite +directions. + +What separated them was opening the record. A required `reason` is the cheapest +mechanism that makes that step happen: a caller who must write down why has to +have looked, and a caller who writes down something wrong has left the operator +a sentence to disagree with. A number moved silently leaves nothing. + +WHAT THIS DELIBERATELY DOES NOT DO + +It does not decide anything itself. There is no rule here that reads a +percentile and picks a value, and that absence is the design — the non-goal that +survived is *statistical* auto-tuning, precisely because the statistic was the +thing that was wrong. The judgement stays with the reader; this module only +makes the judgement recordable and reversible. +""" +from __future__ import annotations + +import logging + +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.retrieval_tuning import RetrievalTuningEvent +from scribe.services.retrieval_surfaces import ( + MAX_BUDGET, + budget_for, + floor_for, + get_surface, + surface_names, +) +from scribe.services.settings import set_setting + +logger = logging.getLogger(__name__) + +DIALS = ("floor", "budget") + +# Long enough to say what was read and what it showed; short enough that nobody +# pastes a telemetry dump in. The number is not a measurement — it is the point +# at which "0.66" stops being an acceptable answer to "why". +_MIN_REASON_CHARS = 20 + + +def _clean_reason(reason: str) -> str: + """The guardrail, enforced here rather than by the column. + + `reason` is NOT NULL in the schema, which "" satisfies. A required field + that accepts an empty string is a formality, and this one is the whole + mechanism — see the module docstring. + """ + text = (reason or "").strip() + if len(text) < _MIN_REASON_CHARS: + raise ValueError( + "reason is required, and has to say what you read. A floor moved " + "without a stated basis is a number nobody can review or revert. " + "Name the evidence: which surface's telemetry, and what the refused " + "records actually were — `retrieval_telemetry(near_miss_samples=N)` " + "returns them by id, and reading them is the step that separates a " + "real miss from a bar doing its job." + ) + return text + + +async def current_settings(user_id: int) -> list[dict]: + """Every tunable surface with its live pair and its last stated reason. + + The read side of the tuning surface, and shaped for a reader who is about to + change something: the value, what the arm asks and over what corpus and how + often — because a floor cannot be moved sensibly without those three — and + the reason last given, so the next change argues with the last one instead + of overwriting it blind. + """ + out: list[dict] = [] + async with async_session() as session: + for name in surface_names(): + s = get_surface(name) + rows = ( + await session.execute( + select(RetrievalTuningEvent) + .where( + RetrievalTuningEvent.surface == name, + RetrievalTuningEvent.user_id == user_id, + ) + .order_by(RetrievalTuningEvent.created_at.desc()) + .limit(len(DIALS)) + ) + ).scalars().all() + last = {r.dial: r for r in rows} + out.append({ + "surface": name, + "floor": await floor_for(user_id, name), + "budget": await budget_for(user_id, name), + "floor_default": s.floor_default, + "budget_default": s.budget_default, + "asks": s.asks, + "over": s.over, + "fires": s.fires, + # Absent rather than empty when a surface has never been moved: + # "still on the shipped starting point" is a real state and + # should not render as a blank reason somebody wrote. + "last_change": { + dial: last[dial].to_dict() for dial in DIALS if dial in last + }, + }) + return out + + +async def set_dial( + user_id: int, + surface: str, + dial: str, + value: float, + *, + reason: str, + actor: str = "model", +) -> dict: + """Move one dial on one surface, recording the change and its argument. + + Returns the applied value alongside the previous one, so a caller can see + that a clamp bit rather than assuming the number it sent is the number in + force — the failure being avoided is a tool reporting success for a value + the registry silently corrected. + """ + s = get_surface(surface) # refuses an unknown name + if dial not in DIALS: + raise ValueError(f"dial must be one of {DIALS}, got {dial!r}") + if actor not in ("model", "human"): + raise ValueError(f"actor must be 'model' or 'human', got {actor!r}") + text = _clean_reason(reason) + + if dial == "floor": + old = await floor_for(user_id, surface) + applied = min(1.0, max(0.0, float(value))) + key, stored = s.floor_key, str(applied) + else: + old = float(await budget_for(user_id, surface)) + applied = float(min(MAX_BUDGET, max(1, int(float(value))))) + key, stored = s.budget_key, str(int(applied)) + + await set_setting(user_id, key, stored) + async with async_session() as session: + session.add(RetrievalTuningEvent( + user_id=user_id, surface=surface, dial=dial, + old_value=old, new_value=applied, actor=actor, reason=text, + )) + await session.commit() + + return { + "surface": surface, + "dial": dial, + "previous": old, + "applied": applied, + # True when the registry corrected what was asked for. Said out loud + # because a caller that believes it set 1.4 will read the next + # telemetry as evidence about a bar that was never in force. + "clamped": abs(applied - float(value)) > 1e-9, + "reason": text, + "actor": actor, + } + + +async def tuning_history( + user_id: int, *, surface: str | None = None, limit: int = 20 +) -> list[dict]: + """What has been moved, newest first — the operator's review surface. + + Scoped to one surface when asked, because the question is almost always + "why is THIS arm set like this", and an unscoped list buries one surface's + two changes under another's twenty. + """ + if surface is not None: + get_surface(surface) # refuse a typo on the read too + async with async_session() as session: + q = ( + select(RetrievalTuningEvent) + .where(RetrievalTuningEvent.user_id == user_id) + .order_by(RetrievalTuningEvent.created_at.desc()) + .limit(max(1, min(int(limit), 200))) + ) + if surface is not None: + q = q.where(RetrievalTuningEvent.surface == surface) + rows = (await session.execute(q)).scalars().all() + return [r.to_dict() for r in rows] diff --git a/tests/test_services_reply_preferences.py b/tests/test_services_reply_preferences.py index 185233b..d742b2c 100644 --- a/tests/test_services_reply_preferences.py +++ b/tests/test_services_reply_preferences.py @@ -9,6 +9,9 @@ import re from unittest.mock import AsyncMock, MagicMock, patch M = "scribe.services.reply_preferences" +# Its two numbers are resolved through the registry now (#4102), so the +# settings reader to patch lives there rather than in the arm's own module. +RS = "scribe.services.retrieval_surfaces" def _rule(rid, kind="preference"): @@ -26,7 +29,7 @@ async def _run(hits, *, searched=True, raises=None): search_mock = AsyncMock(side_effect=search) with patch(f"{M}.semantic_search_rules", search_mock), \ - patch(f"{M}.get_setting", AsyncMock(return_value="0.72")), \ + patch(f"{RS}.get_setting", AsyncMock(return_value="0.72")), \ patch(f"{M}.record_retrieval") as logged, \ patch(f"{M}.record_rule_surfaced") as surfaced: out = await completion_preferences(7, project_id=3) @@ -93,7 +96,7 @@ def test_its_bar_is_its_own_key_not_the_prompt_arm_s(monkeypatch): asked: list[str] = [] - async def get_setting(user_id, key, default): + async def get_setting(user_id, key, default=""): asked.append(key) return default @@ -101,16 +104,26 @@ def test_its_bar_is_its_own_key_not_the_prompt_arm_s(monkeypatch): kw["report"].update({"searched": True}) return [] - with patch(f"{M}.get_setting", AsyncMock(side_effect=get_setting)), \ + with patch(f"{RS}.get_setting", AsyncMock(side_effect=get_setting)), \ patch(f"{M}.semantic_search_rules", AsyncMock(side_effect=search)), \ patch(f"{M}.record_retrieval"): asyncio.run(completion_preferences(7)) - assert asked == [plugin_context.REPORTPREF_THRESHOLD_KEY] - assert plugin_context.REPORTPREF_THRESHOLD_KEY != plugin_context.PROMPTRULE_THRESHOLD_KEY, ( + # Both numbers are its own since #4102 — a floor AND a budget — so the arm + # asks for two keys, and neither may be the prompt arm's. + from scribe.services.retrieval_surfaces import get_surface + + mine = get_surface("report_preference") + assert asked == [mine.floor_key, mine.budget_key] + theirs = get_surface("prompt_rule") + assert mine.floor_key != theirs.floor_key, ( "the two keys are the same string again, so the settings form has one " "dial driving two arms — which is the defect, whatever the value is" ) + assert mine.floor_key == plugin_context.REPORTPREF_THRESHOLD_KEY, ( + "the registry and the module constant disagree about this arm's key, " + "so the Settings form would write one and the arm would read the other" + ) def test_the_query_assumes_no_particular_domain(): diff --git a/tests/test_settings_defaults_agree.py b/tests/test_settings_defaults_agree.py index 390abf7..17166c6 100644 --- a/tests/test_settings_defaults_agree.py +++ b/tests/test_settings_defaults_agree.py @@ -20,9 +20,12 @@ string equals the Python default. Retuning either stays free as long as both move — which is the point, because these are tuning values and #3853 moved one of them the day this was written. -Both sides are read from SOURCE rather than imported. The Vue file cannot be -imported at all, and reading the Python constant through an import would tie -this to module-load side effects it has no interest in. +The Vue side is read from SOURCE, because that file cannot be imported at all. +The Python side is read from source for a loose constant and IMPORTED for a +registry surface — the registry is a plain table of frozen dataclasses with +nothing to trigger on import, and importing it means this guard checks the value +the server will actually resolve rather than a literal that happens to look +right. It cannot check that the form WRITES the right key — that is behaviour, and the keys are asserted where they are built. It catches the drift that has no @@ -39,20 +42,29 @@ ROOT = pathlib.Path(__file__).resolve().parents[1] _SERVICES = ROOT / "src" / "scribe" / "services" _VUE = ROOT / "frontend" / "src" / "views" / "SettingsView.vue" -# (services module, python constant, vue ref). Hand-written because the -# pairing is an editorial fact — the names do not share a convention either -# side could derive — but every entry is asserted to EXIST on both sides, so a -# rename fails loudly here rather than silently dropping that threshold from -# the check. -_PAIRS = ( - ("plugin_context.py", "AUTOINJECT_DEFAULT_THRESHOLD", "kbInjectThreshold"), - ("plugin_context.py", "WRITEPATH_DEFAULT_THRESHOLD", "kbWritePathThreshold"), - ("plugin_context.py", "RULEHINT_DEFAULT_THRESHOLD", "kbRuleHintThreshold"), - ("plugin_context.py", "TOOLRULE_DEFAULT_THRESHOLD", "kbToolRuleThreshold"), - ("plugin_context.py", "PROMPTRULE_DEFAULT_THRESHOLD", "kbPromptRuleThreshold"), - ("plugin_context.py", "REPORTPREF_DEFAULT_THRESHOLD", "kbReportPrefThreshold"), +# THE SIX RETRIEVAL FLOORS now live in one registry (#4102), so their side of +# the pairing is a SURFACE NAME rather than a module constant. That is strictly +# better for this guard: the surface name is also the telemetry source, so a row +# here names the same arm the readout does, and a floor that moved cannot be +# checked against a stale constant that happened to keep its old value. +_SURFACE_PAIRS = ( + ("auto_inject", "kbInjectThreshold"), + ("write_path", "kbWritePathThreshold"), + ("write_path_rule", "kbRuleHintThreshold"), + ("pre_tool_rule", "kbToolRuleThreshold"), + ("prompt_rule", "kbPromptRuleThreshold"), + ("report_preference", "kbReportPrefThreshold"), +) + +# (services module, python constant, vue ref) for the defaults that are NOT +# retrieval surfaces. Hand-written because the pairing is an editorial fact — +# the names share no convention either side could derive — and asserted to +# exist on both sides, so a rename fails loudly rather than silently dropping +# that threshold from the check. +_CONSTANT_PAIRS = ( # The plan gate (milestone 415): it blocks a create, so a form showing a - # looser bar than the one in force would be the more misleading drift. + # looser bar than the one in force would be the more misleading drift. Not + # a push surface, so it has no registry entry and keeps the older shape. ("dedup.py", "PLAN_MATCH_DEFAULT_THRESHOLD", "kbPlanMatchThreshold"), ) @@ -80,14 +92,42 @@ def _vue_default(ref_name: str) -> float: return float(m.group(1)) -@pytest.mark.parametrize(("module", "constant", "ref_name"), _PAIRS, - ids=[p[1] for p in _PAIRS]) -def test_the_form_shows_the_default_the_server_uses(module, constant, ref_name): - """An untouched control must render the bar actually in force.""" - server, form = _python_default(module, constant), _vue_default(ref_name) +def _assert_agrees(server: float, form: float, ref_name: str, what: str) -> None: assert form == server, ( f"SettingsView shows {form} for {ref_name} while the server defaults " - f"to {server} ({constant}). An operator who has never set this reads " - f"the form as the value in force, so the two must move together — " - f"retune both, or neither." + f"to {server} ({what}). An operator who has never set this reads the " + f"form as the value in force, so the two must move together — retune " + f"both, or neither." ) + + +@pytest.mark.parametrize(("surface", "ref_name"), _SURFACE_PAIRS, + ids=[p[0] for p in _SURFACE_PAIRS]) +def test_the_form_shows_the_floor_the_surface_uses(surface, ref_name): + """An untouched control must render the bar actually in force.""" + from scribe.services.retrieval_surfaces import get_surface + + _assert_agrees(get_surface(surface).floor_default, _vue_default(ref_name), + ref_name, f"{surface} floor") + + +def test_every_tunable_surface_has_a_control(): + """Rule 25/27: a number an operator may need different has a UI or it is + not shipped. Derived from the registry, so a seventh surface arrives here + as a failure rather than as a setting only the model can reach.""" + from scribe.services.retrieval_surfaces import surface_names + + covered = {s for s, _ref in _SURFACE_PAIRS} + missing = [n for n in surface_names() if n not in covered] + assert not missing, ( + f"these surfaces are tunable with no Settings control: {missing}. " + "Add the control and its row, or say in _SURFACE_PAIRS why it has none." + ) + + +@pytest.mark.parametrize(("module", "constant", "ref_name"), _CONSTANT_PAIRS, + ids=[p[1] for p in _CONSTANT_PAIRS]) +def test_the_form_shows_the_default_the_server_uses(module, constant, ref_name): + """Same claim, for the defaults that are not retrieval surfaces.""" + _assert_agrees(_python_default(module, constant), _vue_default(ref_name), + ref_name, constant) -- 2.54.0 From ca49a46c2350989a222500d559b0df56e8d0ce2d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 11:28:17 -0400 Subject: [PATCH 08/12] feat(retrieval): the model moves its own floors, and says why (#4102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone 416 step 4's write half. `retrieval_surfaces.py` made the six push arms describe their `{floor, budget}` the same way; this adds the three MCP tools that let the model READ that and change it, and the backup sections that carry the reasons. The operator's decision, which this implements: "the floor should be chosen and adjusted by the model using it… the user should be able to touch it but the model should be the thing handling it 9 times out of 10." WHY A REASON IS REQUIRED, AND WHY THE TOOL ARGUES AGAINST PERCENTILES The milestone originally listed self-tuning as a non-goal on one measured case, and that case is now the tool's docstring rather than a prohibition: `report_preference` logged 69 consecutive declines with the refused record 0.0006 under the bar, and every percentile said "lower it". The refused record was rule 77 "Extract intent from loose phrasing" matched against a query about report layout — a false positive. Lowering would have attached that rule to every completion report ever written. What separated the statistic from the correct action was OPENING the record. So `tune_retrieval` refuses a blank or perfunctory reason, tells the caller to read `retrieval_telemetry(near_miss_samples=5)` and the record ids it names, and carries that 69-decline example — an abstract warning loses to a number. The non-goal that survives is *statistical* auto-tuning; nothing here reads a percentile and picks a value. BACKUP (v16), which is what CI caught `retrieval_tuning_events` was neither backed up nor excluded, and #2293's guard said so. It is backed up: `settings` already carried the numbers, so dropping this would restore an install with six moved dials and no argument for any of them — precisely the state the table exists to prevent, and worse now that the model is the one moving them. One `_retrieval_tuning_event_rows` builder called from both exporters (snippet #2851); `user_id` travels because a restore has to remap it, which is why the row builder is not the model's `to_dict()`. `surface` is a registry name rather than a foreign key, so the history survives a restore into an install whose ids all differ. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/__init__.py | 5 +- src/scribe/mcp/tools/retrieval_tuning.py | 115 +++++++++++++ src/scribe/services/backup.py | 84 +++++++++- tests/test_retrieval_tuning.py | 199 +++++++++++++++++++++++ tests/test_services_backup.py | 10 +- 5 files changed, 408 insertions(+), 5 deletions(-) create mode 100644 src/scribe/mcp/tools/retrieval_tuning.py create mode 100644 tests/test_retrieval_tuning.py diff --git a/src/scribe/mcp/tools/__init__.py b/src/scribe/mcp/tools/__init__.py index e8b0b1f..f292d41 100644 --- a/src/scribe/mcp/tools/__init__.py +++ b/src/scribe/mcp/tools/__init__.py @@ -5,14 +5,15 @@ to a FastMCP instance. `register_all(mcp)` is the single entry point called from `mcp.server.build_mcp_server`. """ from scribe.mcp.tools import ( - design_systems, milestones, notes, processes, projects, recent, repos, rulebooks, search, shapes, - snippets, systems, tags, tasks, trash, + design_systems, milestones, notes, processes, projects, recent, repos, retrieval_tuning, + rulebooks, search, shapes, snippets, systems, tags, tasks, trash, ) def register_all(mcp) -> None: """Register every tool module's tools on the given FastMCP instance.""" search.register(mcp) + retrieval_tuning.register(mcp) notes.register(mcp) tasks.register(mcp) projects.register(mcp) diff --git a/src/scribe/mcp/tools/retrieval_tuning.py b/src/scribe/mcp/tools/retrieval_tuning.py new file mode 100644 index 0000000..f4594e0 --- /dev/null +++ b/src/scribe/mcp/tools/retrieval_tuning.py @@ -0,0 +1,115 @@ +"""Reading and moving a retrieval surface's floor and budget (#4102). + +The write half of the loop `retrieval_telemetry` opens. That tool says what a +bar did; these say what the bar IS, and let it be changed with the argument +attached. +""" +from scribe.mcp._context import current_user_id +from scribe.services import retrieval_tuning as tuning_svc + + +async def retrieval_surfaces() -> dict: + """Every retrieval surface Scribe pushes on, with its floor and its budget. + + Read this before changing either, and read it beside + `retrieval_telemetry(days=…, near_miss_samples=5)` — this tool says what is + in force, that one says what it did. + + Each surface carries what it ASKS, what corpus it asks OVER, and how often + it FIRES, because a floor cannot be moved sensibly without all three. An arm + firing before every Bash call is spending attention on a scale an arm firing + once a turn is not, and the same number means different things to a query + that is a shell command, a code payload, or an operator's sentence. + + `floor` is a COST floor: is this worth ranking at all. It is not a relevance + judgement — relevance is decided by the reader, which is the only + participant that can read a record's trigger against the actual situation, + and every injected line says so out loud ("read it before deciding it does + not apply"). + + `budget` is what actually binds. It is how many lines this surface may spend + on one injection, and it is the control to reach for when a surface feels + noisy — lowering a budget removes the weakest candidates, while raising a + floor removes whichever candidates happen to sit under a number. + + `last_change` carries the reason last given for each dial, or is absent when + the surface is still on its shipped starting point. Those starting points + are starting points: they were measured against one corpus with one + embedding model and cannot be right for another install by construction. + That is why this tool exists rather than a better set of defaults. + """ + return {"surfaces": await tuning_svc.current_settings(current_user_id())} + + +async def tune_retrieval( + surface: str, dial: str, value: float, reason: str, actor: str = "model", +) -> dict: + """Move one surface's floor or budget, recording why. + + DO NOT CALL THIS FROM A PERCENTILE ALONE. `retrieval_telemetry` gives + `near_misses.p90` — the mass sitting just under the bar — and that number + says nothing about whether the mass is RELEVANT. The two have been measured + disagreeing: one surface logged 69 consecutive declines with the refused + record 0.0006 under its bar, and every percentile said "lower it". The + refused record turned out to be a rule about interpreting a REQUEST, matched + against a query about report layout — a false positive. Lowering would have + attached that rule to every completion report ever written. The statistic + and the correct action pointed in opposite directions, and only opening the + record could tell. + + So the procedure is: `retrieval_telemetry(near_miss_samples=5)`, then + `get_rule` / `get_note` the `record_id`s it names, and decide whether those + records SHOULD have surfaced for those queries. Then move the dial, and say + in `reason` what you read and what it showed. + + `reason` is required and must be substantive. It is the guardrail, not + bookkeeping: it is what lets the operator review a change they did not make, + disagree with it, and revert it. A number that moved with no stated basis is + one nobody can audit — including you, next week. + + WHICH DIAL. Reach for `budget` when the complaint is volume, and `floor` + when the complaint is quality. Raising a floor to quieten a surface throws + away its best candidates along with its worst, because a floor cannot tell + rank from relevance; lowering a budget keeps the top of the ranking and + drops the tail, which is usually what was wanted. + + Args: + surface: the surface name from `retrieval_surfaces` — also its + `retrieval_telemetry` source, so the two always name the same arm. + dial: "floor" or "budget". + value: the new value. A floor is clamped to [0, 1], a budget to a whole + number in [1, 10]. The result says whether a clamp bit, which + matters: believing you set a value the registry corrected means + reading the next telemetry as evidence about a bar never in force. + reason: what you read and what it showed. Required. + actor: "model" (default) or "human" — pass "human" only when relaying a + value the operator chose themselves, so the audit trail can tell + a change they made from one made on their behalf. + """ + return await tuning_svc.set_dial( + current_user_id(), surface, dial, value, reason=reason, actor=actor, + ) + + +async def retrieval_tuning_history(surface: str = "", limit: int = 20) -> dict: + """What has been changed about retrieval on this install, newest first. + + Reach for it before moving a dial that has been moved before — the previous + reason is the argument the next change has to answer, and a surface that has + been walked up and down repeatedly is evidence the floor is not the problem. + + Args: + surface: restrict to one surface; omit for everything. + limit: how many events, default 20, capped at 200. + """ + return { + "events": await tuning_svc.tuning_history( + current_user_id(), surface=surface or None, limit=limit, + ) + } + + +def register(mcp) -> None: + mcp.tool(name="retrieval_surfaces")(retrieval_surfaces) + mcp.tool(name="tune_retrieval")(tune_retrieval) + mcp.tool(name="retrieval_tuning_history")(retrieval_tuning_history) diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 1ad21d5..5a9d135 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -13,6 +13,7 @@ from scribe.models.rule_version import RuleVersion from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.note_usage import NoteUsageEvent from scribe.models.rule_usage import RuleUsageEvent +from scribe.models.retrieval_tuning import RetrievalTuningEvent from scribe.models.canonical_system import CanonicalSystem from scribe.models.rulebook import RuleRelation, rule_systems as rule_systems_t from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse @@ -67,8 +68,14 @@ logger = logging.getLogger(__name__) # topic_suppressions with their tables (milestone 414): a rule's scope is its # home now. Older archives carrying those sections still restore — the keys are # simply not read — as do the subscribe_rulebooks inception choices they hold. +# v16 (2026-09) added retrieval_tuning_events (milestone 416): the REASON each +# retrieval floor and budget is where it is. `settings` already carried the +# numbers, so leaving this behind would restore six moved dials with the +# argument for them silently dropped — and from this step on those dials are +# moved by the model, which is exactly the case where the operator needs the +# argument to review. # Bump when the serialized schema changes. -BACKUP_VERSION = 15 +BACKUP_VERSION = 16 # Every table this backup carries, by its REAL name. Paired with _NOT_INCLUDED # below, these two lists must together account for the entire schema — which is @@ -100,6 +107,12 @@ _BACKED_UP = [ # accumulated, so a restore that dropped it would silently reset the # measurement to zero while everything still looked fine. "rule_usage_events", + # v16 (2026-09): why each retrieval floor and budget is where it is + # (milestone 416). The values live in `settings` and already travelled; the + # argument for them had nowhere to go. Now that the model moves these dials + # on the operator's behalf, a restore that kept the numbers and dropped the + # reasons would leave an install tuned by nobody it can name. + "retrieval_tuning_events", ] # Tables intentionally NOT in the backup, surfaced in the payload so the gap is @@ -189,6 +202,9 @@ _COLUMN_EXCLUSIONS: dict[str, set[str]] = { "note_usage_events": {"id"}, # Same as the note twin: the surrogate key is re-issued on insert. "rule_usage_events": {"id"}, + # Same again — and everything else travels, because each remaining column + # is part of the argument: what moved, from what, to what, by whom, why. + "retrieval_tuning_events": {"id"}, "design_systems": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, "design_tokens": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, "repo_bindings": {"id", "created_at", "updated_at"}, @@ -329,6 +345,25 @@ def _rule_usage_event_rows(rows) -> list[dict]: ] +def _retrieval_tuning_event_rows(rows) -> list[dict]: + """The record of why a retrieval dial is where it is (#4102). + + Not `to_dict()`: that method serves the MCP reader, which already knows + whose install it is asking about, so it omits `user_id`. A restore has to + remap it, and a row that arrived without it would have to be dropped or + guessed at. + """ + return [ + { + "user_id": r.user_id, "surface": r.surface, "dial": r.dial, + "old_value": r.old_value, "new_value": r.new_value, + "actor": r.actor, "reason": r.reason, + "created_at": r.created_at.isoformat() if r.created_at else None, + } + for r in rows + ] + + def _code_shape_rows(rows) -> list[dict]: return [r.to_dict() for r in rows] @@ -617,6 +652,12 @@ async def export_full_backup() -> dict: rule_usage_events = ( await session.execute(select(RuleUsageEvent)) ).scalars().all() + # Oldest first, so a restored history reads in the order the dials + # actually moved — the sequence IS the argument when a surface has been + # walked up and down. + retrieval_tuning_events = (await session.execute( + select(RetrievalTuningEvent).order_by(RetrievalTuningEvent.id) + )).scalars().all() repo_bindings = (await session.execute(select(RepoBinding))).scalars().all() code_shapes = (await session.execute(select(CodeShape))).scalars().all() code_shape_events = (await session.execute( @@ -661,6 +702,9 @@ async def export_full_backup() -> dict: "design_tokens": _design_token_rows(design_tokens), "note_usage_events": _usage_event_rows(usage_events), "rule_usage_events": _rule_usage_event_rows(rule_usage_events), + "retrieval_tuning_events": _retrieval_tuning_event_rows( + retrieval_tuning_events + ), "repo_bindings": _repo_binding_rows(repo_bindings), "note_supersessions": _note_supersession_rows(supersessions), "code_shapes": _code_shape_rows(code_shapes), @@ -795,6 +839,15 @@ async def export_user_backup(user_id: int) -> dict: rule_usage_events = (await session.execute( select(RuleUsageEvent).where(RuleUsageEvent.rule_id.in_(_rule_ids)) )).scalars().all() if _rule_ids else [] + # Scoped on user_id, and here that IS the right column — unlike the + # rule usage events directly above. These record changes to this user's + # OWN retrieval settings, which is what `user_id` means on this table; + # there is no second owner to route around. + retrieval_tuning_events = (await session.execute( + select(RetrievalTuningEvent) + .where(RetrievalTuningEvent.user_id == user_id) + .order_by(RetrievalTuningEvent.id) + )).scalars().all() rule_relations = (await session.execute( select(RuleRelation).where( RuleRelation.from_rule_id.in_(_rule_ids), @@ -836,6 +889,9 @@ async def export_user_backup(user_id: int) -> dict: "design_tokens": _design_token_rows(design_tokens), "note_usage_events": _usage_event_rows(usage_events), "rule_usage_events": _rule_usage_event_rows(rule_usage_events), + "retrieval_tuning_events": _retrieval_tuning_event_rows( + retrieval_tuning_events + ), "repo_bindings": _repo_binding_rows(repo_bindings), "note_supersessions": _note_supersession_rows(supersessions), "code_shapes": _code_shape_rows(code_shapes), @@ -975,6 +1031,7 @@ async def _restore_v2(data: dict) -> dict: "note_supersessions": 0, "code_shapes": 0, "code_shape_events": 0, "code_shape_uses": 0, "canonical_systems": 0, "rule_systems": 0, "rule_relations": 0, "rule_versions": 0, + "retrieval_tuning_events": 0, } async with async_session() as session: @@ -1169,6 +1226,31 @@ async def _restore_v2(data: dict) -> dict: session.add(Setting(user_id=mapped_uid, key=s_data["key"], value=s_data.get("value", ""))) stats["settings"] += 1 + # 8b. Retrieval tuning history (v16) — restored beside the settings it + # explains, and for the same reason: the numbers above are the state, + # these rows are the argument for it. From milestone 416 those dials + # move on the operator's behalf, so an install restored with the values + # and without the reasons is one tuned by nobody it can name. + # + # No id remapping beyond the user: `surface` is a registry NAME, not a + # foreign key, which is what lets this history survive a restore into + # an install whose row ids all differ. + for t_data in data.get("retrieval_tuning_events", []): + mapped_uid = user_id_map.get(t_data.get("user_id") or 0) + if mapped_uid is None: + continue + session.add(RetrievalTuningEvent( + user_id=mapped_uid, + surface=t_data.get("surface", ""), + dial=t_data.get("dial", ""), + old_value=t_data.get("old_value"), + new_value=t_data.get("new_value"), + actor=t_data.get("actor") or "model", + reason=t_data.get("reason", ""), + created_at=_dt(t_data.get("created_at")), + )) + stats["retrieval_tuning_events"] += 1 + # 9. Rulebooks (v3) for rb_data in data.get("rulebooks", []): mapped_uid = user_id_map.get(rb_data.get("owner_user_id", 0)) diff --git a/tests/test_retrieval_tuning.py b/tests/test_retrieval_tuning.py new file mode 100644 index 0000000..733315d --- /dev/null +++ b/tests/test_retrieval_tuning.py @@ -0,0 +1,199 @@ +"""Moving a floor, and the reason that has to come with it (#4102). + +WHY THIS EXISTS + +The operator handed the dial to the model: *"the user should be able to touch it +but the model should be the thing handling it 9 times out of 10."* Everything +here guards the half of that sentence people skip — the operator still has to be +able to see what was done on their behalf and disagree with it. + +WHAT THIS PINS + + 1. **A reason is required, and "" does not count.** The column is NOT NULL, + which an empty string satisfies; the service is where the requirement is + real. This is the whole guardrail: a caller who must write down why has to + have looked, and a caller who writes down something wrong leaves the + operator a sentence to argue with. A number that moved silently leaves + nothing. + 2. **The change is recorded, with what it was before.** Without `old_value` a + history cannot answer "was this always like that", which is the first + question anyone asks of a surface behaving oddly. + 3. **A clamp is reported, never swallowed.** A caller that believes it set 1.4 + will read the next telemetry as evidence about a bar that was never in + force — the same class of error as #3739, one layer up. + 4. **An unknown surface is refused before anything is written.** Settings keys + are free-form strings, so a typo'd surface would write a key nothing reads: + a change that reports success and alters nothing. + 5. **The tool teaches the procedure that works**, not the one the numbers + suggest. Asserted on the docstring because the docstring IS the contract an + agent reads, and the failure it prevents is a model tuning from a + percentile — which has been measured pointing the wrong way. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import retrieval_tuning as rt +from tests.helpers import make_mock_session + + +def _patches(floor=0.72, budget=3): + """Patch everything `set_dial` touches except the thing under test.""" + session = make_mock_session() + return session, ( + patch.object(rt, "async_session", MagicMock(return_value=session)), + patch.object(rt, "set_setting", AsyncMock()), + patch.object(rt, "floor_for", AsyncMock(return_value=floor)), + patch.object(rt, "budget_for", AsyncMock(return_value=budget)), + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("reason", ["", " ", "noisy", "too high"]) +async def test_a_change_without_a_real_reason_is_refused(reason): + """The guardrail. "too high" is a restatement of the change, not a basis.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError) as e: + await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason=reason) + # The message has to name the tool that produces a real basis, or the + # caller's next move is a longer sentence rather than a look at the records. + assert "near_miss_samples" in str(e.value) + session.add.assert_not_called() + + +@pytest.mark.asyncio +async def test_nothing_is_written_when_the_reason_is_refused(): + """Refused BEFORE the setting is touched, not after. + + Writing the value and then raising would leave the number moved and the + history empty — the exact state this table exists to make impossible. + """ + session, ctx = _patches() + with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \ + ctx[2], ctx[3]: + with pytest.raises(ValueError): + await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason="x") + setter.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_good_change_writes_the_setting_and_the_event(): + session, ctx = _patches(floor=0.72) + reason = ("read prompt_rule's 5 highest declines: 3 were project rules for " + "another repo, so the bar is doing its job here") + with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \ + ctx[2], ctx[3]: + out = await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason=reason) + + setter.assert_awaited_once() + _uid, key, stored = setter.await_args.args + assert key == "kb_promptrule_threshold" and stored == "0.66" + + event = session.add.call_args.args[0] + assert event.surface == "prompt_rule" and event.dial == "floor" + # The before-value is what makes the history answerable. + assert event.old_value == 0.72 and event.new_value == 0.66 + assert event.reason == reason and event.actor == "model" + assert out["previous"] == 0.72 and out["applied"] == 0.66 + assert out["clamped"] is False + + +@pytest.mark.asyncio +@pytest.mark.parametrize("dial, sent, applied", [ + ("floor", 1.4, 1.0), + ("floor", -0.2, 0.0), + ("budget", 99, 10), + ("budget", 0, 1), +]) +async def test_a_clamp_is_reported_rather_than_swallowed(dial, sent, applied): + """Said out loud, because silence here poisons the next reading. + + A caller that believes it set 1.4 treats the following week's telemetry as + evidence about a bar that never existed, and then moves the dial again to + fix a problem it invented. + """ + session, ctx = _patches() + reason = "checked the refused records for this surface and they were fine" + with ctx[0], ctx[1], ctx[2], ctx[3]: + out = await rt.set_dial(1, "auto_inject", dial, sent, reason=reason) + assert out["applied"] == applied + assert out["clamped"] is True + + +@pytest.mark.asyncio +async def test_an_unknown_surface_is_refused_before_anything_is_written(): + session, ctx = _patches() + with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \ + ctx[2], ctx[3]: + with pytest.raises(ValueError): + await rt.set_dial(1, "pretool_rule", "floor", 0.6, + reason="a perfectly good reason that is long enough") + setter.assert_not_called() + session.add.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("dial", ["threshold", "limit", "", "FLOOR"]) +async def test_an_unknown_dial_is_refused(dial): + """The near-misses are the old vocabulary — `threshold` and `limit` are what + these were called before this step, so they are exactly what a stale caller + will send, and a silent no-op there would be indistinguishable from a + change that did not take.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError): + await rt.set_dial(1, "auto_inject", dial, 0.6, + reason="a perfectly good reason that is long enough") + session.add.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_human_change_is_distinguishable_from_the_models(): + """Both act as the same user, so the id cannot tell them apart — and "did I + do this, or did the session?" is the first question the history is asked.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3]: + await rt.set_dial(1, "auto_inject", "floor", 0.6, actor="human", + reason="operator set this themselves in Settings") + assert session.add.call_args.args[0].actor == "human" + + +@pytest.mark.asyncio +async def test_an_unknown_actor_is_refused(): + """Free text here would make the column unreadable within a month.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError): + await rt.set_dial(1, "auto_inject", "floor", 0.6, actor="agent", + reason="a perfectly good reason that is long enough") + + +def test_the_tool_teaches_reading_the_records_not_the_percentile(): + """The contract an agent actually reads (rule 167). + + The failure this prevents is a model moving a bar because `near_misses.p90` + sat close to it. That has been measured pointing the wrong way — 69 declines + where every percentile said "lower it" and the refused record was a false + positive — so the docstring has to carry the method, not just the warning. + """ + from scribe.mcp.tools import retrieval_tuning as tool + + doc = tool.tune_retrieval.__doc__ + assert "near_miss_samples" in doc, "the tool does not name how to get records" + assert "PERCENTILE ALONE" in doc.upper() + # And the worked example, because an abstract warning loses to a number. + assert "69" in doc + + +def test_all_three_tools_are_registered(): + from scribe.mcp.tools import retrieval_tuning as tool + + names = [] + + class _MCP: + def tool(self, name): + names.append(name) + return lambda fn: fn + + tool.register(_MCP()) + assert names == [ + "retrieval_surfaces", "tune_retrieval", "retrieval_tuning_history", + ] diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index 2fda52e..a74ac45 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -23,7 +23,7 @@ def test_backup_version_is_current(): (Named for the number it asserted until v10, which is exactly the drift a name-carrying-a-value invites; it now says what it checks.)""" - assert backup.BACKUP_VERSION == 15 + assert backup.BACKUP_VERSION == 16 def _exportable_note(**over): @@ -134,6 +134,7 @@ def _column_guard_targets(): from scribe.models.note_supersession import NoteSupersession from scribe.models.note_usage import NoteUsageEvent from scribe.models.rule_usage import RuleUsageEvent + from scribe.models.retrieval_tuning import RetrievalTuningEvent from scribe.models.note_version import NoteVersion from scribe.models.rule_version import RuleVersion from scribe.models.project import Project @@ -164,6 +165,9 @@ def _column_guard_targets(): "rule_relations": (RuleRelation, backup._rule_relation_rows), "note_usage_events": (NoteUsageEvent, backup._usage_event_rows), "rule_usage_events": (RuleUsageEvent, backup._rule_usage_event_rows), + "retrieval_tuning_events": ( + RetrievalTuningEvent, backup._retrieval_tuning_event_rows, + ), "design_systems": (DesignSystem, backup._design_system_rows), "design_tokens": (DesignToken, backup._design_token_rows), "repo_bindings": (RepoBinding, backup._repo_binding_rows), @@ -340,7 +344,9 @@ async def test_export_full_backup_contains_every_declared_section(): "systems", "record_systems", "design_systems", "design_tokens", "note_usage_events", "repo_bindings", "note_supersessions", "code_shapes", "code_shape_events", - "code_shape_uses"): + "code_shape_uses", + # v16: the reasons beside the settings they explain. + "retrieval_tuning_events"): assert key in out, f"missing export section: {key}" assert out[key] == [] -- 2.54.0 From 25bd6742e0fb8c5a6fee92ef6a664cbb807435eb Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 11:30:32 -0400 Subject: [PATCH 09/12] fix(mcp): the three tuning tools get a read/write classification (#4102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI 6962 — `test_every_registered_tool_is_classified_exactly_once`. The backup gap is closed (1692 passed); this is the next guard, and the same shape of one: a tool registered without a classification is silently denied to a read key, with nothing to notice (#3191). `retrieval_surfaces` and `retrieval_tuning_history` read. Read access matters more than usual for these two — a session that cannot see the bar in force, or the reason it was last moved, is one that will move it again blind. `tune_retrieval` writes in both senses: the number the arm reads, and the reason appended to the audit trail. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/server.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 7eb1c5a..4d8bd73 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -131,6 +131,12 @@ _READ_ONLY_TOOLS = frozenset({ # looked at names that had one (#3191). rule_history records a pull the way # the getters above do. "rules_due_for_verification", "rule_history", + # What each retrieval surface's floor and budget currently are, and what + # has been changed about them (#4102). Both pure reads; `tune_retrieval` is + # the write and is deliberately NOT here. Read access matters more than + # usual for these two: a session that cannot see the bar in force, or the + # reason it was last moved, is a session that will move it again blind. + "retrieval_surfaces", "retrieval_tuning_history", }) # Every tool that WRITES, by name. Nothing reads this set at runtime — a tool @@ -166,6 +172,9 @@ _WRITE_TOOLS = frozenset({ "create_rule", "create_project_rule", "update_rule", "move_rule", "delete_rule", "create_preference", "update_preference", "relate_rules", "unrelate_rules", "mark_rule_verified", + # retrieval tuning — a write in both senses: it moves the number the arm + # reads, and it appends the reason to the audit trail (#4102). + "tune_retrieval", # trash "restore", "purge_trash", }) -- 2.54.0 From 6240652dce25df13f4f54a69e7658fb2da0b198e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 11:36:22 -0400 Subject: [PATCH 10/12] feat(retrieval): the operator can see what was tuned, and every write is recorded (#4102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The other half of the bargain in milestone 416 step 4. The model moves these dials; this is what makes that reviewable rather than merely automatic. THE HOLE THIS CLOSES Every retrieval floor is an ordinary settings key, and `/api/settings` accepts any key at all. A floor written through it landed correctly and recorded nothing — a tuning history with holes in it, which is worse than no history because it reads as complete. So the generic endpoint now routes registry-owned keys through `set_dial` instead of writing them as plain rows. ROUTED, not refused: refusing would only work for callers that had been updated, while this way the form, a script, and an old client all leave the trail, and there is no version of "forgot to use the other endpoint". Clearing a control is written as an explicit set back to the shipped default, because the operator reverting something is the single most important move this history can record. `set_dial` now also refuses to record a no-op. The Settings form re-sends every field on every save, so without that one press of Save would write six rows saying the operator set six dials to the numbers they were already on — and a history nobody can skim is one nobody reads. WHAT THE OPERATOR GETS `/api/retrieval/surfaces`, `/surfaces/` and `/tuning-history`, with `actor` fixed server-side rather than taken from the payload: a payload-supplied actor would let a model claim to be the operator, and "did I do this, or did the session?" is the first question this list is asked. In Settings: the five missing BUDGETS (until now only auto-inject had one, so the only control over a noisy surface was to raise its bar — which discards that surface's best candidates along with its worst), and a "What has been tuned" panel showing each change, who made it, and the reason given. The operator's own changes are marked. The MCP tool demands a reason; these endpoints do not. That asymmetry is deliberate and stated in routes/retrieval.py: the requirement exists to make the MODEL read the records before moving a number on someone else's behalf, and the operator is that someone — a mandatory justification box on every control would be friction charged to the one participant who owes no explanation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- frontend/src/views/SettingsView.vue | 267 ++++++++++++++++++++++ src/scribe/app.py | 2 + src/scribe/routes/retrieval.py | 106 +++++++++ src/scribe/routes/settings.py | 36 +++ src/scribe/services/retrieval_surfaces.py | 20 ++ src/scribe/services/retrieval_tuning.py | 18 ++ tests/test_routes_retrieval_tuning.py | 133 +++++++++++ 7 files changed, 582 insertions(+) create mode 100644 src/scribe/routes/retrieval.py create mode 100644 tests/test_routes_retrieval_tuning.py diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 5ecaa00..79920f2 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -97,6 +97,20 @@ const kbToolRuleThreshold = ref("0.68"); // prose rather than anything a tool produced (#3852). const kbPromptRuleThreshold = ref("0.72"); const kbReportPrefThreshold = ref("0.72"); +// The BUDGETS, one per arm (#4102). Until this step only auto-inject had one +// and every other arm's ceiling was a module constant nobody could reach — so +// the only control an operator had over a noisy surface was to raise its bar, +// which discards the surface's best candidates along with its worst. A budget +// keeps the top of the ranking and drops the tail, which is what was wanted. +const kbWritePathTopK = ref("3"); +const kbRuleHintTopK = ref("5"); +const kbToolRuleTopK = ref("5"); +const kbPromptRuleTopK = ref("3"); +const kbReportPrefTopK = ref("3"); +// What has been changed about retrieval, newest first — the review surface for +// changes the model made on the operator's behalf (#4102). +const tuningEvents = ref([]); +const loadingTuning = ref(false); // Near-duplicate report floors, one per record kind (services/dedup.py). // Snippets are single-chunk, so their floor sits below the 0.90 write-time // gate and catches what it lets through. Notes/tasks are scored at chunk @@ -149,6 +163,37 @@ async function saveRetention() { } } +// One entry in the retrieval tuning trail (#4102). Mirrors +// RetrievalTuningEvent.to_dict() — `old_value` is null for the first change to +// a surface, which is a different event from moving off a value somebody chose +// and renders differently below. +interface TuningEvent { + id: number; + created_at: string | null; + surface: string; + dial: string; + old_value: number | null; + new_value: number; + actor: string; + reason: string; +} + +async function loadTuningHistory() { + loadingTuning.value = true; + try { + const res = await apiGet<{ events: TuningEvent[] }>( + "/api/retrieval/tuning-history?limit=25", + ); + tuningEvents.value = res.events ?? []; + } catch { + // A history that cannot be read is not worth a toast on page load — the + // panel says so itself, and the settings above are still usable. + tuningEvents.value = []; + } finally { + loadingTuning.value = false; + } +} + async function saveKbInject() { const t = Math.min(1, Math.max(0, Number(kbInjectThreshold.value) || 0)); const k = Math.min(10, Math.max(1, Math.floor(Number(kbInjectTopK.value) || 1))); @@ -173,6 +218,22 @@ async function saveKbInject() { const trT = Math.min(1, Math.max(0, Number(kbToolRuleThreshold.value) || 0.68)); const prT = Math.min(1, Math.max(0, Number(kbPromptRuleThreshold.value) || 0.72)); const rpT = Math.min(1, Math.max(0, Number(kbReportPrefThreshold.value) || 0.72)); + // The budgets, clamped the way the server clamps them: a whole number in + // [1, 10]. Never 0 — an arm turned off is turned off by its switch, and a + // budget of zero would run the search, log the retrieval and render nothing, + // which reads in the telemetry exactly like a bar nothing cleared. + const asK = (v: string, d: number) => + Math.min(10, Math.max(1, Math.floor(Number(v) || d))); + const wpK = asK(kbWritePathTopK.value, 3); + const rhK = asK(kbRuleHintTopK.value, 5); + const trK = asK(kbToolRuleTopK.value, 5); + const prK = asK(kbPromptRuleTopK.value, 3); + const rpK = asK(kbReportPrefTopK.value, 3); + kbWritePathTopK.value = String(wpK); + kbRuleHintTopK.value = String(rhK); + kbToolRuleTopK.value = String(trK); + kbPromptRuleTopK.value = String(prK); + kbReportPrefTopK.value = String(rpK); kbInjectThreshold.value = String(t); kbInjectTopK.value = String(k); kbDupThresholdSnippet.value = String(dupSnip); @@ -211,6 +272,15 @@ async function saveKbInject() { // and a constant that lands under the bar is a dead arm, not a quiet // one (#3860). kb_reportpref_threshold: String(rpT), + // The budgets. Every one of these keys is recognised by the server as a + // retrieval dial, so this save is recorded in the tuning history as a + // change the OPERATOR made — which is the one entry the model must not + // quietly tune back. + kb_writepath_top_k: String(wpK), + kb_rulehint_top_k: String(rhK), + kb_toolrule_top_k: String(trK), + kb_promptrule_top_k: String(prK), + kb_reportpref_top_k: String(rpK), kb_duplicate_threshold_snippet: String(dupSnip), kb_duplicate_threshold_note: String(dupNote), kb_duplicate_threshold_task: String(dupTask), @@ -218,6 +288,10 @@ async function saveKbInject() { }); kbInjectSaved.value = true; setTimeout(() => (kbInjectSaved.value = false), 2000); + // The save just appended to the history it sits above, so re-read it — + // otherwise the panel shows a trail that is stale by exactly the change + // the operator is looking at it to confirm. + await loadTuningHistory(); } catch { toastStore.show('Failed to save auto-inject settings', 'error'); } finally { @@ -672,6 +746,27 @@ onMounted(async () => { if (allSettings.kb_writepath_threshold !== undefined) { kbWritePathThreshold.value = allSettings.kb_writepath_threshold; } + if (allSettings.kb_writepath_top_k !== undefined) { + kbWritePathTopK.value = allSettings.kb_writepath_top_k; + } else if (allSettings.kb_autoinject_top_k !== undefined) { + // The write path shared auto-inject's ceiling until it was given its own + // key, so an install that tuned the shared one must keep seeing that value + // here — the server falls back the same way. + kbWritePathTopK.value = allSettings.kb_autoinject_top_k; + } + if (allSettings.kb_rulehint_top_k !== undefined) { + kbRuleHintTopK.value = allSettings.kb_rulehint_top_k; + } + if (allSettings.kb_toolrule_top_k !== undefined) { + kbToolRuleTopK.value = allSettings.kb_toolrule_top_k; + } + if (allSettings.kb_promptrule_top_k !== undefined) { + kbPromptRuleTopK.value = allSettings.kb_promptrule_top_k; + } + if (allSettings.kb_reportpref_top_k !== undefined) { + kbReportPrefTopK.value = allSettings.kb_reportpref_top_k; + } + await loadTuningHistory(); if (allSettings.kb_duplicate_threshold_snippet !== undefined) { kbDupThresholdSnippet.value = allSettings.kb_duplicate_threshold_snippet; } @@ -1517,6 +1612,20 @@ async function deleteUser(userId: number) { location, not by resemblance.

+
+ + +

How many snippets and issues one edit may be offered (1–10). Reach for this rather than the threshold when the hint feels long: lowering it keeps the best matches and drops the tail, while raising the bar above throws away good matches along with weak ones.

+
+
+ + +

How many standing rules one edit may be shown (1–10).

+
git push arrives with nothing.

+
+ + +

How many standing rules one command may be shown (1–10). This is the busiest arm there is — it fires before every command — so its budget is the one most worth keeping small.

+
+
+ + +

How many rules or preferences one message may be shown (1–10).

+
+
+ + +

How many preferences a finished task may be shown (1–10).

+
+ +
+

What has been tuned

+

+ Claude adjusts the bars and budgets above from what each arm actually + surfaced and refused, and has to state a reason to change one. Nothing + here needs your attention as a matter of course — it is here so that + when a surface behaves oddly, why it is set the way it is can be read + rather than guessed at. Anything you change yourself is recorded the + same way, and is the one entry Claude will not quietly move back. +

+

Loading…

+

+ Nothing has been changed yet — every surface is on the value Scribe + shipped. Those are starting points measured against one corpus with + one embedding model, not answers, so expect this to fill. +

+
    +
  • +
    + {{ ev.surface }} + {{ ev.dial }} + + + + {{ ev.new_value }} + + + {{ ev.actor === 'human' ? 'you' : 'Claude' }} + + {{ fmtDate(ev.created_at) }} +
    +

    {{ ev.reason }}

    +
    • +
+
+