feat(retrieval): the model moves its own floors, and says why (#4102)
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 43s
CI & Build / TypeScript typecheck (push) Successful in 56s
CI & Build / Python tests (push) Failing after 1m5s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 43s
CI & Build / TypeScript typecheck (push) Successful in 56s
CI & Build / Python tests (push) Failing after 1m5s
CI & Build / Build & push image (push) Skipped
Milestone 416 step 4's write half. `retrieval_surfaces.py` made the six
push arms describe their `{floor, budget}` the same way; this adds the
three MCP tools that let the model READ that and change it, and the
backup sections that carry the reasons.
The operator's decision, which this implements:
"the floor should be chosen and adjusted by the model using it… the
user should be able to touch it but the model should be the thing
handling it 9 times out of 10."
WHY A REASON IS REQUIRED, AND WHY THE TOOL ARGUES AGAINST PERCENTILES
The milestone originally listed self-tuning as a non-goal on one
measured case, and that case is now the tool's docstring rather than a
prohibition: `report_preference` logged 69 consecutive declines with
the refused record 0.0006 under the bar, and every percentile said
"lower it". The refused record was rule 77 "Extract intent from loose
phrasing" matched against a query about report layout — a false
positive. Lowering would have attached that rule to every completion
report ever written.
What separated the statistic from the correct action was OPENING the
record. So `tune_retrieval` refuses a blank or perfunctory reason,
tells the caller to read `retrieval_telemetry(near_miss_samples=5)`
and the record ids it names, and carries that 69-decline example — an
abstract warning loses to a number. The non-goal that survives is
*statistical* auto-tuning; nothing here reads a percentile and picks a
value.
BACKUP (v16), which is what CI caught
`retrieval_tuning_events` was neither backed up nor excluded, and
#2293's guard said so. It is backed up: `settings` already carried the
numbers, so dropping this would restore an install with six moved
dials and no argument for any of them — precisely the state the table
exists to prevent, and worse now that the model is the one moving
them. One `_retrieval_tuning_event_rows` builder called from both
exporters (snippet #2851); `user_id` travels because a restore has to
remap it, which is why the row builder is not the model's `to_dict()`.
`surface` is a registry name rather than a foreign key, so the history
survives a restore into an install whose ids all differ.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -5,14 +5,15 @@ to a FastMCP instance. `register_all(mcp)` is the single entry point called
|
|||||||
from `mcp.server.build_mcp_server`.
|
from `mcp.server.build_mcp_server`.
|
||||||
"""
|
"""
|
||||||
from scribe.mcp.tools import (
|
from scribe.mcp.tools import (
|
||||||
design_systems, milestones, notes, processes, projects, recent, repos, rulebooks, search, shapes,
|
design_systems, milestones, notes, processes, projects, recent, repos, retrieval_tuning,
|
||||||
snippets, systems, tags, tasks, trash,
|
rulebooks, search, shapes, snippets, systems, tags, tasks, trash,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def register_all(mcp) -> None:
|
def register_all(mcp) -> None:
|
||||||
"""Register every tool module's tools on the given FastMCP instance."""
|
"""Register every tool module's tools on the given FastMCP instance."""
|
||||||
search.register(mcp)
|
search.register(mcp)
|
||||||
|
retrieval_tuning.register(mcp)
|
||||||
notes.register(mcp)
|
notes.register(mcp)
|
||||||
tasks.register(mcp)
|
tasks.register(mcp)
|
||||||
projects.register(mcp)
|
projects.register(mcp)
|
||||||
|
|||||||
@@ -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)
|
||||||
@@ -13,6 +13,7 @@ from scribe.models.rule_version import RuleVersion
|
|||||||
from scribe.models.design_system import DesignSystem, DesignToken
|
from scribe.models.design_system import DesignSystem, DesignToken
|
||||||
from scribe.models.note_usage import NoteUsageEvent
|
from scribe.models.note_usage import NoteUsageEvent
|
||||||
from scribe.models.rule_usage import RuleUsageEvent
|
from scribe.models.rule_usage import RuleUsageEvent
|
||||||
|
from scribe.models.retrieval_tuning import RetrievalTuningEvent
|
||||||
from scribe.models.canonical_system import CanonicalSystem
|
from scribe.models.canonical_system import CanonicalSystem
|
||||||
from scribe.models.rulebook import RuleRelation, rule_systems as rule_systems_t
|
from scribe.models.rulebook import RuleRelation, rule_systems as rule_systems_t
|
||||||
from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse
|
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
|
# 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
|
# home now. Older archives carrying those sections still restore — the keys are
|
||||||
# simply not read — as do the subscribe_rulebooks inception choices they hold.
|
# 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.
|
# 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
|
# 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
|
# 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
|
# accumulated, so a restore that dropped it would silently reset the
|
||||||
# measurement to zero while everything still looked fine.
|
# measurement to zero while everything still looked fine.
|
||||||
"rule_usage_events",
|
"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
|
# 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"},
|
"note_usage_events": {"id"},
|
||||||
# Same as the note twin: the surrogate key is re-issued on insert.
|
# Same as the note twin: the surrogate key is re-issued on insert.
|
||||||
"rule_usage_events": {"id"},
|
"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_systems": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"},
|
||||||
"design_tokens": {"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"},
|
"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]:
|
def _code_shape_rows(rows) -> list[dict]:
|
||||||
return [r.to_dict() for r in rows]
|
return [r.to_dict() for r in rows]
|
||||||
|
|
||||||
@@ -617,6 +652,12 @@ async def export_full_backup() -> dict:
|
|||||||
rule_usage_events = (
|
rule_usage_events = (
|
||||||
await session.execute(select(RuleUsageEvent))
|
await session.execute(select(RuleUsageEvent))
|
||||||
).scalars().all()
|
).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()
|
repo_bindings = (await session.execute(select(RepoBinding))).scalars().all()
|
||||||
code_shapes = (await session.execute(select(CodeShape))).scalars().all()
|
code_shapes = (await session.execute(select(CodeShape))).scalars().all()
|
||||||
code_shape_events = (await session.execute(
|
code_shape_events = (await session.execute(
|
||||||
@@ -661,6 +702,9 @@ async def export_full_backup() -> dict:
|
|||||||
"design_tokens": _design_token_rows(design_tokens),
|
"design_tokens": _design_token_rows(design_tokens),
|
||||||
"note_usage_events": _usage_event_rows(usage_events),
|
"note_usage_events": _usage_event_rows(usage_events),
|
||||||
"rule_usage_events": _rule_usage_event_rows(rule_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),
|
"repo_bindings": _repo_binding_rows(repo_bindings),
|
||||||
"note_supersessions": _note_supersession_rows(supersessions),
|
"note_supersessions": _note_supersession_rows(supersessions),
|
||||||
"code_shapes": _code_shape_rows(code_shapes),
|
"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(
|
rule_usage_events = (await session.execute(
|
||||||
select(RuleUsageEvent).where(RuleUsageEvent.rule_id.in_(_rule_ids))
|
select(RuleUsageEvent).where(RuleUsageEvent.rule_id.in_(_rule_ids))
|
||||||
)).scalars().all() if _rule_ids else []
|
)).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(
|
rule_relations = (await session.execute(
|
||||||
select(RuleRelation).where(
|
select(RuleRelation).where(
|
||||||
RuleRelation.from_rule_id.in_(_rule_ids),
|
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),
|
"design_tokens": _design_token_rows(design_tokens),
|
||||||
"note_usage_events": _usage_event_rows(usage_events),
|
"note_usage_events": _usage_event_rows(usage_events),
|
||||||
"rule_usage_events": _rule_usage_event_rows(rule_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),
|
"repo_bindings": _repo_binding_rows(repo_bindings),
|
||||||
"note_supersessions": _note_supersession_rows(supersessions),
|
"note_supersessions": _note_supersession_rows(supersessions),
|
||||||
"code_shapes": _code_shape_rows(code_shapes),
|
"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,
|
"note_supersessions": 0, "code_shapes": 0, "code_shape_events": 0,
|
||||||
"code_shape_uses": 0, "canonical_systems": 0,
|
"code_shape_uses": 0, "canonical_systems": 0,
|
||||||
"rule_systems": 0, "rule_relations": 0, "rule_versions": 0,
|
"rule_systems": 0, "rule_relations": 0, "rule_versions": 0,
|
||||||
|
"retrieval_tuning_events": 0,
|
||||||
}
|
}
|
||||||
|
|
||||||
async with async_session() as session:
|
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", "")))
|
session.add(Setting(user_id=mapped_uid, key=s_data["key"], value=s_data.get("value", "")))
|
||||||
stats["settings"] += 1
|
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)
|
# 9. Rulebooks (v3)
|
||||||
for rb_data in data.get("rulebooks", []):
|
for rb_data in data.get("rulebooks", []):
|
||||||
mapped_uid = user_id_map.get(rb_data.get("owner_user_id", 0))
|
mapped_uid = user_id_map.get(rb_data.get("owner_user_id", 0))
|
||||||
|
|||||||
@@ -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",
|
||||||
|
]
|
||||||
@@ -23,7 +23,7 @@ def test_backup_version_is_current():
|
|||||||
|
|
||||||
(Named for the number it asserted until v10, which is exactly the drift a
|
(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.)"""
|
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):
|
def _exportable_note(**over):
|
||||||
@@ -134,6 +134,7 @@ def _column_guard_targets():
|
|||||||
from scribe.models.note_supersession import NoteSupersession
|
from scribe.models.note_supersession import NoteSupersession
|
||||||
from scribe.models.note_usage import NoteUsageEvent
|
from scribe.models.note_usage import NoteUsageEvent
|
||||||
from scribe.models.rule_usage import RuleUsageEvent
|
from scribe.models.rule_usage import RuleUsageEvent
|
||||||
|
from scribe.models.retrieval_tuning import RetrievalTuningEvent
|
||||||
from scribe.models.note_version import NoteVersion
|
from scribe.models.note_version import NoteVersion
|
||||||
from scribe.models.rule_version import RuleVersion
|
from scribe.models.rule_version import RuleVersion
|
||||||
from scribe.models.project import Project
|
from scribe.models.project import Project
|
||||||
@@ -164,6 +165,9 @@ def _column_guard_targets():
|
|||||||
"rule_relations": (RuleRelation, backup._rule_relation_rows),
|
"rule_relations": (RuleRelation, backup._rule_relation_rows),
|
||||||
"note_usage_events": (NoteUsageEvent, backup._usage_event_rows),
|
"note_usage_events": (NoteUsageEvent, backup._usage_event_rows),
|
||||||
"rule_usage_events": (RuleUsageEvent, backup._rule_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_systems": (DesignSystem, backup._design_system_rows),
|
||||||
"design_tokens": (DesignToken, backup._design_token_rows),
|
"design_tokens": (DesignToken, backup._design_token_rows),
|
||||||
"repo_bindings": (RepoBinding, backup._repo_binding_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",
|
"systems", "record_systems", "design_systems",
|
||||||
"design_tokens", "note_usage_events", "repo_bindings",
|
"design_tokens", "note_usage_events", "repo_bindings",
|
||||||
"note_supersessions", "code_shapes", "code_shape_events",
|
"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 key in out, f"missing export section: {key}"
|
||||||
assert out[key] == []
|
assert out[key] == []
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user