From 381c90ca7e731e5970a8bafc8c179eaef5ce7302 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 12:18:25 -0400 Subject: [PATCH 1/5] =?UTF-8?q?feat(retrieval):=20the=20wide=20net=20becom?= =?UTF-8?q?es=20a=20pull=20=E2=80=94=20fifty=20candidates,=20no=20bar=20(#?= =?UTF-8?q?4103)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone 416 step 5. The operator's compromise — "if you're worried about excluding potentially important data let limit it to 50 entries" — moved to the surface where it is safe. Fifty in the push would be milestone 394 with extra steps; fifty in a pull crowds nothing out. `what_might_apply(query)` returns ranked rule candidates with NO threshold. The moment it serves is the one where the caller does not trust a bar to decide for them, so it does not have one — every row carries its score and the reader judges. WHY IT IS NOT A BIGGER `limit` ON `search` `_search_rules` returns statement, why and how_to_apply in full, on the stated reasoning that a caller who went looking deserves the whole record. That is the DEEP pull and should stay that way. This is the SHALLOW one — many candidates, each just enough to decide whether to open it. Opposite trade-offs, so it is a second tool. THE PREMISE THE STEP GOT WRONG The task said fifty "costs nothing". `_rule_hint_line` had already measured otherwise: ~143 tokens per line once the trigger is rendered, and #3855 tripled trigger lengths across the corpus. Fifty is ~7,000 tokens — cheap next to an arm firing before every Bash call, but not free, and a tool promising a free wide net gets reached for casually and then regretted. So it reuses the graduated shape #3851 measured for the push: the top few carry their trigger whole, the rest carry a cut of it. TRUNCATED, never dropped — the trigger is what lets a reader judge without opening, and a teaser without one is just an id. The cut borrows `_goal_line`'s technique including the fallback that matters (#4036): `textwrap.shorten` returns a bare "…" for one unbroken word. TELEMETRY Logged under its own pull source, asserted absent from both the tunable push registry and AMBIENT_SOURCES. That guard is load-bearing right now: the push arms' near-miss distributions are the evidence #4121 argues from, and a pull folded into them would move those numbers. INSTRUCTION SURFACES The using-scribe reflex and the MCP instructions both pointed at `search(content_type="rule")` for the consequential moment — the deep tool, at the moment you want breadth. They now point here, and keep `search` for reading a rule you already suspect. Written as a practice rather than a prohibition (rule 165). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- plugin/.claude-plugin/plugin.json | 2 +- plugin/skills/using-scribe/SKILL.md | 18 ++- src/scribe/mcp/server.py | 12 +- src/scribe/mcp/tools/__init__.py | 2 + src/scribe/mcp/tools/wide_net.py | 218 +++++++++++++++++++++++++ tests/test_retrieval_tuning.py | 12 +- tests/test_wide_net.py | 243 ++++++++++++++++++++++++++++ 7 files changed, 492 insertions(+), 15 deletions(-) create mode 100644 src/scribe/mcp/tools/wide_net.py create mode 100644 tests/test_wide_net.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 5600365..a266b4f 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.0111", + "version": "2026.09.17.1618", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index 972ee1a..edc3a39 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -1,6 +1,6 @@ --- name: using-scribe -description: Use at the START of every session, and before answering anything about the operator's work or starting any task — establishes the Scribe-first reflex. You hold none of the operator's rules: they arrive by retrieval when your work matches one, and search(content_type="rule") is how you ask before a consequential act. Call enter_project when a repo/project is in scope. Then recall before acting, update over duplicate, plan in Scribe not in files. +description: Use at the START of every session, and before answering anything about the operator's work or starting any task — establishes the Scribe-first reflex. You hold none of the operator's rules: they arrive by retrieval when your work matches one, and what_might_apply is how you ask before a consequential act — it returns the wide net of candidates with no bar. Call enter_project when a repo/project is in scope. Then recall before acting, update over duplicate, plan in Scribe not in files. --- # Using Scribe @@ -73,9 +73,19 @@ Two constraints on *how* that's achieved: So "no rule arrived" means "nothing matched", never "no rule exists" — an empty session is not evidence of an empty rulebook. Retrieval fires when - something asks: before a consequential act, `search(content_type="rule")` on - what you are about to do, and pull a record's full statement with - `get_rule(id)` when it is about to bite. When a project is in scope, pass + something asks, so ask — and reach for the tool that fits the moment: + + - **Before a consequential act**, and before handing work back because you + are unsure you may finish it: `what_might_apply("what you are about to + do")`. It returns up to fifty ranked candidates with NO bar. The arms that + push rules at you spend a budget of three and say nothing about what sat + just underneath — right for something firing before every command, wrong + for the one moment you actually want to be sure. Expect the tail to be + noise; you are reading for the one record you would have missed. + - **When you already suspect a particular rule**: `search(content_type= + "rule")` for full statements, or `get_rule(id)` when it is about to bite. + + When a project is in scope, pass its `project_id`: the answer is then the global rules plus that project's own, never another project's. `enter_project(id)` lists the project's own rules by title. diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 4d8bd73..c6719a6 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -44,8 +44,11 @@ client reads Agent Skills) and in each tool's description. The index: system. An `inception` key: ask what it inherits, then decide_project_inception. - RULES: nothing preloads; a rule arrives when your work matches it. Before a - consequential act, search(content_type="rule"). Silence means - nothing matched, not none. Rules bind; preferences guide. + consequential act — or before handing work back unsure you may finish it — + what_might_apply("what you are about to do"): the wide net, fifty ranked + candidates, no bar. search(content_type="rule") reads one you already + suspect. Silence means nothing matched, not none. Rules bind; preferences + guide. - RECALL: search before acting, scoped with the active project_id. - RECORD: create_task; a fix is kind="issue". add_task_log as you go; status in_progress on start, done on finish. Tag system_ids as you write. @@ -137,6 +140,11 @@ _READ_ONLY_TOOLS = frozenset({ # 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", + # The wide net (#4103) — ranked rule candidates with no bar, for the + # moment before a consequential act. A pure read, and one a read key needs + # most: it is the surface the "ask before acting" reflex calls, and a key + # that could not reach it would be denied exactly the check it should run. + "what_might_apply", }) # Every tool that WRITES, by name. Nothing reads this set at runtime — a tool diff --git a/src/scribe/mcp/tools/__init__.py b/src/scribe/mcp/tools/__init__.py index f292d41..cdc6944 100644 --- a/src/scribe/mcp/tools/__init__.py +++ b/src/scribe/mcp/tools/__init__.py @@ -6,6 +6,7 @@ from `mcp.server.build_mcp_server`. """ from scribe.mcp.tools import ( design_systems, milestones, notes, processes, projects, recent, repos, retrieval_tuning, + wide_net, rulebooks, search, shapes, snippets, systems, tags, tasks, trash, ) @@ -14,6 +15,7 @@ def register_all(mcp) -> None: """Register every tool module's tools on the given FastMCP instance.""" search.register(mcp) retrieval_tuning.register(mcp) + wide_net.register(mcp) notes.register(mcp) tasks.register(mcp) projects.register(mcp) diff --git a/src/scribe/mcp/tools/wide_net.py b/src/scribe/mcp/tools/wide_net.py new file mode 100644 index 0000000..981db0c --- /dev/null +++ b/src/scribe/mcp/tools/wide_net.py @@ -0,0 +1,218 @@ +"""The wide net — every candidate that might govern what you are about to do (#4103). + +WHY THIS EXISTS + +Milestone 416 step 5, from the operator's compromise: + + "if you're worried about excluding potentially important data let limit it + to 50 entries or something like that" + +Fifty in the PUSH would be milestone 394 with extra steps — a wall the reader +skims, with the governing record indistinguishable from forty-nine others. +Fifty in a PULL is a different object: it arrives only when a session asks, it +crowds nothing out, and it is the honest home for "do not exclude potentially +important data". + +WHY IT IS NOT `search(content_type="rule")` + +That tool is the DEEP pull and should stay that way — `_search_rules` returns +`statement`, `why` and `how_to_apply` in full, on the reasoning that a caller +who went looking deserves the whole record rather than a summary to re-fetch. +Six results already run to thousands of tokens. + +This is the SHALLOW pull: many candidates, each just enough to decide whether +to open it. Opposite trade-off, so it is a second tool rather than a bigger +`limit` on the first. + +WHY THE TAIL IS TRUNCATED RATHER THAN FULL + +Because the step's premise needed correcting. The task said fifty "costs +nothing", and `_rule_hint_line` had already measured otherwise: a line runs +~143 tokens once its trigger is rendered, and #3855 tripled trigger lengths +across the corpus. Fifty of those is ~7,000 tokens — cheap next to an arm that +fires before every Bash call, but not free, and a tool that promises a free +wide net gets reached for casually and then regretted. + +So this reuses the graduated shape #3851 measured for the push: the top few +carry their trigger in full, the rest carry a cut of it. TRUNCATED, never +dropped — the trigger is what lets a reader judge relevance without opening +the record, and a teaser without one is just an id. +""" +from __future__ import annotations + +import textwrap +import time + +from scribe.mcp._context import current_user_id +from scribe.services.embeddings import semantic_search_rules +from scribe.services.retrieval_telemetry import record_retrieval + +# The telemetry `source`, and a PULL — never added to AMBIENT_SOURCES (those +# are deliveries nobody chose) and never confused with a push arm. Keeping it +# separate is load-bearing right now: the push arms' near-miss distributions +# are the evidence #4121 rests on, and a pull mixed into them would move the +# very numbers that step is arguing from. +SOURCE = "wide_net" + +# No bar. That is the point of the tool rather than an oversight — the caller +# asked for the wide net precisely because they do not trust a bar to decide +# for them here. Every row carries its score, so the reader sees where the +# ranking falls off and judges it themselves. +THRESHOLD = 0.0 + +MAX_LIMIT = 50 +DEFAULT_LIMIT = 25 + +# How many candidates carry their trigger in full before the rest are cut. +DEFAULT_DETAIL = 5 + +# The cut length for the tail. Long enough to carry the first clause of a +# trigger — which is where these state the act they are about — short enough +# that forty-five of them stay affordable. +_TEASER_CHARS = 140 + + +def _teaser(text: str) -> tuple[str, bool]: + """Shorten at a word break with a visible cut. Returns (text, was_cut). + + The technique is `plugin_context._goal_line`'s, and the fallback is the + part worth copying: `textwrap.shorten` returns a bare "…" when the string + is one unbroken word longer than the cap, and a raw slice ends mid-word + claiming to be the whole thing (#4036). Kept local rather than shared + because the two callers wrap it in different sentences; if a third appears, + that is the moment to extract it rather than now. + """ + flat = " ".join((text or "").split()) + if len(flat) <= _TEASER_CHARS: + return flat, False + short = textwrap.shorten(flat, width=_TEASER_CHARS, placeholder="…") + if short == "…": + short = flat[: _TEASER_CHARS - 1] + "…" + return short, True + + +async def what_might_apply( + query: str, + limit: int = DEFAULT_LIMIT, + detail: int = DEFAULT_DETAIL, + kind: str = "", + project_id: int = 0, +) -> dict: + """Every rule that might bear on what you are about to do, ranked, with no bar. + + REACH FOR THIS BEFORE A CONSEQUENTIAL OR IRREVERSIBLE ACT — a push, a + merge, a delete, a deploy, anything outward-facing — and before handing + work back because you are unsure whether you are allowed to finish it. + + It is the tool the "ask before a consequential act" reflex should call. + A rule reaches a session by retrieval, and the arms that push rules at you + have a small budget: they deliver the few highest-scoring candidates and + say nothing about what sat just underneath. That is right for an arm that + fires before every command and wrong for the one moment you actually want + to be sure. This is that moment's tool. + + WHAT IT RETURNS, AND WHY THE TAIL LOOKS LIKE NOISE + + There is no threshold. You get the `limit` nearest candidates whatever they + score, ordered, each with its score — so the tail IS expected to be + irrelevant, and that is the design. You are not reading the list for its + average quality; you are reading it for the one record you would otherwise + have missed. Scan the triggers, open what looks live with `get_rule(id)`, + and ignore the rest. + + The first few carry their trigger in full; the rest carry a cut of it, + marked `truncated`. A cut trigger is still enough to decide whether to + open the record, which is the whole job of a teaser. + + `kind` is on every row and is never something to infer: a **rule** must be + followed, a **preference** records how the operator wants work done. + Missing a rule is a mistake; missing a preference costs consistency. + + A NOTE ON WHAT THIS CANNOT DO. It is a pull, so it only helps if you ask. + The failure it was built for — a session withholding a routine action + because the rule permitting it never arrived — produces no tool call of its + own, so nothing will prompt you. Asking is the habit; this is where to put + it. + + Args: + query: what you are about to do, in the words you would use to + describe it — "push to dev after committing", "delete the staging + database", "merge dev to main". A command string works; a sentence + usually works better, because triggers are written as prose. + limit: how many candidates, default 25, capped at 50. + detail: how many carry their trigger in FULL before the rest are cut, + default 5. + kind: "rule" or "preference" to restrict; omit for both. + project_id: scope to one project — its own rules plus every global + one. Omit to ask the whole rulebook. + """ + uid = current_user_id() + limit = max(1, min(int(limit), MAX_LIMIT)) + detail = max(0, min(int(detail), limit)) + + report: dict = {} + started = time.perf_counter() + if project_id: + raw = await semantic_search_rules( + uid, query, limit=limit, threshold=THRESHOLD, kind=kind or None, + report=report, project_id=project_id, + ) + else: + raw = await semantic_search_rules( + uid, query, limit=limit, threshold=THRESHOLD, kind=kind or None, + report=report, everywhere=True, + ) + duration_ms = (time.perf_counter() - started) * 1000 + + candidates = [] + for rank, (score, rule) in enumerate(raw): + full = rank < detail + trigger, was_cut = ( + (" ".join((rule.when_to_apply or "").split()), False) + if full else _teaser(rule.when_to_apply or "") + ) + candidates.append({ + "id": rule.id, + "title": rule.title, + # Force, not topic. See the docstring — this is never inferred. + "kind": rule.kind, + # A rule in a rulebook topic is global; one on a project binds + # there alone (milestone 414). Which it is changes how far a + # reader should generalise from it. + "scope": "project" if rule.project_id else "global", + "when_to_apply": trigger, + "truncated": was_cut, + "score": round(float(score), 4), + }) + + # Logged as a pull, with `searched` honoured: a search that never ran must + # not be recorded as a ranker declining (#3765). + record_retrieval( + user_id=uid, + source=SOURCE, + query=query, + threshold=THRESHOLD, + limit=limit, + project_id=project_id or None, + is_task=None, + results=raw, + duration_ms=duration_ms, + best_available=report.get("best_available_score"), + best_available_id=report.get("best_available_id"), + searched=report.get("searched", True), + ) + + return { + "candidates": candidates, + "returned": len(candidates), + "detailed": min(detail, len(candidates)), + # Present so a caller can tell "the corpus offered nothing" from "the + # search never ran" — an empty query, an unavailable embedder and a + # failed query all return zero rows and mean different things (#3670). + "searched": bool(report.get("searched", True)), + "open_with": "get_rule(id)", + } + + +def register(mcp) -> None: + mcp.tool(name="what_might_apply")(what_might_apply) diff --git a/tests/test_retrieval_tuning.py b/tests/test_retrieval_tuning.py index 733315d..0c0ac5c 100644 --- a/tests/test_retrieval_tuning.py +++ b/tests/test_retrieval_tuning.py @@ -186,14 +186,10 @@ def test_the_tool_teaches_reading_the_records_not_the_percentile(): def test_all_three_tools_are_registered(): from scribe.mcp.tools import retrieval_tuning as tool - names = [] + from tests.helpers import FakeMCP - class _MCP: - def tool(self, name): - names.append(name) - return lambda fn: fn - - tool.register(_MCP()) - assert names == [ + mcp = FakeMCP() + tool.register(mcp) + assert mcp.names == [ "retrieval_surfaces", "tune_retrieval", "retrieval_tuning_history", ] diff --git a/tests/test_wide_net.py b/tests/test_wide_net.py new file mode 100644 index 0000000..cce7ee9 --- /dev/null +++ b/tests/test_wide_net.py @@ -0,0 +1,243 @@ +"""The wide net: many candidates, no bar, and a cost that stays bounded (#4103). + +WHY THIS EXISTS + +Milestone 416 step 5 moves "do not exclude potentially important data" off the +push, where fifty candidates would be a wall nobody reads, and onto a pull, +where they arrive only when a session asks for them. + +Three things have to hold, and each has a way of quietly failing: + + 1. **No bar.** The moment this tool serves is the one where the caller does + not trust a threshold to decide for them. A default floor creeping in + here would turn the wide net into the narrow one and nothing would look + wrong — the results would simply be fewer. + 2. **A bounded cost.** The step was filed claiming fifty "costs nothing". + `_rule_hint_line` had already measured ~143 tokens for a line with its + trigger rendered, so fifty is ~7,000. The graduated shape (#3851) is what + keeps this affordable, and a change that renders every trigger in full + would pass every other test here. + 3. **A pull, logged as one.** The push arms' near-miss distributions are the + evidence #4121 argues from. A pull mixed into them moves those numbers. +""" +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.mcp.tools import wide_net +from tests.helpers import FakeMCP, fake_rule, tool_doc + + +def _hits(n=3, trigger="Running git push", **over): + """n (score, rule) pairs, descending, the shape the service returns.""" + return [ + (0.9 - i * 0.01, + fake_rule(id=i + 1, title=f"rule {i + 1}", when_to_apply=trigger, **over)) + for i in range(n) + ] + + +def _patched(hits, report=None): + """Patch the search and the telemetry sink; hand back the search mock.""" + async def _search(*a, **kw): + if report is not None and "report" in kw and kw["report"] is not None: + kw["report"].update(report) + return hits + return patch.object(wide_net, "semantic_search_rules", AsyncMock(side_effect=_search)) + + +@pytest.mark.asyncio +async def test_no_bar_reaches_the_search(): + """THE POINT OF THE TOOL. A floor here would narrow the net silently.""" + with _patched(_hits()) as search, \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("push to dev") + + assert search.await_args.kwargs["threshold"] == 0.0 + assert wide_net.THRESHOLD == 0.0 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("asked, expected", [(999, 50), (0, 1), (-5, 1), (25, 25)]) +async def test_the_limit_is_clamped_to_the_cap(asked, expected): + with _patched(_hits()) as search, \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("q", limit=asked) + + assert search.await_args.kwargs["limit"] == expected + + +@pytest.mark.asyncio +async def test_the_head_carries_its_trigger_whole_and_the_tail_is_cut(): + """The graduated shape (#3851), which is what makes fifty affordable. + + A regression that rendered every trigger in full would satisfy every other + assertion in this file, so the cut is pinned on both sides: the head is + NOT marked truncated and the tail IS. + """ + long_trigger = " ".join(["running a git command before pushing anything"] * 12) + with _patched(_hits(6, trigger=long_trigger)), \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("q", detail=2) + + head, tail = out["candidates"][:2], out["candidates"][2:] + assert all(c["truncated"] is False for c in head) + assert all(c["when_to_apply"] == " ".join(long_trigger.split()) for c in head) + assert all(c["truncated"] is True for c in tail) + assert all(len(c["when_to_apply"]) <= wide_net._TEASER_CHARS + 1 for c in tail) + assert out["detailed"] == 2 + + +@pytest.mark.asyncio +async def test_a_short_trigger_is_never_marked_truncated(): + """`truncated` is a claim about this row, not about its rank. A tail row + whose trigger already fits must not claim a cut that did not happen.""" + with _patched(_hits(4, trigger="Running git push")), \ + patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("q", detail=1) + + assert all(c["truncated"] is False for c in out["candidates"]) + + +def test_the_cut_breaks_on_a_word_and_says_it_was_cut(): + """#4036's lesson, borrowed: a raw slice ends mid-word and reads as the + whole thing.""" + text, cut = wide_net._teaser("alpha beta gamma delta " * 40) + assert cut is True + assert text.endswith("…") + # Broke on a word, so no partial token sits before the marker. + assert not text.removesuffix("…").rstrip().endswith(("alph", "bet", "gam")) + + +def test_one_unbroken_word_still_yields_text_rather_than_a_bare_marker(): + """`textwrap.shorten` returns just "…" here, which would render a teaser + carrying no information at all.""" + text, cut = wide_net._teaser("x" * 500) + assert cut is True and text != "…" + assert len(text) == wide_net._TEASER_CHARS + + +@pytest.mark.asyncio +async def test_every_row_says_its_force_and_its_scope(): + """`kind` is never inferred — a rule must be followed, a preference guides + — and `scope` says how far a reader should generalise from it.""" + hits = [ + (0.8, fake_rule(id=1, kind="rule", project_id=None)), + (0.7, fake_rule(id=2, kind="preference", project_id=44)), + ] + with _patched(hits), patch.object(wide_net, "record_retrieval"), \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("q") + + assert [c["kind"] for c in out["candidates"]] == ["rule", "preference"] + assert [c["scope"] for c in out["candidates"]] == ["global", "project"] + + +# ── telemetry: a pull, and never mistaken for a push ──────────────────────── + +@pytest.mark.asyncio +async def test_the_call_is_logged_under_its_own_pull_source(): + with _patched(_hits()), \ + patch.object(wide_net, "record_retrieval") as rec, \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("push to dev") + + kw = rec.call_args.kwargs + assert kw["source"] == wide_net.SOURCE == "wide_net" + assert kw["threshold"] == 0.0 + + +def test_the_wide_net_is_not_one_of_the_tunable_push_surfaces(): + """THE GUARD that keeps #4121's evidence clean (rule 167). + + The registry holds the PUSH arms — the ones with a floor and a budget the + model tunes. This source must not appear there: a pull folded into those + rows would move the near-miss distributions that step is arguing from, and + would offer a floor to tune on a tool whose whole point is not having one. + """ + from scribe.services.retrieval_surfaces import SURFACES + + assert wide_net.SOURCE not in SURFACES + + +def test_the_wide_net_is_not_ambient_either(): + """Ambient means a delivery nobody chose. This one is chosen by definition + — somebody called the tool — so counting it as ambient would make a + deliberate ask read as a bulk hand-over.""" + from scribe.services.note_usage import AMBIENT_SOURCES + + assert wide_net.SOURCE not in AMBIENT_SOURCES + + +@pytest.mark.asyncio +async def test_a_search_that_never_ran_is_not_reported_as_a_decline(): + """#3765: an empty query, a dead embedder and a failed query all return + nothing, and none of them is a ranker declining.""" + with _patched([], report={"searched": False, "best_available_score": None}), \ + patch.object(wide_net, "record_retrieval") as rec, \ + patch.object(wide_net, "current_user_id", lambda: 1): + out = await wide_net.what_might_apply("") + + assert rec.call_args.kwargs["searched"] is False + assert out["searched"] is False + + +@pytest.mark.asyncio +async def test_what_the_bar_turned_away_is_carried_through(): + """There is no bar here, but `best_available` still answers "did the corpus + have anything at all" for a call that came back empty (#3670).""" + with _patched([], report={"searched": True, "best_available_score": 0.31, + "best_available_id": 7}), \ + patch.object(wide_net, "record_retrieval") as rec, \ + patch.object(wide_net, "current_user_id", lambda: 1): + await wide_net.what_might_apply("q") + + assert rec.call_args.kwargs["best_available"] == 0.31 + assert rec.call_args.kwargs["best_available_id"] == 7 + + +# ── the contract a session actually reads ─────────────────────────────────── + +def test_the_docstring_says_when_to_reach_for_it(): + """The step's done-when, and the load-bearing half of this tool. + + A wide net nobody knows to call is worth nothing, so the docstring has to + name the MOMENT, not just the parameters — including the one that prompted + it, where a session hands work back rather than finishing it. + """ + doc = tool_doc("scribe.mcp.tools.wide_net", "what_might_apply").lower() + + assert "consequential" in doc + assert "handing work back" in doc + # Says the tail is expected to be noise — otherwise the first caller reads + # a low-scoring list as the tool being broken. + assert "noise" in doc + # And names its own limit: a pull only helps if somebody asks. + assert "only helps if you ask" in doc + + +def test_it_is_registered_and_readable_with_a_read_key(): + from scribe.mcp.server import _READ_ONLY_TOOLS + + mcp = FakeMCP() + wide_net.register(mcp) + assert mcp.names == ["what_might_apply"] + assert "what_might_apply" in _READ_ONLY_TOOLS + + +def test_the_instruction_surfaces_point_at_it(): + """Rule 119: the instruction surfaces ARE the specification for product + behaviour, so a tool the reflex never learns about is not shipped.""" + import pathlib + + from scribe.mcp import server + + assert "what_might_apply" in server._INSTRUCTIONS + + skill = (pathlib.Path(__file__).resolve().parents[1] + / "plugin" / "skills" / "using-scribe" / "SKILL.md").read_text() + assert "what_might_apply" in skill -- 2.54.0 From dcf800ed65bafb51e81614841d45b84991b5a685 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 12:20:56 -0400 Subject: [PATCH 2/5] fix(plugin): a line break split the token a guidance guard matches on (#4103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI 6973 — `test_guidance_ownership` on "rules are retrieved; ask before a consequential act": the using-scribe skill was missing the marker `content_type="rule"`. It was there. I had hard-wrapped the new bullet across `content_type=` / `"rule"`, and that guard matches its MARKERS raw while normalising whitespace only for the longer `statement` — which is why the failure reported the statement present and the marker absent in the same breath. Rewrapped so the token sits on one line. Worth noting rather than just fixing: the markers are deliberately raw-matched, since a marker IS a literal a reader copies. `tool_doc` flattens for exactly the opposite reason one layer over. Both are right; the trap is that they differ. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- plugin/skills/using-scribe/SKILL.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index edc3a39..ea83b4b 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -82,12 +82,12 @@ Two constraints on *how* that's achieved: just underneath — right for something firing before every command, wrong for the one moment you actually want to be sure. Expect the tail to be noise; you are reading for the one record you would have missed. - - **When you already suspect a particular rule**: `search(content_type= - "rule")` for full statements, or `get_rule(id)` when it is about to bite. + - **When you already suspect a particular rule**: reach for + `search(content_type="rule")` to read full statements, or `get_rule(id)` + when one is about to bite. - When a project is in scope, pass - its `project_id`: the answer is then the global rules plus that project's - own, never another project's. `enter_project(id)` lists the project's own + Either way, when a project is in scope pass its `project_id`: the answer is + then the global rules plus that project's own, never another project's. `enter_project(id)` lists the project's own rules by title. **`kind` says how much force a record carries, and it is never something to -- 2.54.0 From aee24c9c1c1e9418aed80dd99cc030fb7c90040a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 12:45:48 -0400 Subject: [PATCH 3/5] feat(retrieval): a tuned number carries the space it was measured in (#4104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone 416 step 6. A retrieval floor is a cosine similarity, which only means something inside one embedding model's geometry over documents cut one particular way. Change either and every floor on the install keeps applying while describing nothing — and nothing anywhere says so, because the scores simply come out different and the bar goes on cutting. `CHUNKER_VERSION` already solved this for documents: stamped per row, so the backfill re-embeds precisely what is stale. The same idea, applied to the numbers: - `calibration_stamp()` — embedding model + document shape, one definition. TWO fields, never a fused string (rule 149): a mismatch has to say WHICH half moved, because they call for different responses. - `retrieval_tuning_events` gains `embedding_model` / `shape_version` (migration 0104), stamped on every write. Nullable and NOT backfilled — "unstamped" is the honest answer for a row written before this existed, and it reports as `stale: null`, never as fine. - `current_settings` reports calibration per dial: tuned rows from their event, untouched dials from the registry default's own stamp. - `retrieval_surfaces` and the Settings panel show the mismatch. The panel renders ONLY when something is stale, so seeing it at all is the signal. - `migrate_floor` / `migrate_retrieval_floor` answers "a path for thresholds to be inherited by the next model so that they don't have to recalibrate a lot": the raw cosine cannot cross models, but the PERCENTILE it represented can. Measure what fraction of a surface's logged calls the old floor admitted, re-score those queries under the current model, take the value admitting the same fraction. Dry run by default; applying writes an ordinary tuning event with the arithmetic in its reason. Nothing auto-retunes. A stale stamp says a number is no longer a measurement; it does not say what the number should be, and #4102 measured the one case where the statistic and the correct action pointed opposite ways. The load-bearing test is an ABSENCE: no chat-model identifier may appear anywhere in the calibration path. Claude produces none of these scores, so a Claude upgrade must trigger nothing — a false alarm here teaches the operator to ignore the real one on the day bge-small becomes bge-base. Backup v17 carries both columns, unfilled on the way out and on the way back: a round trip must not turn "we don't know" into a stated fact. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- ...4_tuning_events_carry_their_calibration.py | 51 ++++ frontend/src/views/SettingsView.vue | 118 +++++++++ src/scribe/mcp/server.py | 1 + src/scribe/mcp/tools/retrieval_tuning.py | 74 ++++++ src/scribe/models/retrieval_tuning.py | 21 ++ src/scribe/services/backup.py | 20 +- src/scribe/services/embeddings.py | 32 +++ src/scribe/services/retrieval_migration.py | 248 ++++++++++++++++++ src/scribe/services/retrieval_surfaces.py | 25 ++ src/scribe/services/retrieval_tuning.py | 68 +++++ tests/test_calibration_stamp.py | 191 ++++++++++++++ tests/test_retrieval_migration.py | 153 +++++++++++ tests/test_services_backup.py | 2 +- 13 files changed, 1002 insertions(+), 2 deletions(-) create mode 100644 alembic/versions/0104_tuning_events_carry_their_calibration.py create mode 100644 src/scribe/services/retrieval_migration.py create mode 100644 tests/test_calibration_stamp.py create mode 100644 tests/test_retrieval_migration.py diff --git a/alembic/versions/0104_tuning_events_carry_their_calibration.py b/alembic/versions/0104_tuning_events_carry_their_calibration.py new file mode 100644 index 0000000..2211714 --- /dev/null +++ b/alembic/versions/0104_tuning_events_carry_their_calibration.py @@ -0,0 +1,51 @@ +"""retrieval_tuning_events carries the space each number was measured in (#4104) + +Revision ID: 0104 +Revises: 0103 +Create Date: 2026-09-17 + +Milestone 416 step 6. A retrieval floor is a cosine distance in ONE embedding +model's geometry, computed over documents cut one particular way. Change the +model and every score moves at once; change the chunker and the same record +embeds different text. Either way a number chosen before the change is a +measurement of something that no longer exists — and today nothing records +which world it was chosen in, so the staleness is unknowable rather than +merely unknown. + +`CHUNKER_VERSION` already solved exactly this for documents: stored per row, so +the startup backfill re-embeds precisely what is stale instead of wiping the +table. These two columns are that idea applied to the tuned numbers. + +TWO COLUMNS, NOT ONE (rule 149). A reader has to be able to say WHICH half +moved: a new embedding model and a re-cut document shape invalidate the same +numbers for different reasons, and a fused `"@"` could only report +that something changed. + +NULLABLE, and not backfilled. The rows already in this table were written +under something, but naming it would be inventing a fact — the honest value is +"unstamped", which is a different answer from a model name that might be wrong. +`current_settings` reports an unstamped dial as exactly that. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0104" +down_revision = "0103" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column( + "retrieval_tuning_events", + sa.Column("embedding_model", sa.Text(), nullable=True), + ) + op.add_column( + "retrieval_tuning_events", + sa.Column("shape_version", sa.Integer(), nullable=True), + ) + + +def downgrade() -> None: + op.drop_column("retrieval_tuning_events", "shape_version") + op.drop_column("retrieval_tuning_events", "embedding_model") diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 8b1b452..587aba5 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -178,6 +178,64 @@ interface TuningEvent { reason: string; } +// What a dial's number was measured against, and whether that space has moved +// (#4104). Mirrors the `calibration` block `current_settings` returns per dial. +// `stale` is nullable on purpose: a dial moved before Scribe recorded stamps is +// UNKNOWN rather than fine, and rendering the two the same would hide the case +// most likely to be wrong. +interface DialCalibration { + source: string; + embedding_model: string | null; + shape_version: number | null; + model_changed: boolean | null; + shape_changed: boolean | null; + stale: boolean | null; +} + +interface SurfaceRow { + surface: string; + calibration: Record; +} + +const surfaceRows = ref([]); + +// Flattened to one line per dial that needs looking at, because the operator's +// question is "which numbers do I no longer trust", not "tell me about each of +// six surfaces". Anything calibrated against the live model contributes +// nothing — the panel below renders only when this is non-empty, so a healthy +// install never sees it. A warning that is always on the screen is one nobody +// reads when it finally means something. +const staleDials = computed(() => + surfaceRows.value.flatMap((row) => + Object.entries(row.calibration ?? {}) + .filter(([, cal]) => cal.stale !== false) + .map(([dial, cal]) => ({ + key: `${row.surface}.${dial}`, + surface: row.surface, + dial, + why: + cal.source === "unstamped" + ? "changed before Scribe recorded what it was measured against" + : cal.model_changed && cal.shape_changed + ? `measured under ${cal.embedding_model} at document shape ${cal.shape_version} — both have changed since` + : cal.model_changed + ? `measured under ${cal.embedding_model}, which is no longer the embedding model` + : `measured at document shape ${cal.shape_version}, and documents are cut differently now`, + })), + ), +); + +async function loadSurfaces() { + try { + const res = await apiGet<{ surfaces: SurfaceRow[] }>("/api/retrieval/surfaces"); + surfaceRows.value = res.surfaces ?? []; + } catch { + // Same reasoning as the history below: unreadable calibration is not worth + // a toast on page load, and the dials above still work. + surfaceRows.value = []; + } +} + async function loadTuningHistory() { loadingTuning.value = true; try { @@ -292,6 +350,7 @@ async function saveKbInject() { // otherwise the panel shows a trail that is stale by exactly the change // the operator is looking at it to confirm. await loadTuningHistory(); + await loadSurfaces(); } catch { toastStore.show('Failed to save auto-inject settings', 'error'); } finally { @@ -767,6 +826,7 @@ onMounted(async () => { kbReportPrefTopK.value = allSettings.kb_reportpref_top_k; } await loadTuningHistory(); + await loadSurfaces(); if (allSettings.kb_duplicate_threshold_snippet !== undefined) { kbDupThresholdSnippet.value = allSettings.kb_duplicate_threshold_snippet; } @@ -1772,6 +1832,32 @@ async function deleteUser(userId: number) { moves the dial with the argument attached. This panel is the other half of that bargain — a change made on your behalf is one you can read, disagree with, and set back by hand above. --> + +
+

+ {{ staleDials.length }} + {{ staleDials.length === 1 ? 'value was' : 'values were' }} + measured against something that has changed +

+

+ These bars are similarity scores, and a similarity score only means + something inside the model that produced it. They still apply — they + just no longer measure what they were set to measure. Nothing is + adjusted automatically; ask Claude to migrate them and it will carry + each one across at the same selectivity, then check the result. +

+
    +
  • + {{ d.surface }} + {{ d.dial }} + {{ d.why }} +
    • +
+

What has been tuned

@@ -4115,4 +4201,36 @@ async function deleteUser(userId: number) { font-size: 0.85rem; color: var(--fs-text-secondary); } +/* Stale calibration (#4104). Tinted rather than loud: the numbers still work, + they have merely stopped being measurements — that is a "come back to this", + not an outage. It only renders when something is actually stale, which is + what earns it the tint at all. */ +.calibration-warning { + margin-top: var(--fs-space-5); + padding: var(--fs-space-3); + border: 1px solid var(--fs-warning); + border-radius: var(--fs-radius-md); + background: color-mix(in srgb, var(--fs-warning) 12%, var(--fs-surface-raised)); +} +.calibration-warning-title { + margin: 0 0 var(--fs-space-2); + font-size: 0.95rem; + color: var(--fs-warning-fg); +} +.calibration-list { + list-style: none; + margin: var(--fs-space-2) 0 0; + padding: 0; + display: flex; + flex-direction: column; + gap: var(--fs-space-1); + font-size: 0.85rem; +} +.calibration-list li { + display: flex; + align-items: baseline; + flex-wrap: wrap; + gap: var(--fs-space-2); +} +.calibration-why { color: var(--fs-text-secondary); } diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index c6719a6..7462c89 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -183,6 +183,7 @@ _WRITE_TOOLS = frozenset({ # 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", + "migrate_retrieval_floor", # trash "restore", "purge_trash", }) diff --git a/src/scribe/mcp/tools/retrieval_tuning.py b/src/scribe/mcp/tools/retrieval_tuning.py index f4594e0..062ce36 100644 --- a/src/scribe/mcp/tools/retrieval_tuning.py +++ b/src/scribe/mcp/tools/retrieval_tuning.py @@ -5,6 +5,7 @@ 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_migration as migration_svc from scribe.services import retrieval_tuning as tuning_svc @@ -37,6 +38,33 @@ async def retrieval_surfaces() -> dict: 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. + + `calibration` says what SPACE each number was chosen in, per dial, and + whether that space has moved. A floor is a cosine distance in one embedding + model's geometry over documents cut one particular way; swap the model and + every score shifts at once, re-cut the documents and the same record embeds + different text. Either way the number is a measurement of something that no + longer exists, and no amount of telemetry will say so — the scores simply + come out different and the bar keeps applying. + + Read the three fields apart: + + - `stale: true` with `model_changed` — the geometry is new. Every floor on + this install needs re-measuring, not adjusting; a number that meant "quite + similar" in the old space means nothing particular in this one. + - `stale: true` with `shape_changed` — the documents are cut differently. + The scale still holds, but what a record embeds has changed, so which + records clear a bar has. + - `stale: null`, `source: "unstamped"` — the dial was moved before Scribe + recorded this. It was measured under SOMETHING and there is no way to say + what, which is a different answer from "it is fine". Treat it as worth + re-measuring, and the next tuning call stamps it. + + NOTHING IS RETUNED AUTOMATICALLY on the strength of this, here or anywhere. + A stale stamp says a number is no longer a measurement; it does not say what + the number should be. That judgement wants the same procedure as any other + tuning change — `retrieval_telemetry(near_miss_samples=5)`, open the records + it names, then `tune_retrieval` with what you read in the reason. """ return {"surfaces": await tuning_svc.current_settings(current_user_id())} @@ -109,7 +137,53 @@ async def retrieval_tuning_history(surface: str = "", limit: int = 20) -> dict: } +async def migrate_retrieval_floor( + surface: str, sample: int = 200, apply: bool = False, +) -> dict: + """Carry one surface's floor across an embedding-model or chunker change. + + Reach for this when `retrieval_surfaces` reports a dial `stale` — and only + then. A floor is a cosine similarity, so a new embedding model moves every + score on the install at once and the stored number silently stops describing + anything. Re-deriving six floors by reading telemetry is the work this + avoids. + + WHAT IT TRANSFERS. Not the number — the SELECTIVITY. A floor's real content + is a decision about how much of what an arm sees is worth spending attention + on, and that decision survives a change of units. This measures what fraction + of the surface's recent calls the old floor admitted, re-scores those same + queries under the current model, and proposes the value that admits the same + fraction. + + DRY RUN BY DEFAULT. With `apply=False` it returns the arithmetic and writes + nothing. Read it before applying: the sample sizes, both score ranges, and + whether the shift looks like a change of scale or like a corpus that has not + finished re-embedding. A model change is the moment every number is + uncertain at once, which is the worst moment to let a statistic move dials + unattended. + + AND IT IS STILL A STARTING POINT. Percentile-preserving means the new floor + is as good as the old one was, not better — if the old floor was wrong, this + faithfully carries the wrongness onto the new scale. It is the number to + begin from while the surface collects enough calls to judge properly, which + is `retrieval_telemetry(near_miss_samples=5)` and `tune_retrieval` as usual. + + Args: + surface: the surface name from `retrieval_surfaces`. + sample: how many recent logged calls to re-score, default 200. Each one + is an embedding plus a scan, so this is real work; a surface with + only a handful of logged calls gives a percentile made of noise. + apply: False (default) returns the proposal. True writes it, as an + ordinary tuning event with the arithmetic in its reason — so a + migrated floor is reviewable and revertible like any other. + """ + return await migration_svc.migrate_floor( + current_user_id(), surface, sample=sample, apply=apply, + ) + + def register(mcp) -> None: mcp.tool(name="retrieval_surfaces")(retrieval_surfaces) + mcp.tool(name="migrate_retrieval_floor")(migrate_retrieval_floor) mcp.tool(name="tune_retrieval")(tune_retrieval) mcp.tool(name="retrieval_tuning_history")(retrieval_tuning_history) diff --git a/src/scribe/models/retrieval_tuning.py b/src/scribe/models/retrieval_tuning.py index b08d2c7..dff3fe8 100644 --- a/src/scribe/models/retrieval_tuning.py +++ b/src/scribe/models/retrieval_tuning.py @@ -75,6 +75,23 @@ class RetrievalTuningEvent(Base): # becomes a formality; the service refuses a blank one. reason: Mapped[str] = mapped_column(Text, nullable=False) + # WHAT SPACE THIS NUMBER WAS MEASURED IN (#4104). A floor is a cosine + # distance in one embedding model's geometry, over documents cut one + # particular way — change either and the number describes something that + # no longer exists. + # + # On the EVENT rather than beside the setting, because this table already + # holds one row per change and `current_settings` already reads the latest + # per dial. A parallel stamp row next to the value would be a second place + # to keep in sync, and the two disagreeing is worse than neither. + # + # Both NULLABLE for the rows written before this step: those were measured + # under something, but claiming to know which would be inventing a fact. + # Null here means "unstamped", which is a different and honest answer from + # naming a model that may be wrong. + embedding_model: Mapped[str | None] = mapped_column(Text, nullable=True) + shape_version: Mapped[int | None] = mapped_column(Integer, nullable=True) + __table_args__ = ( # The only read this table has: one surface's history, newest first. Index( @@ -94,4 +111,8 @@ class RetrievalTuningEvent(Base): "new_value": self.new_value, "actor": self.actor, "reason": self.reason, + # Null for every row written before #4104, and rendered as + # "unstamped" rather than guessed at — see the column comments. + "embedding_model": self.embedding_model, + "shape_version": self.shape_version, } diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 5a9d135..cf7573e 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -74,8 +74,14 @@ logger = logging.getLogger(__name__) # 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. +# v17 (2026-09) added retrieval_tuning_events.embedding_model / shape_version +# (milestone 416 step 6): a floor is a distance in ONE embedding model's +# geometry over documents cut one particular way, so the number alone cannot +# say whether it still measures anything. Both travel NULLABLE and unfilled — +# a row written before the stamp existed restores unstamped, because inventing +# the model it was measured under would turn "unknown" into a stated fact. # Bump when the serialized schema changes. -BACKUP_VERSION = 16 +BACKUP_VERSION = 17 # 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 @@ -358,6 +364,12 @@ def _retrieval_tuning_event_rows(rows) -> list[dict]: "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, + # Carried, and NOT defaulted to the current model on the way out + # (#4104): a row that was unstamped when it was written is still + # unstamped after a round trip, and a backup that quietly filled + # the gap would turn "we don't know" into a stated fact. + "embedding_model": r.embedding_model, + "shape_version": r.shape_version, "created_at": r.created_at.isoformat() if r.created_at else None, } for r in rows @@ -1247,6 +1259,12 @@ async def _restore_v2(data: dict) -> dict: new_value=t_data.get("new_value"), actor=t_data.get("actor") or "model", reason=t_data.get("reason", ""), + # .get with no default, deliberately (v17): an archive written + # before the stamp existed has no key here, and None is the + # right answer for it — the same "unstamped" a pre-#4104 row + # carries in place. + embedding_model=t_data.get("embedding_model"), + shape_version=t_data.get("shape_version"), created_at=_dt(t_data.get("created_at")), )) stats["retrieval_tuning_events"] += 1 diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 000dfed..2dcbbe7 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -217,6 +217,38 @@ def embedding_text(title: str | None, body: str | None) -> str: # wipe migrations 0067/0077 had to do. CHUNKER_VERSION = 1 + +# The public name of the space every score lives in, and the two facts that +# can invalidate a tuned number (#4104). +# +# EMBEDDING_MODEL is `_MODEL_NAME` under a name other modules may read. It was +# private until this step, which is precisely why nothing outside this file +# could state what space a threshold was measured in — a floor is a distance in +# THIS model's geometry and means nothing in another's. +# +# The pair is what a stamp is made of, and the pairing is the point: a score +# changes when the model changes (different geometry) OR when the document +# shape changes (different text embedded for the same record). Either one +# invalidates a number that was measured before it. +EMBEDDING_MODEL = _MODEL_NAME + + +def calibration_stamp() -> dict: + """What a tuned retrieval number was measured against. + + ONE definition, because the alternative is each reader assembling the pair + and one of them forgetting a half. Returned as a dict rather than a string + so a mismatch can say WHICH half moved — "the model changed" and "the + chunker changed" call for different responses, and a fused string can only + report that something did. + + Deliberately says nothing about the CHAT model. A Claude upgrade changes no + score here and must never raise a recalibration prompt: a false alarm on + this surface teaches an operator to ignore the true one. `tests/ + test_calibration_stamp.py` asserts that absence rather than trusting it. + """ + return {"embedding_model": EMBEDDING_MODEL, "shape_version": CHUNKER_VERSION} + # Character budget approximating the model window. Tokens-per-char varies by # content — ~4 chars/token for prose, closer to 3 for code and tables — so 1400 # chars sits at roughly 350-470 tokens, leaving headroom for the title prefixed diff --git a/src/scribe/services/retrieval_migration.py b/src/scribe/services/retrieval_migration.py new file mode 100644 index 0000000..ba20473 --- /dev/null +++ b/src/scribe/services/retrieval_migration.py @@ -0,0 +1,248 @@ +"""Carrying a tuned floor across an embedding-model change (#4104). + +THE PROBLEM + +A floor is a cosine similarity, and a cosine similarity is a distance in one +model's geometry. `bge-small-en-v1.5` → `bge-base-en-v1.5` moves every score on +the install at once, in no direction anyone can predict per record. The numbers +in `settings` survive the swap unchanged and silently stop describing anything: +the bar keeps applying, the telemetry keeps filling, and nothing anywhere says +the arm is now cutting in a different place. + +Re-deriving six floors by hand is the alternative, and it is the thing the +operator asked for a way out of — *"a path for thresholds to be inherited by +the next version or different model so that they don't have to recalibrate a +lot."* + +WHAT TRANSFERS, AND WHY IT IS NOT THE NUMBER + +The raw cosine does not transfer. **The percentile it represented does.** A +floor's real content is a decision about SELECTIVITY — "admit roughly the top +fifth of what this arm sees" — and that decision is about the operator's +tolerance for noise on that surface, not about the embedder. It was true before +the model changed and is still true after. + +So: measure what fraction of this surface's calls the old floor admitted, using +the scores the old model actually produced; re-score the same queries under the +new one; and take the value that admits the same fraction. Same decision, new +units. + +WHY `best_available_score` IS THE FIGURE ON BOTH SIDES + +Because it is the one number that exists whether or not a call returned +anything — the highest score the corpus offered, before the bar was applied +(#3670). It is what `retrieval_logs` recorded under the old model, and it is +what a re-score reproduces under the new one, so the two sides are the same +measurement rather than two things that resemble each other. + +It also makes the re-score cheap to get right: `best_available_score` is +computed over the whole candidate set BEFORE `limit` and before any +`exclude_ids` the arm passed, so neither has to be reproduced here. Only the +corpus FILTERS matter, which is why `_RESCORERS` below carries those and +nothing else. + +WHAT THIS DOES NOT DO + +It does not fire by itself, and `migrate_floor` will not write anything unless +asked twice — `apply=True` on top of having read the dry run. A model change is +exactly the moment when every number is uncertain at once, which is the worst +possible moment to let a statistic move six dials unattended. The percentile is +a starting point on the new scale, in the same sense the shipped defaults are a +starting point: better than a stale number, not a substitute for reading what +the surface actually turned away. +""" +from __future__ import annotations + +import logging + +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.retrieval_log import RetrievalLog +from scribe.services.embeddings import ( + calibration_stamp, + semantic_search_notes, + semantic_search_rules, +) +from scribe.services.retrieval_surfaces import floor_for, get_surface +from scribe.services.retrieval_tuning import set_dial + +logger = logging.getLogger(__name__) + +# How many of a surface's recent calls to re-score. Every one is an embedding +# plus a full scan, so this is a real cost — but a percentile off twenty calls +# is noise, and the arms that matter here log hundreds a week. +DEFAULT_SAMPLE = 200 + + +async def _rescore_rules(user_id: int, query: str, project_id: int | None, + kind: str | None) -> float | None: + rep: dict = {} + await semantic_search_rules( + user_id, query, limit=1, threshold=0.0, kind=kind, + report=rep, project_id=project_id or None, + ) + return rep.get("best_available_score") + + +async def _rescore_notes(user_id: int, query: str, project_id: int | None, + note_type, task_kind) -> float | None: + rep: dict = {} + await semantic_search_notes( + user_id, query, limit=1, threshold=0.0, + project_id=project_id or None, note_type=note_type, + task_kind=task_kind, scope="browse", report=rep, + ) + return rep.get("best_available_score") + + +# ONE re-scorer per surface, carrying that arm's corpus filters and nothing +# else. These filters are stated a second time here — the arms in +# `plugin_context` are where they are first declared — and that duplication is +# deliberate rather than overlooked: the alternative is calling the arms +# themselves, which build a menu, write telemetry and record records as +# surfaced. A migration that logged two hundred fake retrievals would corrupt +# the very table the next tuning decision reads. +# +# `tests/test_retrieval_migration.py` asserts every registry surface has an +# entry, so a seventh arm cannot quietly become un-migratable. +_RESCORERS = { + "auto_inject": lambda u, q, p: _rescore_notes(u, q, p, None, None), + "write_path": lambda u, q, p: _rescore_notes( + u, q, p, ("snippet", "note"), "issue" + ), + "write_path_rule": lambda u, q, p: _rescore_rules(u, q, p, None), + "pre_tool_rule": lambda u, q, p: _rescore_rules(u, q, p, None), + "prompt_rule": lambda u, q, p: _rescore_rules(u, q, p, None), + "report_preference": lambda u, q, p: _rescore_rules(u, q, p, "preference"), +} + + +def _floor_admitting(scores: list[float], fraction: float) -> float: + """The floor that admits `fraction` of `scores`, on this scale. + + Deliberately exact rather than interpolated: with the scores sorted + highest-first, the k-th one IS the bar that admits exactly k. An + interpolated quantile would return a number no observed call sits on, which + is harder to sanity-check against the sample it came from. + + A fraction rounding to zero returns a floor just above the best score seen — + an arm that admitted nothing keeps admitting nothing, rather than being + quietly reopened by a migration. + """ + ranked = sorted(scores, reverse=True) + k = int(round(fraction * len(ranked))) + if k <= 0: + return min(1.0, ranked[0] + 1e-6) + return ranked[min(k, len(ranked)) - 1] + + +async def migrate_floor( + user_id: int, + surface: str, + *, + sample: int = DEFAULT_SAMPLE, + apply: bool = False, +) -> dict: + """Recompute one surface's floor on the current model, preserving selectivity. + + Returns the working: how many calls were sampled, what fraction the old + floor admitted, and what value admits the same fraction now. Writes nothing + unless `apply=True`, and when it does it writes an ordinary tuning event + with the arithmetic in its reason — a migrated floor is reviewable and + revertible on exactly the same terms as one a reader chose. + """ + get_surface(surface) # refuses an unknown name + rescore = _RESCORERS.get(surface) + if rescore is None: + raise ValueError( + f"no re-scorer for surface {surface!r}. A surface that cannot be " + "re-scored cannot be migrated — add it to _RESCORERS beside the " + "arm's own corpus filters." + ) + + old_floor = await floor_for(user_id, surface) + async with async_session() as session: + rows = (await session.execute( + select( + RetrievalLog.query, + RetrievalLog.project_id, + RetrievalLog.best_available_score, + ) + .where( + RetrievalLog.source == surface, + RetrievalLog.user_id == user_id, + RetrievalLog.best_available_score.is_not(None), + RetrievalLog.query.is_not(None), + RetrievalLog.query != "", + ) + .order_by(RetrievalLog.id.desc()) + .limit(max(1, int(sample))) + )).all() + + if not rows: + # Not an error. A surface with no logged calls has no evidence of what + # its floor was doing, and inventing a migration for it would be the + # exact failure this module's docstring warns about. + return { + "surface": surface, "migrated": False, + "why": "no logged calls carry a best_available_score for this " + "surface, so there is no old distribution to preserve", + "sampled": 0, "old_floor": old_floor, + } + + old_scores = [float(r.best_available_score) for r in rows] + admitted = sum(1 for s in old_scores if s >= old_floor) + fraction = admitted / len(old_scores) + + new_scores: list[float] = [] + for r in rows: + score = await rescore(user_id, r.query, r.project_id) + if score is not None: + new_scores.append(float(score)) + + if not new_scores: + # The corpus answered nothing for any sampled query. Almost always an + # embedder that has not finished backfilling under the new model — + # migrating from it would set every floor off an empty distribution. + return { + "surface": surface, "migrated": False, + "why": "re-scoring returned nothing for any sampled query — the " + "corpus is probably not embedded under the current model yet", + "sampled": len(rows), "old_floor": old_floor, + "old_admit_rate": round(fraction, 4), + } + + proposed = round(min(1.0, max(0.0, _floor_admitting(new_scores, fraction))), 4) + stamp = calibration_stamp() + reason = ( + f"Migrated across a calibration change, preserving selectivity: the old " + f"floor {old_floor} admitted {admitted} of {len(old_scores)} sampled " + f"calls ({fraction:.1%}); {proposed} admits the same share of " + f"{len(new_scores)} queries re-scored under " + f"{stamp['embedding_model']}/shape {stamp['shape_version']}. The " + f"percentile is what carried across, not the number — spot-check the " + f"arm before trusting it." + ) + + result = { + "surface": surface, + "migrated": False, + "sampled": len(rows), + "rescored": len(new_scores), + "old_floor": old_floor, + "old_admit_rate": round(fraction, 4), + "old_score_range": [round(min(old_scores), 4), round(max(old_scores), 4)], + "new_score_range": [round(min(new_scores), 4), round(max(new_scores), 4)], + "proposed_floor": proposed, + "calibration": stamp, + "reason": reason, + } + if not apply: + return result + + result["migrated"] = True + result["applied"] = await set_dial( + user_id, surface, "floor", proposed, reason=reason, actor="model", + ) + return result diff --git a/src/scribe/services/retrieval_surfaces.py b/src/scribe/services/retrieval_surfaces.py index bd6063e..88ae47b 100644 --- a/src/scribe/services/retrieval_surfaces.py +++ b/src/scribe/services/retrieval_surfaces.py @@ -103,6 +103,31 @@ class Surface: asks: str over: str fires: str + measured_model: str = "BAAI/bge-small-en-v1.5" + measured_shape: int = 1 + """What the SHIPPED defaults above were measured against (#4104). + + A floor is a distance in one embedding model's geometry, over documents cut + one particular way. Either can change, and when one does every number in + this table describes something that no longer exists. + + TWO FIELDS, NEVER ONE FUSED STRING (rule 149). A mismatch has to be able to + say WHICH half moved: a new embedding model and a re-cut document shape + invalidate the same numbers for different reasons and call for different + responses. `"@"` could only report that something changed, which + is the answer nobody can act on. Same reason `calibration_stamp()` returns + a dict and the event table gives each half its own column. + + Recorded per surface rather than once for the module because they need not + move together: a surface retuned after a model change carries the new stamp + while its untouched siblings still carry the old one, and telling those + apart is the whole job. + + LITERALS, deliberately, rather than an import of the live values — a stamp + says what was true when the number was chosen, so one that tracked the + current model would always agree with it and could never report staleness. + """ + budget_falls_back_to: str = "" """A budget key to inherit when this surface has none of its own set. diff --git a/src/scribe/services/retrieval_tuning.py b/src/scribe/services/retrieval_tuning.py index 471df2f..787f166 100644 --- a/src/scribe/services/retrieval_tuning.py +++ b/src/scribe/services/retrieval_tuning.py @@ -47,6 +47,7 @@ from sqlalchemy import select from scribe.models import async_session from scribe.models.retrieval_tuning import RetrievalTuningEvent +from scribe.services.embeddings import calibration_stamp from scribe.services.retrieval_surfaces import ( MAX_BUDGET, budget_for, @@ -86,6 +87,50 @@ def _clean_reason(reason: str) -> str: return text +def _calibration(row, s, live: dict) -> dict: + """What space one dial's number was chosen in, and whether that space moved. + + Three sources, and they are not interchangeable: + + - `tuned` — the dial was moved after #4104 and the event carries its + stamp. The only case where the answer is known. + - `shipped` — the dial has never been moved, so the number in force is + the registry default, and the registry records what THAT + was measured against (`Surface.measured_model`). + - `unstamped` — the dial was moved before this step existed. It was + measured under something; naming it would be inventing a + fact, so `stale` is None rather than True or False. + "Unknown" and "fine" must not render the same. + + `model_changed` and `shape_changed` are reported apart (rule 149) because + they call for different responses: a new embedding model means every number + is a distance in a geometry that no longer exists, while a re-cut document + shape means the same records now embed different text. A caller that only + ever sees `stale: true` cannot tell those apart. + """ + if row is None: + model, shape, source = s.measured_model, s.measured_shape, "shipped" + elif row.embedding_model is None and row.shape_version is None: + return { + "source": "unstamped", "embedding_model": None, + "shape_version": None, "model_changed": None, + "shape_changed": None, "stale": None, + } + else: + model, shape, source = row.embedding_model, row.shape_version, "tuned" + + model_changed = model != live["embedding_model"] + shape_changed = shape != live["shape_version"] + return { + "source": source, + "embedding_model": model, + "shape_version": shape, + "model_changed": model_changed, + "shape_changed": shape_changed, + "stale": model_changed or shape_changed, + } + + async def current_settings(user_id: int) -> list[dict]: """Every tunable surface with its live pair and its last stated reason. @@ -94,7 +139,16 @@ async def current_settings(user_id: int) -> list[dict]: 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. + + From #4104 it also carries `calibration` per dial: the embedding model and + document shape the number in force was measured in, and whether either has + moved since. NOTHING AUTO-RETUNES on the strength of it. A stale stamp says + a number is a measurement of a space that no longer exists — it does not + say what the number should be now, and the one time a statistic was allowed + to answer that question it was wrong (see the module docstring). The stamp + is here so a reader knows which floors to go and re-measure. """ + live = calibration_stamp() out: list[dict] = [] async with async_session() as session: for name in surface_names(): @@ -126,6 +180,13 @@ async def current_settings(user_id: int) -> list[dict]: "last_change": { dial: last[dial].to_dict() for dial in DIALS if dial in last }, + # Always present for BOTH dials, unlike `last_change`: an + # untouched dial still has a number in force, and that number + # was still measured in some space. Reported, never acted on — + # see the note below on why nothing auto-retunes. + "calibration": { + dial: _calibration(last.get(dial), s, live) for dial in DIALS + }, }) return out @@ -179,9 +240,16 @@ async def set_dial( await set_setting(user_id, key, stored) async with async_session() as session: + # Stamped with the space this number was chosen in (#4104). Read at + # write time rather than passed in: the caller measuring a floor and + # the caller recording it are the same call, so there is no window in + # which they could disagree. + stamp = calibration_stamp() session.add(RetrievalTuningEvent( user_id=user_id, surface=surface, dial=dial, old_value=old, new_value=applied, actor=actor, reason=text, + embedding_model=stamp["embedding_model"], + shape_version=stamp["shape_version"], )) await session.commit() diff --git a/tests/test_calibration_stamp.py b/tests/test_calibration_stamp.py new file mode 100644 index 0000000..09434e6 --- /dev/null +++ b/tests/test_calibration_stamp.py @@ -0,0 +1,191 @@ +"""A tuned number carries the space it was measured in — and only that (#4104). + +WHY THIS EXISTS + +Milestone 416 step 6 answers *"a path for thresholds to be inherited by the next +version or different model so that they don't have to recalibrate a lot."* The +mechanism is a stamp: every tuning event records the embedding model and +document shape the number was chosen under, and a mismatch is reported rather +than left to be noticed. + +THE ONE THAT MATTERS MOST IS THE ABSENCE + +`test_no_chat_model_identifier_reaches_the_calibration_path` is the load-bearing +guard here, and it is a guard against a plausible mistake rather than a +hypothetical one. These floors live in BAAI/bge-small-en-v1.5's vector space. +The CHAT model — Claude — produces none of these scores and changes none of +them, so a Claude upgrade must trigger nothing at all. If it ever did, the +operator would be asked to recalibrate six dials for no reason, learn that this +surface cries wolf, and ignore it on the day `bge-small` → `bge-base` actually +moves every score on the install. A false alarm here does not cost a +notification; it costs the real alarm. + +The rest pins the honest-unknown behaviour: a row written before stamps existed +reports as `unstamped` with `stale: None`, never as fine and never as a model +name somebody guessed at. +""" +import re +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import embeddings as emb +from scribe.services import retrieval_tuning as rt +from scribe.services.retrieval_surfaces import SURFACES, get_surface +from tests.helpers import make_mock_session, tool_doc + +# Names for the thing that talks, as opposed to the thing that embeds. Any of +# these appearing in the calibration path means a chat-model change could move a +# stamp — see the module docstring for why that is the expensive failure. +_CHAT_MODEL_WORDS = ( + "claude", "opus", "sonnet", "haiku", "gpt", "llama", "mistral", "gemini", + "anthropic", "openai", "chat_model", "chatmodel", "model_version", +) + + +def test_calibration_stamp_names_the_embedder_and_the_shape(): + stamp = emb.calibration_stamp() + assert stamp == { + "embedding_model": emb.EMBEDDING_MODEL, + "shape_version": emb.CHUNKER_VERSION, + } + # Two keys, never one fused string (rule 149): a mismatch has to be able to + # say WHICH half moved, because a new embedder and a re-cut document + # invalidate the same numbers for different reasons. + assert len(stamp) == 2 + + +def test_the_stamp_tracks_the_embedding_model_constant_not_a_copy_of_it(): + """A second literal would drift, and drift here reads as a model change.""" + with patch.object(emb, "EMBEDDING_MODEL", "some/other-model"): + assert emb.calibration_stamp()["embedding_model"] == "some/other-model" + with patch.object(emb, "CHUNKER_VERSION", 99): + assert emb.calibration_stamp()["shape_version"] == 99 + + +def test_no_chat_model_identifier_reaches_the_calibration_path(): + """THE GUARD. A Claude upgrade must move nothing in this path. + + Structural rather than behavioural on purpose: the failure is someone + *adding* a chat-model field in good faith ("surely the model matters"), and + no behavioural test catches a field that has not been written yet. Asserted + over the source of every module that produces or consumes a stamp. + """ + import inspect + + from scribe.services import retrieval_migration as rm + + for label, obj in ( + ("calibration_stamp", emb.calibration_stamp), + ("_calibration", rt._calibration), + ("migrate_floor", rm.migrate_floor), + ): + src = inspect.getsource(obj).lower() + # Comments and docstrings here legitimately discuss the chat model in + # order to rule it out, so strip prose and assert on code only. + code = "\n".join( + line.split("#", 1)[0] + for line in re.sub(r'""".*?"""', "", src, flags=re.S).splitlines() + ) + for word in _CHAT_MODEL_WORDS: + assert word not in code, f"{label} names the chat model: {word!r}" + + +@pytest.mark.asyncio +async def test_a_moved_dial_is_written_with_the_live_stamp(): + session = make_mock_session() + with patch.object(rt, "async_session", MagicMock(return_value=session)), \ + patch.object(rt, "set_setting", AsyncMock()), \ + patch.object(rt, "floor_for", AsyncMock(return_value=0.72)), \ + patch.object(rt, "budget_for", AsyncMock(return_value=3)), \ + patch.object(rt, "calibration_stamp", + MagicMock(return_value={"embedding_model": "m/x", + "shape_version": 7})): + await rt.set_dial( + 1, "prompt_rule", "floor", 0.66, + reason="read the five refused records; four were genuine matches", + ) + event = session.add.call_args[0][0] + assert event.embedding_model == "m/x" + assert event.shape_version == 7 + + +def _row(model="BAAI/bge-small-en-v1.5", shape=1): + return MagicMock(embedding_model=model, shape_version=shape) + + +def test_an_untouched_dial_reports_the_shipped_default_it_is_still_on(): + s = get_surface("prompt_rule") + live = {"embedding_model": s.measured_model, "shape_version": s.measured_shape} + cal = rt._calibration(None, s, live) + assert cal["source"] == "shipped" + assert cal["stale"] is False + + +def test_a_dial_moved_before_stamps_existed_is_unknown_not_fine(): + """`stale: None`, because "we don't know" and "it's fine" are not the same. + + Collapsing them would hide the dials MOST likely to be wrong — the ones + somebody tuned longest ago. + """ + s = get_surface("prompt_rule") + cal = rt._calibration(_row(model=None, shape=None), s, emb.calibration_stamp()) + assert cal["source"] == "unstamped" + assert cal["stale"] is None + assert cal["model_changed"] is None and cal["shape_changed"] is None + + +def test_a_model_change_and_a_shape_change_are_reported_apart(): + """Rule 149. Two conditions, two answers — they call for different work.""" + s = get_surface("prompt_rule") + live = {"embedding_model": "new/model", "shape_version": 1} + cal = rt._calibration(_row(model="old/model", shape=1), s, live) + assert (cal["model_changed"], cal["shape_changed"], cal["stale"]) == ( + True, False, True, + ) + + live = {"embedding_model": "old/model", "shape_version": 2} + cal = rt._calibration(_row(model="old/model", shape=1), s, live) + assert (cal["model_changed"], cal["shape_changed"], cal["stale"]) == ( + False, True, True, + ) + + +def test_a_dial_tuned_under_the_live_stamp_is_not_stale(): + s = get_surface("prompt_rule") + live = emb.calibration_stamp() + cal = rt._calibration( + _row(model=live["embedding_model"], shape=live["shape_version"]), s, live, + ) + assert cal["source"] == "tuned" + assert cal["stale"] is False + + +def test_every_surface_ships_a_stamp_for_its_defaults(): + """A default with no stamp cannot be told from one measured yesterday.""" + for name, s in SURFACES.items(): + assert s.measured_model, f"{name} default has no measured model" + assert isinstance(s.measured_shape, int), f"{name} shape is not a version" + + +def test_the_registry_stamp_is_a_literal_not_the_live_value(): + """It says what WAS true, so it must not follow the current constants. + + A field that tracked `EMBEDDING_MODEL` would agree with it forever and could + never report the one thing it exists to report. + """ + with patch.object(emb, "EMBEDDING_MODEL", "some/other-model"): + assert get_surface("prompt_rule").measured_model != "some/other-model" + + +def test_the_tool_says_nothing_auto_retunes(): + """The contract an agent reads. A stale stamp is a prompt to MEASURE. + + Without this, the obvious next move on seeing `stale: true` is to move the + dial — which is tuning from a statistic, the exact failure #4102 measured + pointing the wrong way. + """ + doc = tool_doc("scribe.mcp.tools.retrieval_tuning", "retrieval_surfaces") + assert "calibration" in doc + assert "NOTHING IS RETUNED AUTOMATICALLY" in doc + assert "unstamped" in doc diff --git a/tests/test_retrieval_migration.py b/tests/test_retrieval_migration.py new file mode 100644 index 0000000..0856bea --- /dev/null +++ b/tests/test_retrieval_migration.py @@ -0,0 +1,153 @@ +"""Carrying a floor across a calibration change (#4104). + +WHAT THIS PINS + + 1. **The percentile is what transfers, not the number.** A floor's content is + a decision about selectivity; the cosine expressing it is units. So a + migration that admitted 30% before admits 30% after, whatever the new + scores look like. + 2. **Nothing is written unless asked twice.** `apply` defaults to False. A + model change makes every number uncertain at once, which is the worst + moment to let a statistic move six dials unattended. + 3. **Missing evidence is a refusal, not a guess.** No logged calls, or a + corpus that re-scores to nothing, returns `migrated: False` with a reason — + never a floor computed off an empty distribution, which is how an install + mid-backfill would end up with every bar at zero. + 4. **Every registry surface can be migrated.** A seventh arm that nobody adds + a re-scorer for is one whose floor silently cannot survive a model change. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import retrieval_migration as rm +from scribe.services.retrieval_surfaces import SURFACES +from tests.helpers import make_mock_session, tool_doc + + +def _logs(pairs): + """Rows as `migrate_floor` reads them: (query, project_id, old score).""" + return [MagicMock(query=q, project_id=p, best_available_score=s) + for q, p, s in pairs] + + +def _session_with(rows): + session = make_mock_session() + session.execute.return_value.all.return_value = rows + return session + + +def test_every_surface_has_a_rescorer(): + """Otherwise a surface's floor cannot cross a model change at all.""" + assert set(rm._RESCORERS) == set(SURFACES) + + +def test_the_floor_that_admits_a_fraction_is_an_observed_score(): + scores = [0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2, 0.1, 0.05] + # 30% of ten is three; the third-best score is the bar that admits exactly + # those three. Exact rather than interpolated, so the answer can be checked + # against the sample it came from. + assert rm._floor_admitting(scores, 0.3) == 0.7 + assert sum(1 for s in scores if s >= 0.7) == 3 + + +def test_a_surface_that_admitted_nothing_keeps_admitting_nothing(): + """A migration must not quietly reopen an arm the operator had shut.""" + scores = [0.5, 0.4, 0.3] + assert rm._floor_admitting(scores, 0.0) > max(scores) + + +@pytest.mark.asyncio +async def test_selectivity_is_preserved_across_a_scale_change(): + """The whole idea, on numbers that move a long way. + + Old scores cluster near 0.7 with a bar at 0.72 admitting two of five. New + scores sit far lower — a different geometry — and the proposal is the value + that admits two of five there, not anything resembling 0.72. + """ + rows = _logs([ + ("q1", 2, 0.80), ("q2", 2, 0.75), ("q3", 2, 0.70), + ("q4", 2, 0.60), ("q5", 2, 0.50), + ]) + new = {"q1": 0.42, "q2": 0.38, "q3": 0.31, "q4": 0.22, "q5": 0.10} + rescore = AsyncMock(side_effect=lambda u, q, p: new[q]) + + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.72)), \ + patch.dict(rm._RESCORERS, {"prompt_rule": rescore}): + out = await rm.migrate_floor(1, "prompt_rule") + + assert out["old_admit_rate"] == 0.4 # 0.80 and 0.75 cleared 0.72 + assert out["proposed_floor"] == 0.38 # admits 0.42 and 0.38 — also two + assert out["migrated"] is False # dry run by default + + +@pytest.mark.asyncio +async def test_a_dry_run_writes_nothing(): + rows = _logs([("q1", None, 0.9), ("q2", None, 0.1)]) + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock()) as set_dial, \ + patch.dict(rm._RESCORERS, {"prompt_rule": AsyncMock(return_value=0.4)}): + out = await rm.migrate_floor(1, "prompt_rule") + set_dial.assert_not_called() + assert "proposed_floor" in out + + +@pytest.mark.asyncio +async def test_applying_writes_an_ordinary_tuning_event_with_the_arithmetic(): + """A migrated floor is reviewable and revertible like any other change.""" + rows = _logs([("q1", None, 0.9), ("q2", None, 0.1)]) + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock(return_value={})) as set_dial, \ + patch.dict(rm._RESCORERS, {"prompt_rule": AsyncMock(return_value=0.4)}): + out = await rm.migrate_floor(1, "prompt_rule", apply=True) + + assert out["migrated"] is True + kwargs = set_dial.call_args.kwargs + reason = kwargs["reason"] + # The reason has to carry the working, not just the verdict — it is what the + # operator reads to decide whether to keep the number. + assert "0.5" in reason and "sampled" in reason + assert kwargs["actor"] == "model" + + +@pytest.mark.asyncio +async def test_no_logged_calls_refuses_rather_than_inventing_a_distribution(): + with patch.object(rm, "async_session", MagicMock(return_value=_session_with([]))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock()) as set_dial: + out = await rm.migrate_floor(1, "prompt_rule", apply=True) + assert out["migrated"] is False + assert "no logged calls" in out["why"] + set_dial.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_corpus_that_rescores_to_nothing_refuses(): + """The mid-backfill case: every bar would otherwise be set off no data.""" + rows = _logs([("q1", None, 0.9), ("q2", None, 0.8)]) + with patch.object(rm, "async_session", MagicMock(return_value=_session_with(rows))), \ + patch.object(rm, "floor_for", AsyncMock(return_value=0.5)), \ + patch.object(rm, "set_dial", AsyncMock()) as set_dial, \ + patch.dict(rm._RESCORERS, {"prompt_rule": AsyncMock(return_value=None)}): + out = await rm.migrate_floor(1, "prompt_rule", apply=True) + assert out["migrated"] is False + assert "not embedded" in out["why"] + set_dial.assert_not_called() + + +@pytest.mark.asyncio +async def test_an_unknown_surface_is_refused(): + with pytest.raises(ValueError): + await rm.migrate_floor(1, "promptrule") + + +def test_the_tool_says_it_is_a_starting_point_and_defaults_to_a_dry_run(): + doc = tool_doc("scribe.mcp.tools.retrieval_tuning", "migrate_retrieval_floor") + assert "DRY RUN BY DEFAULT" in doc + # Percentile-preserving carries the old floor's wrongness forward faithfully. + # A reader who misses that will treat a migrated number as a measured one. + assert "STARTING POINT" in doc.upper() + assert "stale" in doc diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index a74ac45..40ed6f2 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 == 16 + assert backup.BACKUP_VERSION == 17 def _exportable_note(**over): -- 2.54.0 From def144df06ecce4f864864d9f0b604f15ac591d6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 12:47:57 -0400 Subject: [PATCH 4/5] fix(tests): a Result's .all() is sync, and the module has a fourth tool (#4104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failures in the tests added with the step, both mine. `make_mock_session` is an AsyncMock and every child of an AsyncMock is one too, so leaving `.all` as it came handed `migrate_floor` a coroutine where it reads a list — the same trap the helper's own docstring already flags for `add`. And the registration test enumerated three tools by name, which is exactly what it is for: `migrate_retrieval_floor` made it four. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_retrieval_migration.py | 8 +++++++- tests/test_retrieval_tuning.py | 7 +++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/test_retrieval_migration.py b/tests/test_retrieval_migration.py index 0856bea..e46820e 100644 --- a/tests/test_retrieval_migration.py +++ b/tests/test_retrieval_migration.py @@ -32,8 +32,14 @@ def _logs(pairs): def _session_with(rows): + """`.all()` is SYNCHRONOUS on a Result, so it needs a MagicMock. + + `make_mock_session` is an AsyncMock, and every child of an AsyncMock is one + too — leaving `.all` as it comes hands the service a coroutine where it + expects a list, the same trap the helper's docstring flags for `add`. + """ session = make_mock_session() - session.execute.return_value.all.return_value = rows + session.execute.return_value = MagicMock(all=MagicMock(return_value=rows)) return session diff --git a/tests/test_retrieval_tuning.py b/tests/test_retrieval_tuning.py index 0c0ac5c..d55169f 100644 --- a/tests/test_retrieval_tuning.py +++ b/tests/test_retrieval_tuning.py @@ -183,13 +183,16 @@ def test_the_tool_teaches_reading_the_records_not_the_percentile(): assert "69" in doc -def test_all_three_tools_are_registered(): +def test_every_tool_in_the_module_is_registered(): from scribe.mcp.tools import retrieval_tuning as tool from tests.helpers import FakeMCP mcp = FakeMCP() tool.register(mcp) + # Order is the module's, and asserted rather than sorted: an unregistered + # tool is invisible to every caller, so the list is worth reading literally. assert mcp.names == [ - "retrieval_surfaces", "tune_retrieval", "retrieval_tuning_history", + "retrieval_surfaces", "migrate_retrieval_floor", + "tune_retrieval", "retrieval_tuning_history", ] -- 2.54.0 From a7d736860fa5bd2e51f0ac6b576d98cbe1258c11 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 17 Sep 2026 12:49:15 -0400 Subject: [PATCH 5/5] fix(retrieval): the newest two rows are not the newest row of each dial (#4104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `current_settings` read a surface's history with one query and `limit(2)`, then keyed the rows by dial. That is only the same thing while both dials have moved equally often — and they do not. Floors get walked; budgets rarely move. Three floor changes and one budget change returns two floor rows, and the budget change vanishes. Under #4102 that cost a missing reason. Since #4104 it is worse: the dial then reports `source: "shipped"` — still on the value Scribe shipped — for a number somebody deliberately tuned. A wrong calibration answer, in the direction a reader has no cause to double-check. One query per dial, `limit(1)` each. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/retrieval_tuning.py | 34 ++++++++++++------ tests/test_calibration_stamp.py | 47 +++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/src/scribe/services/retrieval_tuning.py b/src/scribe/services/retrieval_tuning.py index 787f166..f6a8682 100644 --- a/src/scribe/services/retrieval_tuning.py +++ b/src/scribe/services/retrieval_tuning.py @@ -153,18 +153,30 @@ async def current_settings(user_id: int) -> 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, + # ONE QUERY PER DIAL, not one `limit(len(DIALS))` over both. + # "The newest two rows" is not "the newest row of each kind": a + # surface whose floor was moved three times and whose budget was + # moved once returns two floor rows, and the budget change + # disappears. That was a missing reason when this only fed + # `last_change`; since #4104 it is also a WRONG calibration answer — + # a tuned dial reporting as "still on the shipped default", which is + # the one state a reader would not think to check. + last = {} + for dial in DIALS: + row = ( + await session.execute( + select(RetrievalTuningEvent) + .where( + RetrievalTuningEvent.surface == name, + RetrievalTuningEvent.user_id == user_id, + RetrievalTuningEvent.dial == dial, + ) + .order_by(RetrievalTuningEvent.created_at.desc()) + .limit(1) ) - .order_by(RetrievalTuningEvent.created_at.desc()) - .limit(len(DIALS)) - ) - ).scalars().all() - last = {r.dial: r for r in rows} + ).scalars().first() + if row is not None: + last[dial] = row out.append({ "surface": name, "floor": await floor_for(user_id, name), diff --git a/tests/test_calibration_stamp.py b/tests/test_calibration_stamp.py index 09434e6..6e2d530 100644 --- a/tests/test_calibration_stamp.py +++ b/tests/test_calibration_stamp.py @@ -189,3 +189,50 @@ def test_the_tool_says_nothing_auto_retunes(): assert "calibration" in doc assert "NOTHING IS RETUNED AUTOMATICALLY" in doc assert "unstamped" in doc + + +@pytest.mark.asyncio +async def test_a_dial_is_read_per_dial_not_from_the_newest_two_rows(): + """The floor's history must not be able to bury the budget's. + + "The newest two rows" and "the newest row of each dial" differ the moment + one dial moves more often than the other — which is the normal case, since + floors get walked and budgets rarely do. Before this was one query per dial, + a surface with three floor changes and one budget change reported the budget + as untouched: a WRONG calibration answer rather than a missing one, and + wrong in the direction a reader would not think to check. + """ + session = make_mock_session() + per_dial = { + "floor": MagicMock(dial="floor", embedding_model="old/model", + shape_version=1, + to_dict=MagicMock(return_value={"dial": "floor"})), + "budget": MagicMock(dial="budget", embedding_model="old/model", + shape_version=1, + to_dict=MagicMock(return_value={"dial": "budget"})), + } + calls = [] + + def execute(stmt): + # The dial is whichever the WHERE clause names; a query that did not + # scope by dial would render this stand-in unable to answer, which is + # the point. + sql = str(stmt.compile(compile_kwargs={"literal_binds": True})) + dial = "budget" if "'budget'" in sql else "floor" + calls.append(dial) + result = MagicMock() + result.scalars.return_value.first.return_value = per_dial[dial] + return result + + session.execute = AsyncMock(side_effect=execute) + with patch.object(rt, "async_session", MagicMock(return_value=session)), \ + patch.object(rt, "floor_for", AsyncMock(return_value=0.7)), \ + patch.object(rt, "budget_for", AsyncMock(return_value=3)): + out = await rt.current_settings(1) + + assert calls.count("floor") == len(SURFACES) + assert calls.count("budget") == len(SURFACES) + for row in out: + # Both dials tuned under a model that is not the live one. + assert row["calibration"]["budget"]["source"] == "tuned" + assert row["calibration"]["budget"]["stale"] is True -- 2.54.0