diff --git a/alembic/versions/0103_retrieval_tuning_events.py b/alembic/versions/0103_retrieval_tuning_events.py new file mode 100644 index 0000000..7dc69ac --- /dev/null +++ b/alembic/versions/0103_retrieval_tuning_events.py @@ -0,0 +1,75 @@ +"""retrieval_tuning_events — why a floor is where it is (#4102) + +Revision ID: 0103 +Revises: 0102 +Create Date: 2026-09-17 + +Milestone 416 stops shipping similarity thresholds as values somebody has to +defend, and hands the adjustment to the model that reads the surface's own +telemetry. The operator's decision: + + "the floor should be chosen and adjusted by the model using it… the user + should be able to touch it but the model should be the thing handling it 9 + times out of 10." + +The number itself already has a home — the generic settings table. What has no +home is the ARGUMENT, and once the values move on their own the argument is the +part an operator needs: what changed, from what to what, who moved it, and on +what evidence. This table is that trail, and it is what makes the delegation +reviewable rather than merely automatic. + +Nothing is backfilled. A surface with no rows here is sitting on its shipped +starting point, which is a true and useful thing for the history to say. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0103" +down_revision = "0102" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "retrieval_tuning_events", + sa.Column("id", sa.Integer(), primary_key=True), + sa.Column( + "created_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + # FK-free, like retrieval_logs and app_logs: the record of why a number + # is where it is must outlive the account that moved it. + sa.Column("user_id", sa.Integer(), nullable=True), + # Also the surface's `retrieval_logs.source`, so a change can be read + # next to what the change did. + sa.Column("surface", sa.Text(), nullable=False), + sa.Column("dial", sa.Text(), nullable=False), + # Nullable: the first change to a surface has no stored predecessor. It + # moved off the shipped starting point, which is a different event from + # moving off a value somebody chose. + sa.Column("old_value", sa.Float(), nullable=True), + sa.Column("new_value", sa.Float(), nullable=False), + sa.Column( + "actor", sa.Text(), nullable=False, server_default=sa.text("'model'") + ), + # Non-null here; non-BLANK is enforced at the service boundary, because + # a column that merely forbids NULL is satisfied by "" and a required + # field that accepts "" is a formality. + sa.Column("reason", sa.Text(), nullable=False), + ) + # The only read this table has: one surface's history, newest first. + op.create_index( + "ix_retrieval_tuning_surface_created", + "retrieval_tuning_events", + ["surface", sa.text("created_at DESC")], + ) + + +def downgrade() -> None: + op.drop_index( + "ix_retrieval_tuning_surface_created", table_name="retrieval_tuning_events" + ) + op.drop_table("retrieval_tuning_events") diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index f522011..aa91587 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -27,6 +27,7 @@ from scribe.models.password_reset import PasswordResetToken # noqa: E402, F401 from scribe.models.invitation import InvitationToken # noqa: E402, F401 from scribe.models.embedding import MilestoneEmbedding, NoteEmbedding, RuleEmbedding # noqa: E402, F401 from scribe.models.retrieval_log import RetrievalLog # noqa: E402, F401 +from scribe.models.retrieval_tuning import RetrievalTuningEvent # noqa: E402, F401 from scribe.models.note_usage import NoteUsageEvent # noqa: E402, F401 from scribe.models.rule_usage import RuleUsageEvent # noqa: E402, F401 from scribe.models.project import Project # noqa: E402, F401 diff --git a/src/scribe/models/retrieval_tuning.py b/src/scribe/models/retrieval_tuning.py new file mode 100644 index 0000000..b08d2c7 --- /dev/null +++ b/src/scribe/models/retrieval_tuning.py @@ -0,0 +1,97 @@ +from datetime import datetime, timezone + +from sqlalchemy import DateTime, Float, Index, Integer, Text +from sqlalchemy.orm import Mapped, mapped_column + +from scribe.models import Base + + +class RetrievalTuningEvent(Base): + """One row per change to a retrieval surface's floor or budget (#4102). + + WHY A TABLE AND NOT JUST THE SETTING + + The setting holds the number. It cannot hold the argument, and from this + step onward the argument is the interesting part: the operator's decision is + that the model moves these values, *"9 times out of 10"*, without being + asked each time. Delegation like that is only safe if it leaves a trail the + operator can read afterwards — what changed, from what to what, and on what + evidence — and can disagree with. + + So `reason` is not decoration and not optional. It is the guardrail: a model + that has to state why is a model that has to have looked, and a reason that + turns out to be wrong is a reason someone can point at. A bare number in a + settings row records that something moved and nothing about whether it + should have. + + WHY `actor` IS NOT JUST `user_id` + + Both a person and a model act as the same user — the MCP tools run under the + operator's own id — so the id cannot distinguish them. That distinction is + exactly what an operator scanning this table wants first: "did I do this, or + did the session?" A change the operator made themselves needs no review; one + made on their behalf might. + + FK-FREE ON user_id, like `RetrievalLog` and `AppLog` above it: the history of + how a surface was tuned should outlive the account that tuned it, and a + deleted user must not cascade away the record of why a number is where it is. + """ + + __tablename__ = "retrieval_tuning_events" + + id: Mapped[int] = mapped_column(primary_key=True) + # Declared here rather than via CreatedAtMixin because the index below + # orders on `created_at.desc()`, which needs the column object in this class + # body — a mixin's column is not in scope there. + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) + ) + user_id: Mapped[int | None] = mapped_column(Integer, nullable=True) + + # The surface's registry name, which is also its `retrieval_logs.source`. + # That identity is what lets a reader put a change next to what the change + # did; `tests/test_retrieval_surfaces.py` asserts it rather than trusting it. + surface: Mapped[str] = mapped_column(Text, nullable=False) + + # "floor" or "budget". Text rather than an enum because rule 36 makes a new + # CHECK value a migration, and this is a young surface that may well grow a + # third tunable — a constraint bought nothing here except a future migration. + dial: Mapped[str] = mapped_column(Text, nullable=False) + + # Both stored as float so one pair of columns serves both dials; a budget is + # a whole number and reads back as one. `old_value` is nullable because the + # first change to a surface has no stored predecessor — it moved off the + # shipped starting point, which is a different event from moving off a value + # somebody chose, and worth being able to tell apart. + old_value: Mapped[float | None] = mapped_column(Float, nullable=True) + new_value: Mapped[float] = mapped_column(Float, nullable=False) + + # "model" | "human". See the class docstring: the user id cannot tell these + # apart, and the difference is the first thing a reviewer wants. + actor: Mapped[str] = mapped_column(Text, nullable=False, default="model") + + # REQUIRED at the service boundary, not merely non-null here. A column that + # is technically non-null is satisfied by "", which is how a required field + # becomes a formality; the service refuses a blank one. + reason: Mapped[str] = mapped_column(Text, nullable=False) + + __table_args__ = ( + # The only read this table has: one surface's history, newest first. + Index( + "ix_retrieval_tuning_surface_created", + "surface", + created_at.desc(), + ), + ) + + def to_dict(self) -> dict: + return { + "id": self.id, + "created_at": self.created_at.isoformat() if self.created_at else None, + "surface": self.surface, + "dial": self.dial, + "old_value": self.old_value, + "new_value": self.new_value, + "actor": self.actor, + "reason": self.reason, + } diff --git a/src/scribe/services/retrieval_tuning.py b/src/scribe/services/retrieval_tuning.py new file mode 100644 index 0000000..5e92033 --- /dev/null +++ b/src/scribe/services/retrieval_tuning.py @@ -0,0 +1,208 @@ +"""Moving a retrieval surface's floor or budget, with the argument attached (#4102). + +WHY THIS EXISTS + +The operator's decision for milestone 416 step 4: + + "the floor should be chosen and adjusted by the model using it. we've come + back to something either fails or has to be looked at by the user we need a + model consistent surface for the adjustment of these floor values. the user + should be able to touch it but the model should be the thing handling it 9 + times out of 10." + +`retrieval_surfaces` made the six arms describe their numbers the same way. +This module is the write half: one call that moves one dial on one surface, +records what it was, what it became, who moved it and why, and refuses to do +any of that without a reason. + +WHY A REASON IS REQUIRED + +Because the alternative was tried and measured. The milestone originally listed +self-tuning as a non-goal on the strength of one case: `report_preference` +logged 69 consecutive declines with the refused record 0.0006 under the bar, and +every percentile in the readout said "lower it". Reading the refused record +showed it was rule 77 "Extract intent from loose phrasing" — a false positive — +so lowering the bar would have attached that rule to every completion report +ever written. The statistic and the correct action pointed in opposite +directions. + +What separated them was opening the record. A required `reason` is the cheapest +mechanism that makes that step happen: a caller who must write down why has to +have looked, and a caller who writes down something wrong has left the operator +a sentence to disagree with. A number moved silently leaves nothing. + +WHAT THIS DELIBERATELY DOES NOT DO + +It does not decide anything itself. There is no rule here that reads a +percentile and picks a value, and that absence is the design — the non-goal that +survived is *statistical* auto-tuning, precisely because the statistic was the +thing that was wrong. The judgement stays with the reader; this module only +makes the judgement recordable and reversible. +""" +from __future__ import annotations + +import logging + +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.retrieval_tuning import RetrievalTuningEvent +from scribe.services.retrieval_surfaces import ( + MAX_BUDGET, + budget_for, + floor_for, + get_surface, + surface_names, +) +from scribe.services.settings import set_setting + +logger = logging.getLogger(__name__) + +DIALS = ("floor", "budget") + +# Long enough to say what was read and what it showed; short enough that nobody +# pastes a telemetry dump in. The number is not a measurement — it is the point +# at which "0.66" stops being an acceptable answer to "why". +_MIN_REASON_CHARS = 20 + + +def _clean_reason(reason: str) -> str: + """The guardrail, enforced here rather than by the column. + + `reason` is NOT NULL in the schema, which "" satisfies. A required field + that accepts an empty string is a formality, and this one is the whole + mechanism — see the module docstring. + """ + text = (reason or "").strip() + if len(text) < _MIN_REASON_CHARS: + raise ValueError( + "reason is required, and has to say what you read. A floor moved " + "without a stated basis is a number nobody can review or revert. " + "Name the evidence: which surface's telemetry, and what the refused " + "records actually were — `retrieval_telemetry(near_miss_samples=N)` " + "returns them by id, and reading them is the step that separates a " + "real miss from a bar doing its job." + ) + return text + + +async def current_settings(user_id: int) -> list[dict]: + """Every tunable surface with its live pair and its last stated reason. + + The read side of the tuning surface, and shaped for a reader who is about to + change something: the value, what the arm asks and over what corpus and how + often — because a floor cannot be moved sensibly without those three — and + the reason last given, so the next change argues with the last one instead + of overwriting it blind. + """ + out: list[dict] = [] + async with async_session() as session: + for name in surface_names(): + s = get_surface(name) + rows = ( + await session.execute( + select(RetrievalTuningEvent) + .where( + RetrievalTuningEvent.surface == name, + RetrievalTuningEvent.user_id == user_id, + ) + .order_by(RetrievalTuningEvent.created_at.desc()) + .limit(len(DIALS)) + ) + ).scalars().all() + last = {r.dial: r for r in rows} + out.append({ + "surface": name, + "floor": await floor_for(user_id, name), + "budget": await budget_for(user_id, name), + "floor_default": s.floor_default, + "budget_default": s.budget_default, + "asks": s.asks, + "over": s.over, + "fires": s.fires, + # Absent rather than empty when a surface has never been moved: + # "still on the shipped starting point" is a real state and + # should not render as a blank reason somebody wrote. + "last_change": { + dial: last[dial].to_dict() for dial in DIALS if dial in last + }, + }) + return out + + +async def set_dial( + user_id: int, + surface: str, + dial: str, + value: float, + *, + reason: str, + actor: str = "model", +) -> dict: + """Move one dial on one surface, recording the change and its argument. + + Returns the applied value alongside the previous one, so a caller can see + that a clamp bit rather than assuming the number it sent is the number in + force — the failure being avoided is a tool reporting success for a value + the registry silently corrected. + """ + s = get_surface(surface) # refuses an unknown name + if dial not in DIALS: + raise ValueError(f"dial must be one of {DIALS}, got {dial!r}") + if actor not in ("model", "human"): + raise ValueError(f"actor must be 'model' or 'human', got {actor!r}") + text = _clean_reason(reason) + + if dial == "floor": + old = await floor_for(user_id, surface) + applied = min(1.0, max(0.0, float(value))) + key, stored = s.floor_key, str(applied) + else: + old = float(await budget_for(user_id, surface)) + applied = float(min(MAX_BUDGET, max(1, int(float(value))))) + key, stored = s.budget_key, str(int(applied)) + + await set_setting(user_id, key, stored) + async with async_session() as session: + session.add(RetrievalTuningEvent( + user_id=user_id, surface=surface, dial=dial, + old_value=old, new_value=applied, actor=actor, reason=text, + )) + await session.commit() + + return { + "surface": surface, + "dial": dial, + "previous": old, + "applied": applied, + # True when the registry corrected what was asked for. Said out loud + # because a caller that believes it set 1.4 will read the next + # telemetry as evidence about a bar that was never in force. + "clamped": abs(applied - float(value)) > 1e-9, + "reason": text, + "actor": actor, + } + + +async def tuning_history( + user_id: int, *, surface: str | None = None, limit: int = 20 +) -> list[dict]: + """What has been moved, newest first — the operator's review surface. + + Scoped to one surface when asked, because the question is almost always + "why is THIS arm set like this", and an unscoped list buries one surface's + two changes under another's twenty. + """ + if surface is not None: + get_surface(surface) # refuse a typo on the read too + async with async_session() as session: + q = ( + select(RetrievalTuningEvent) + .where(RetrievalTuningEvent.user_id == user_id) + .order_by(RetrievalTuningEvent.created_at.desc()) + .limit(max(1, min(int(limit), 200))) + ) + if surface is not None: + q = q.where(RetrievalTuningEvent.surface == surface) + rows = (await session.execute(q)).scalars().all() + return [r.to_dict() for r in rows] diff --git a/tests/test_services_reply_preferences.py b/tests/test_services_reply_preferences.py index 185233b..d742b2c 100644 --- a/tests/test_services_reply_preferences.py +++ b/tests/test_services_reply_preferences.py @@ -9,6 +9,9 @@ import re from unittest.mock import AsyncMock, MagicMock, patch M = "scribe.services.reply_preferences" +# Its two numbers are resolved through the registry now (#4102), so the +# settings reader to patch lives there rather than in the arm's own module. +RS = "scribe.services.retrieval_surfaces" def _rule(rid, kind="preference"): @@ -26,7 +29,7 @@ async def _run(hits, *, searched=True, raises=None): search_mock = AsyncMock(side_effect=search) with patch(f"{M}.semantic_search_rules", search_mock), \ - patch(f"{M}.get_setting", AsyncMock(return_value="0.72")), \ + patch(f"{RS}.get_setting", AsyncMock(return_value="0.72")), \ patch(f"{M}.record_retrieval") as logged, \ patch(f"{M}.record_rule_surfaced") as surfaced: out = await completion_preferences(7, project_id=3) @@ -93,7 +96,7 @@ def test_its_bar_is_its_own_key_not_the_prompt_arm_s(monkeypatch): asked: list[str] = [] - async def get_setting(user_id, key, default): + async def get_setting(user_id, key, default=""): asked.append(key) return default @@ -101,16 +104,26 @@ def test_its_bar_is_its_own_key_not_the_prompt_arm_s(monkeypatch): kw["report"].update({"searched": True}) return [] - with patch(f"{M}.get_setting", AsyncMock(side_effect=get_setting)), \ + with patch(f"{RS}.get_setting", AsyncMock(side_effect=get_setting)), \ patch(f"{M}.semantic_search_rules", AsyncMock(side_effect=search)), \ patch(f"{M}.record_retrieval"): asyncio.run(completion_preferences(7)) - assert asked == [plugin_context.REPORTPREF_THRESHOLD_KEY] - assert plugin_context.REPORTPREF_THRESHOLD_KEY != plugin_context.PROMPTRULE_THRESHOLD_KEY, ( + # Both numbers are its own since #4102 — a floor AND a budget — so the arm + # asks for two keys, and neither may be the prompt arm's. + from scribe.services.retrieval_surfaces import get_surface + + mine = get_surface("report_preference") + assert asked == [mine.floor_key, mine.budget_key] + theirs = get_surface("prompt_rule") + assert mine.floor_key != theirs.floor_key, ( "the two keys are the same string again, so the settings form has one " "dial driving two arms — which is the defect, whatever the value is" ) + assert mine.floor_key == plugin_context.REPORTPREF_THRESHOLD_KEY, ( + "the registry and the module constant disagree about this arm's key, " + "so the Settings form would write one and the arm would read the other" + ) def test_the_query_assumes_no_particular_domain(): diff --git a/tests/test_settings_defaults_agree.py b/tests/test_settings_defaults_agree.py index 390abf7..17166c6 100644 --- a/tests/test_settings_defaults_agree.py +++ b/tests/test_settings_defaults_agree.py @@ -20,9 +20,12 @@ string equals the Python default. Retuning either stays free as long as both move — which is the point, because these are tuning values and #3853 moved one of them the day this was written. -Both sides are read from SOURCE rather than imported. The Vue file cannot be -imported at all, and reading the Python constant through an import would tie -this to module-load side effects it has no interest in. +The Vue side is read from SOURCE, because that file cannot be imported at all. +The Python side is read from source for a loose constant and IMPORTED for a +registry surface — the registry is a plain table of frozen dataclasses with +nothing to trigger on import, and importing it means this guard checks the value +the server will actually resolve rather than a literal that happens to look +right. It cannot check that the form WRITES the right key — that is behaviour, and the keys are asserted where they are built. It catches the drift that has no @@ -39,20 +42,29 @@ ROOT = pathlib.Path(__file__).resolve().parents[1] _SERVICES = ROOT / "src" / "scribe" / "services" _VUE = ROOT / "frontend" / "src" / "views" / "SettingsView.vue" -# (services module, python constant, vue ref). Hand-written because the -# pairing is an editorial fact — the names do not share a convention either -# side could derive — but every entry is asserted to EXIST on both sides, so a -# rename fails loudly here rather than silently dropping that threshold from -# the check. -_PAIRS = ( - ("plugin_context.py", "AUTOINJECT_DEFAULT_THRESHOLD", "kbInjectThreshold"), - ("plugin_context.py", "WRITEPATH_DEFAULT_THRESHOLD", "kbWritePathThreshold"), - ("plugin_context.py", "RULEHINT_DEFAULT_THRESHOLD", "kbRuleHintThreshold"), - ("plugin_context.py", "TOOLRULE_DEFAULT_THRESHOLD", "kbToolRuleThreshold"), - ("plugin_context.py", "PROMPTRULE_DEFAULT_THRESHOLD", "kbPromptRuleThreshold"), - ("plugin_context.py", "REPORTPREF_DEFAULT_THRESHOLD", "kbReportPrefThreshold"), +# THE SIX RETRIEVAL FLOORS now live in one registry (#4102), so their side of +# the pairing is a SURFACE NAME rather than a module constant. That is strictly +# better for this guard: the surface name is also the telemetry source, so a row +# here names the same arm the readout does, and a floor that moved cannot be +# checked against a stale constant that happened to keep its old value. +_SURFACE_PAIRS = ( + ("auto_inject", "kbInjectThreshold"), + ("write_path", "kbWritePathThreshold"), + ("write_path_rule", "kbRuleHintThreshold"), + ("pre_tool_rule", "kbToolRuleThreshold"), + ("prompt_rule", "kbPromptRuleThreshold"), + ("report_preference", "kbReportPrefThreshold"), +) + +# (services module, python constant, vue ref) for the defaults that are NOT +# retrieval surfaces. Hand-written because the pairing is an editorial fact — +# the names share no convention either side could derive — and asserted to +# exist on both sides, so a rename fails loudly rather than silently dropping +# that threshold from the check. +_CONSTANT_PAIRS = ( # The plan gate (milestone 415): it blocks a create, so a form showing a - # looser bar than the one in force would be the more misleading drift. + # looser bar than the one in force would be the more misleading drift. Not + # a push surface, so it has no registry entry and keeps the older shape. ("dedup.py", "PLAN_MATCH_DEFAULT_THRESHOLD", "kbPlanMatchThreshold"), ) @@ -80,14 +92,42 @@ def _vue_default(ref_name: str) -> float: return float(m.group(1)) -@pytest.mark.parametrize(("module", "constant", "ref_name"), _PAIRS, - ids=[p[1] for p in _PAIRS]) -def test_the_form_shows_the_default_the_server_uses(module, constant, ref_name): - """An untouched control must render the bar actually in force.""" - server, form = _python_default(module, constant), _vue_default(ref_name) +def _assert_agrees(server: float, form: float, ref_name: str, what: str) -> None: assert form == server, ( f"SettingsView shows {form} for {ref_name} while the server defaults " - f"to {server} ({constant}). An operator who has never set this reads " - f"the form as the value in force, so the two must move together — " - f"retune both, or neither." + f"to {server} ({what}). An operator who has never set this reads the " + f"form as the value in force, so the two must move together — retune " + f"both, or neither." ) + + +@pytest.mark.parametrize(("surface", "ref_name"), _SURFACE_PAIRS, + ids=[p[0] for p in _SURFACE_PAIRS]) +def test_the_form_shows_the_floor_the_surface_uses(surface, ref_name): + """An untouched control must render the bar actually in force.""" + from scribe.services.retrieval_surfaces import get_surface + + _assert_agrees(get_surface(surface).floor_default, _vue_default(ref_name), + ref_name, f"{surface} floor") + + +def test_every_tunable_surface_has_a_control(): + """Rule 25/27: a number an operator may need different has a UI or it is + not shipped. Derived from the registry, so a seventh surface arrives here + as a failure rather than as a setting only the model can reach.""" + from scribe.services.retrieval_surfaces import surface_names + + covered = {s for s, _ref in _SURFACE_PAIRS} + missing = [n for n in surface_names() if n not in covered] + assert not missing, ( + f"these surfaces are tunable with no Settings control: {missing}. " + "Add the control and its row, or say in _SURFACE_PAIRS why it has none." + ) + + +@pytest.mark.parametrize(("module", "constant", "ref_name"), _CONSTANT_PAIRS, + ids=[p[1] for p in _CONSTANT_PAIRS]) +def test_the_form_shows_the_default_the_server_uses(module, constant, ref_name): + """Same claim, for the defaults that are not retrieval surfaces.""" + _assert_agrees(_python_default(module, constant), _vue_default(ref_name), + ref_name, constant)