diff --git a/src/scribe/mcp/tools/__init__.py b/src/scribe/mcp/tools/__init__.py index e8b0b1f..f292d41 100644 --- a/src/scribe/mcp/tools/__init__.py +++ b/src/scribe/mcp/tools/__init__.py @@ -5,14 +5,15 @@ to a FastMCP instance. `register_all(mcp)` is the single entry point called from `mcp.server.build_mcp_server`. """ from scribe.mcp.tools import ( - design_systems, milestones, notes, processes, projects, recent, repos, rulebooks, search, shapes, - snippets, systems, tags, tasks, trash, + design_systems, milestones, notes, processes, projects, recent, repos, retrieval_tuning, + rulebooks, search, shapes, snippets, systems, tags, tasks, trash, ) def register_all(mcp) -> None: """Register every tool module's tools on the given FastMCP instance.""" search.register(mcp) + retrieval_tuning.register(mcp) notes.register(mcp) tasks.register(mcp) projects.register(mcp) diff --git a/src/scribe/mcp/tools/retrieval_tuning.py b/src/scribe/mcp/tools/retrieval_tuning.py new file mode 100644 index 0000000..f4594e0 --- /dev/null +++ b/src/scribe/mcp/tools/retrieval_tuning.py @@ -0,0 +1,115 @@ +"""Reading and moving a retrieval surface's floor and budget (#4102). + +The write half of the loop `retrieval_telemetry` opens. That tool says what a +bar did; these say what the bar IS, and let it be changed with the argument +attached. +""" +from scribe.mcp._context import current_user_id +from scribe.services import retrieval_tuning as tuning_svc + + +async def retrieval_surfaces() -> dict: + """Every retrieval surface Scribe pushes on, with its floor and its budget. + + Read this before changing either, and read it beside + `retrieval_telemetry(days=…, near_miss_samples=5)` — this tool says what is + in force, that one says what it did. + + Each surface carries what it ASKS, what corpus it asks OVER, and how often + it FIRES, because a floor cannot be moved sensibly without all three. An arm + firing before every Bash call is spending attention on a scale an arm firing + once a turn is not, and the same number means different things to a query + that is a shell command, a code payload, or an operator's sentence. + + `floor` is a COST floor: is this worth ranking at all. It is not a relevance + judgement — relevance is decided by the reader, which is the only + participant that can read a record's trigger against the actual situation, + and every injected line says so out loud ("read it before deciding it does + not apply"). + + `budget` is what actually binds. It is how many lines this surface may spend + on one injection, and it is the control to reach for when a surface feels + noisy — lowering a budget removes the weakest candidates, while raising a + floor removes whichever candidates happen to sit under a number. + + `last_change` carries the reason last given for each dial, or is absent when + the surface is still on its shipped starting point. Those starting points + are starting points: they were measured against one corpus with one + embedding model and cannot be right for another install by construction. + That is why this tool exists rather than a better set of defaults. + """ + return {"surfaces": await tuning_svc.current_settings(current_user_id())} + + +async def tune_retrieval( + surface: str, dial: str, value: float, reason: str, actor: str = "model", +) -> dict: + """Move one surface's floor or budget, recording why. + + DO NOT CALL THIS FROM A PERCENTILE ALONE. `retrieval_telemetry` gives + `near_misses.p90` — the mass sitting just under the bar — and that number + says nothing about whether the mass is RELEVANT. The two have been measured + disagreeing: one surface logged 69 consecutive declines with the refused + record 0.0006 under its bar, and every percentile said "lower it". The + refused record turned out to be a rule about interpreting a REQUEST, matched + against a query about report layout — a false positive. Lowering would have + attached that rule to every completion report ever written. The statistic + and the correct action pointed in opposite directions, and only opening the + record could tell. + + So the procedure is: `retrieval_telemetry(near_miss_samples=5)`, then + `get_rule` / `get_note` the `record_id`s it names, and decide whether those + records SHOULD have surfaced for those queries. Then move the dial, and say + in `reason` what you read and what it showed. + + `reason` is required and must be substantive. It is the guardrail, not + bookkeeping: it is what lets the operator review a change they did not make, + disagree with it, and revert it. A number that moved with no stated basis is + one nobody can audit — including you, next week. + + WHICH DIAL. Reach for `budget` when the complaint is volume, and `floor` + when the complaint is quality. Raising a floor to quieten a surface throws + away its best candidates along with its worst, because a floor cannot tell + rank from relevance; lowering a budget keeps the top of the ranking and + drops the tail, which is usually what was wanted. + + Args: + surface: the surface name from `retrieval_surfaces` — also its + `retrieval_telemetry` source, so the two always name the same arm. + dial: "floor" or "budget". + value: the new value. A floor is clamped to [0, 1], a budget to a whole + number in [1, 10]. The result says whether a clamp bit, which + matters: believing you set a value the registry corrected means + reading the next telemetry as evidence about a bar never in force. + reason: what you read and what it showed. Required. + actor: "model" (default) or "human" — pass "human" only when relaying a + value the operator chose themselves, so the audit trail can tell + a change they made from one made on their behalf. + """ + return await tuning_svc.set_dial( + current_user_id(), surface, dial, value, reason=reason, actor=actor, + ) + + +async def retrieval_tuning_history(surface: str = "", limit: int = 20) -> dict: + """What has been changed about retrieval on this install, newest first. + + Reach for it before moving a dial that has been moved before — the previous + reason is the argument the next change has to answer, and a surface that has + been walked up and down repeatedly is evidence the floor is not the problem. + + Args: + surface: restrict to one surface; omit for everything. + limit: how many events, default 20, capped at 200. + """ + return { + "events": await tuning_svc.tuning_history( + current_user_id(), surface=surface or None, limit=limit, + ) + } + + +def register(mcp) -> None: + mcp.tool(name="retrieval_surfaces")(retrieval_surfaces) + mcp.tool(name="tune_retrieval")(tune_retrieval) + mcp.tool(name="retrieval_tuning_history")(retrieval_tuning_history) diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 1ad21d5..5a9d135 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -13,6 +13,7 @@ from scribe.models.rule_version import RuleVersion from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.note_usage import NoteUsageEvent from scribe.models.rule_usage import RuleUsageEvent +from scribe.models.retrieval_tuning import RetrievalTuningEvent from scribe.models.canonical_system import CanonicalSystem from scribe.models.rulebook import RuleRelation, rule_systems as rule_systems_t from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse @@ -67,8 +68,14 @@ logger = logging.getLogger(__name__) # topic_suppressions with their tables (milestone 414): a rule's scope is its # home now. Older archives carrying those sections still restore — the keys are # simply not read — as do the subscribe_rulebooks inception choices they hold. +# v16 (2026-09) added retrieval_tuning_events (milestone 416): the REASON each +# retrieval floor and budget is where it is. `settings` already carried the +# numbers, so leaving this behind would restore six moved dials with the +# argument for them silently dropped — and from this step on those dials are +# moved by the model, which is exactly the case where the operator needs the +# argument to review. # Bump when the serialized schema changes. -BACKUP_VERSION = 15 +BACKUP_VERSION = 16 # Every table this backup carries, by its REAL name. Paired with _NOT_INCLUDED # below, these two lists must together account for the entire schema — which is @@ -100,6 +107,12 @@ _BACKED_UP = [ # accumulated, so a restore that dropped it would silently reset the # measurement to zero while everything still looked fine. "rule_usage_events", + # v16 (2026-09): why each retrieval floor and budget is where it is + # (milestone 416). The values live in `settings` and already travelled; the + # argument for them had nowhere to go. Now that the model moves these dials + # on the operator's behalf, a restore that kept the numbers and dropped the + # reasons would leave an install tuned by nobody it can name. + "retrieval_tuning_events", ] # Tables intentionally NOT in the backup, surfaced in the payload so the gap is @@ -189,6 +202,9 @@ _COLUMN_EXCLUSIONS: dict[str, set[str]] = { "note_usage_events": {"id"}, # Same as the note twin: the surrogate key is re-issued on insert. "rule_usage_events": {"id"}, + # Same again — and everything else travels, because each remaining column + # is part of the argument: what moved, from what, to what, by whom, why. + "retrieval_tuning_events": {"id"}, "design_systems": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, "design_tokens": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, "repo_bindings": {"id", "created_at", "updated_at"}, @@ -329,6 +345,25 @@ def _rule_usage_event_rows(rows) -> list[dict]: ] +def _retrieval_tuning_event_rows(rows) -> list[dict]: + """The record of why a retrieval dial is where it is (#4102). + + Not `to_dict()`: that method serves the MCP reader, which already knows + whose install it is asking about, so it omits `user_id`. A restore has to + remap it, and a row that arrived without it would have to be dropped or + guessed at. + """ + return [ + { + "user_id": r.user_id, "surface": r.surface, "dial": r.dial, + "old_value": r.old_value, "new_value": r.new_value, + "actor": r.actor, "reason": r.reason, + "created_at": r.created_at.isoformat() if r.created_at else None, + } + for r in rows + ] + + def _code_shape_rows(rows) -> list[dict]: return [r.to_dict() for r in rows] @@ -617,6 +652,12 @@ async def export_full_backup() -> dict: rule_usage_events = ( await session.execute(select(RuleUsageEvent)) ).scalars().all() + # Oldest first, so a restored history reads in the order the dials + # actually moved — the sequence IS the argument when a surface has been + # walked up and down. + retrieval_tuning_events = (await session.execute( + select(RetrievalTuningEvent).order_by(RetrievalTuningEvent.id) + )).scalars().all() repo_bindings = (await session.execute(select(RepoBinding))).scalars().all() code_shapes = (await session.execute(select(CodeShape))).scalars().all() code_shape_events = (await session.execute( @@ -661,6 +702,9 @@ async def export_full_backup() -> dict: "design_tokens": _design_token_rows(design_tokens), "note_usage_events": _usage_event_rows(usage_events), "rule_usage_events": _rule_usage_event_rows(rule_usage_events), + "retrieval_tuning_events": _retrieval_tuning_event_rows( + retrieval_tuning_events + ), "repo_bindings": _repo_binding_rows(repo_bindings), "note_supersessions": _note_supersession_rows(supersessions), "code_shapes": _code_shape_rows(code_shapes), @@ -795,6 +839,15 @@ async def export_user_backup(user_id: int) -> dict: rule_usage_events = (await session.execute( select(RuleUsageEvent).where(RuleUsageEvent.rule_id.in_(_rule_ids)) )).scalars().all() if _rule_ids else [] + # Scoped on user_id, and here that IS the right column — unlike the + # rule usage events directly above. These record changes to this user's + # OWN retrieval settings, which is what `user_id` means on this table; + # there is no second owner to route around. + retrieval_tuning_events = (await session.execute( + select(RetrievalTuningEvent) + .where(RetrievalTuningEvent.user_id == user_id) + .order_by(RetrievalTuningEvent.id) + )).scalars().all() rule_relations = (await session.execute( select(RuleRelation).where( RuleRelation.from_rule_id.in_(_rule_ids), @@ -836,6 +889,9 @@ async def export_user_backup(user_id: int) -> dict: "design_tokens": _design_token_rows(design_tokens), "note_usage_events": _usage_event_rows(usage_events), "rule_usage_events": _rule_usage_event_rows(rule_usage_events), + "retrieval_tuning_events": _retrieval_tuning_event_rows( + retrieval_tuning_events + ), "repo_bindings": _repo_binding_rows(repo_bindings), "note_supersessions": _note_supersession_rows(supersessions), "code_shapes": _code_shape_rows(code_shapes), @@ -975,6 +1031,7 @@ async def _restore_v2(data: dict) -> dict: "note_supersessions": 0, "code_shapes": 0, "code_shape_events": 0, "code_shape_uses": 0, "canonical_systems": 0, "rule_systems": 0, "rule_relations": 0, "rule_versions": 0, + "retrieval_tuning_events": 0, } async with async_session() as session: @@ -1169,6 +1226,31 @@ async def _restore_v2(data: dict) -> dict: session.add(Setting(user_id=mapped_uid, key=s_data["key"], value=s_data.get("value", ""))) stats["settings"] += 1 + # 8b. Retrieval tuning history (v16) — restored beside the settings it + # explains, and for the same reason: the numbers above are the state, + # these rows are the argument for it. From milestone 416 those dials + # move on the operator's behalf, so an install restored with the values + # and without the reasons is one tuned by nobody it can name. + # + # No id remapping beyond the user: `surface` is a registry NAME, not a + # foreign key, which is what lets this history survive a restore into + # an install whose row ids all differ. + for t_data in data.get("retrieval_tuning_events", []): + mapped_uid = user_id_map.get(t_data.get("user_id") or 0) + if mapped_uid is None: + continue + session.add(RetrievalTuningEvent( + user_id=mapped_uid, + surface=t_data.get("surface", ""), + dial=t_data.get("dial", ""), + old_value=t_data.get("old_value"), + new_value=t_data.get("new_value"), + actor=t_data.get("actor") or "model", + reason=t_data.get("reason", ""), + created_at=_dt(t_data.get("created_at")), + )) + stats["retrieval_tuning_events"] += 1 + # 9. Rulebooks (v3) for rb_data in data.get("rulebooks", []): mapped_uid = user_id_map.get(rb_data.get("owner_user_id", 0)) diff --git a/tests/test_retrieval_tuning.py b/tests/test_retrieval_tuning.py new file mode 100644 index 0000000..733315d --- /dev/null +++ b/tests/test_retrieval_tuning.py @@ -0,0 +1,199 @@ +"""Moving a floor, and the reason that has to come with it (#4102). + +WHY THIS EXISTS + +The operator handed the dial to the model: *"the user should be able to touch it +but the model should be the thing handling it 9 times out of 10."* Everything +here guards the half of that sentence people skip — the operator still has to be +able to see what was done on their behalf and disagree with it. + +WHAT THIS PINS + + 1. **A reason is required, and "" does not count.** The column is NOT NULL, + which an empty string satisfies; the service is where the requirement is + real. This is the whole guardrail: a caller who must write down why has to + have looked, and a caller who writes down something wrong leaves the + operator a sentence to argue with. A number that moved silently leaves + nothing. + 2. **The change is recorded, with what it was before.** Without `old_value` a + history cannot answer "was this always like that", which is the first + question anyone asks of a surface behaving oddly. + 3. **A clamp is reported, never swallowed.** A caller that believes it set 1.4 + will read the next telemetry as evidence about a bar that was never in + force — the same class of error as #3739, one layer up. + 4. **An unknown surface is refused before anything is written.** Settings keys + are free-form strings, so a typo'd surface would write a key nothing reads: + a change that reports success and alters nothing. + 5. **The tool teaches the procedure that works**, not the one the numbers + suggest. Asserted on the docstring because the docstring IS the contract an + agent reads, and the failure it prevents is a model tuning from a + percentile — which has been measured pointing the wrong way. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import retrieval_tuning as rt +from tests.helpers import make_mock_session + + +def _patches(floor=0.72, budget=3): + """Patch everything `set_dial` touches except the thing under test.""" + session = make_mock_session() + return session, ( + patch.object(rt, "async_session", MagicMock(return_value=session)), + patch.object(rt, "set_setting", AsyncMock()), + patch.object(rt, "floor_for", AsyncMock(return_value=floor)), + patch.object(rt, "budget_for", AsyncMock(return_value=budget)), + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("reason", ["", " ", "noisy", "too high"]) +async def test_a_change_without_a_real_reason_is_refused(reason): + """The guardrail. "too high" is a restatement of the change, not a basis.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError) as e: + await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason=reason) + # The message has to name the tool that produces a real basis, or the + # caller's next move is a longer sentence rather than a look at the records. + assert "near_miss_samples" in str(e.value) + session.add.assert_not_called() + + +@pytest.mark.asyncio +async def test_nothing_is_written_when_the_reason_is_refused(): + """Refused BEFORE the setting is touched, not after. + + Writing the value and then raising would leave the number moved and the + history empty — the exact state this table exists to make impossible. + """ + session, ctx = _patches() + with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \ + ctx[2], ctx[3]: + with pytest.raises(ValueError): + await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason="x") + setter.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_good_change_writes_the_setting_and_the_event(): + session, ctx = _patches(floor=0.72) + reason = ("read prompt_rule's 5 highest declines: 3 were project rules for " + "another repo, so the bar is doing its job here") + with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \ + ctx[2], ctx[3]: + out = await rt.set_dial(1, "prompt_rule", "floor", 0.66, reason=reason) + + setter.assert_awaited_once() + _uid, key, stored = setter.await_args.args + assert key == "kb_promptrule_threshold" and stored == "0.66" + + event = session.add.call_args.args[0] + assert event.surface == "prompt_rule" and event.dial == "floor" + # The before-value is what makes the history answerable. + assert event.old_value == 0.72 and event.new_value == 0.66 + assert event.reason == reason and event.actor == "model" + assert out["previous"] == 0.72 and out["applied"] == 0.66 + assert out["clamped"] is False + + +@pytest.mark.asyncio +@pytest.mark.parametrize("dial, sent, applied", [ + ("floor", 1.4, 1.0), + ("floor", -0.2, 0.0), + ("budget", 99, 10), + ("budget", 0, 1), +]) +async def test_a_clamp_is_reported_rather_than_swallowed(dial, sent, applied): + """Said out loud, because silence here poisons the next reading. + + A caller that believes it set 1.4 treats the following week's telemetry as + evidence about a bar that never existed, and then moves the dial again to + fix a problem it invented. + """ + session, ctx = _patches() + reason = "checked the refused records for this surface and they were fine" + with ctx[0], ctx[1], ctx[2], ctx[3]: + out = await rt.set_dial(1, "auto_inject", dial, sent, reason=reason) + assert out["applied"] == applied + assert out["clamped"] is True + + +@pytest.mark.asyncio +async def test_an_unknown_surface_is_refused_before_anything_is_written(): + session, ctx = _patches() + with ctx[0], patch.object(rt, "set_setting", AsyncMock()) as setter, \ + ctx[2], ctx[3]: + with pytest.raises(ValueError): + await rt.set_dial(1, "pretool_rule", "floor", 0.6, + reason="a perfectly good reason that is long enough") + setter.assert_not_called() + session.add.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("dial", ["threshold", "limit", "", "FLOOR"]) +async def test_an_unknown_dial_is_refused(dial): + """The near-misses are the old vocabulary — `threshold` and `limit` are what + these were called before this step, so they are exactly what a stale caller + will send, and a silent no-op there would be indistinguishable from a + change that did not take.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError): + await rt.set_dial(1, "auto_inject", dial, 0.6, + reason="a perfectly good reason that is long enough") + session.add.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_human_change_is_distinguishable_from_the_models(): + """Both act as the same user, so the id cannot tell them apart — and "did I + do this, or did the session?" is the first question the history is asked.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3]: + await rt.set_dial(1, "auto_inject", "floor", 0.6, actor="human", + reason="operator set this themselves in Settings") + assert session.add.call_args.args[0].actor == "human" + + +@pytest.mark.asyncio +async def test_an_unknown_actor_is_refused(): + """Free text here would make the column unreadable within a month.""" + session, ctx = _patches() + with ctx[0], ctx[1], ctx[2], ctx[3], pytest.raises(ValueError): + await rt.set_dial(1, "auto_inject", "floor", 0.6, actor="agent", + reason="a perfectly good reason that is long enough") + + +def test_the_tool_teaches_reading_the_records_not_the_percentile(): + """The contract an agent actually reads (rule 167). + + The failure this prevents is a model moving a bar because `near_misses.p90` + sat close to it. That has been measured pointing the wrong way — 69 declines + where every percentile said "lower it" and the refused record was a false + positive — so the docstring has to carry the method, not just the warning. + """ + from scribe.mcp.tools import retrieval_tuning as tool + + doc = tool.tune_retrieval.__doc__ + assert "near_miss_samples" in doc, "the tool does not name how to get records" + assert "PERCENTILE ALONE" in doc.upper() + # And the worked example, because an abstract warning loses to a number. + assert "69" in doc + + +def test_all_three_tools_are_registered(): + from scribe.mcp.tools import retrieval_tuning as tool + + names = [] + + class _MCP: + def tool(self, name): + names.append(name) + return lambda fn: fn + + tool.register(_MCP()) + assert names == [ + "retrieval_surfaces", "tune_retrieval", "retrieval_tuning_history", + ] diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index 2fda52e..a74ac45 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -23,7 +23,7 @@ def test_backup_version_is_current(): (Named for the number it asserted until v10, which is exactly the drift a name-carrying-a-value invites; it now says what it checks.)""" - assert backup.BACKUP_VERSION == 15 + assert backup.BACKUP_VERSION == 16 def _exportable_note(**over): @@ -134,6 +134,7 @@ def _column_guard_targets(): from scribe.models.note_supersession import NoteSupersession from scribe.models.note_usage import NoteUsageEvent from scribe.models.rule_usage import RuleUsageEvent + from scribe.models.retrieval_tuning import RetrievalTuningEvent from scribe.models.note_version import NoteVersion from scribe.models.rule_version import RuleVersion from scribe.models.project import Project @@ -164,6 +165,9 @@ def _column_guard_targets(): "rule_relations": (RuleRelation, backup._rule_relation_rows), "note_usage_events": (NoteUsageEvent, backup._usage_event_rows), "rule_usage_events": (RuleUsageEvent, backup._rule_usage_event_rows), + "retrieval_tuning_events": ( + RetrievalTuningEvent, backup._retrieval_tuning_event_rows, + ), "design_systems": (DesignSystem, backup._design_system_rows), "design_tokens": (DesignToken, backup._design_token_rows), "repo_bindings": (RepoBinding, backup._repo_binding_rows), @@ -340,7 +344,9 @@ async def test_export_full_backup_contains_every_declared_section(): "systems", "record_systems", "design_systems", "design_tokens", "note_usage_events", "repo_bindings", "note_supersessions", "code_shapes", "code_shape_events", - "code_shape_uses"): + "code_shape_uses", + # v16: the reasons beside the settings they explain. + "retrieval_tuning_events"): assert key in out, f"missing export section: {key}" assert out[key] == []