feat(retrieval): the operator can see what was tuned, and every write is recorded (#4102)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m30s
CI & Build / Build & push image (push) Canceled after 31s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m30s
CI & Build / Build & push image (push) Canceled after 31s
The other half of the bargain in milestone 416 step 4. The model moves these dials; this is what makes that reviewable rather than merely automatic. THE HOLE THIS CLOSES Every retrieval floor is an ordinary settings key, and `/api/settings` accepts any key at all. A floor written through it landed correctly and recorded nothing — a tuning history with holes in it, which is worse than no history because it reads as complete. So the generic endpoint now routes registry-owned keys through `set_dial` instead of writing them as plain rows. ROUTED, not refused: refusing would only work for callers that had been updated, while this way the form, a script, and an old client all leave the trail, and there is no version of "forgot to use the other endpoint". Clearing a control is written as an explicit set back to the shipped default, because the operator reverting something is the single most important move this history can record. `set_dial` now also refuses to record a no-op. The Settings form re-sends every field on every save, so without that one press of Save would write six rows saying the operator set six dials to the numbers they were already on — and a history nobody can skim is one nobody reads. WHAT THE OPERATOR GETS `/api/retrieval/surfaces`, `/surfaces/<name>` and `/tuning-history`, with `actor` fixed server-side rather than taken from the payload: a payload-supplied actor would let a model claim to be the operator, and "did I do this, or did the session?" is the first question this list is asked. In Settings: the five missing BUDGETS (until now only auto-inject had one, so the only control over a noisy surface was to raise its bar — which discards that surface's best candidates along with its worst), and a "What has been tuned" panel showing each change, who made it, and the reason given. The operator's own changes are marked. The MCP tool demands a reason; these endpoints do not. That asymmetry is deliberate and stated in routes/retrieval.py: the requirement exists to make the MODEL read the records before moving a number on someone else's behalf, and the operator is that someone — a mandatory justification box on every control would be friction charged to the one participant who owes no explanation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -14,6 +14,7 @@ from scribe.routes.notes import notes_bp
|
||||
from scribe.routes.milestones import milestones_bp
|
||||
from scribe.routes.task_logs import task_logs_bp
|
||||
from scribe.routes.projects import projects_bp
|
||||
from scribe.routes.retrieval import retrieval_bp
|
||||
from scribe.routes.settings import settings_bp
|
||||
from scribe.routes.tasks import tasks_bp
|
||||
from scribe.routes.groups import groups_bp
|
||||
@@ -79,6 +80,7 @@ def create_app() -> Quart:
|
||||
app.register_blueprint(milestones_bp)
|
||||
app.register_blueprint(notes_bp)
|
||||
app.register_blueprint(projects_bp)
|
||||
app.register_blueprint(retrieval_bp)
|
||||
app.register_blueprint(settings_bp)
|
||||
app.register_blueprint(task_logs_bp)
|
||||
app.register_blueprint(tasks_bp)
|
||||
|
||||
@@ -0,0 +1,106 @@
|
||||
"""The operator's view of retrieval tuning — what the dials are, and who moved them (#4102).
|
||||
|
||||
WHY THIS EXISTS SEPARATELY FROM /api/settings
|
||||
|
||||
The numbers themselves are ordinary settings rows and could be written through
|
||||
the generic KV endpoint. They must not be, and that is the whole point of this
|
||||
blueprint: a floor changed through `/api/settings` moves silently, leaving the
|
||||
history saying nothing happened. From milestone 416 those dials are moved by
|
||||
the model on the operator's behalf, so a trail with holes in it is worse than
|
||||
no trail — it reads as complete.
|
||||
|
||||
So every change to a retrieval dial goes through `set_dial`, from the UI as
|
||||
much as from the MCP tool, and the only difference is the `actor` recorded.
|
||||
"""
|
||||
import logging
|
||||
|
||||
from quart import Blueprint, jsonify, request
|
||||
|
||||
from scribe.auth import get_current_user_id, login_required
|
||||
from scribe.services.retrieval_tuning import set_dial, current_settings, tuning_history
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
retrieval_bp = Blueprint("retrieval", __name__, url_prefix="/api/retrieval")
|
||||
|
||||
# What is recorded when the operator changes a dial in Settings and says
|
||||
# nothing about why.
|
||||
#
|
||||
# The MCP tool REFUSES a blank reason, and this endpoint does not, which is a
|
||||
# deliberate asymmetry rather than an oversight. The requirement exists to make
|
||||
# the model look at the records before it moves a number on someone else's
|
||||
# behalf. The operator IS that someone: they are the audience the trail is
|
||||
# written for, they cannot be uninformed about their own decision, and a
|
||||
# mandatory justification textarea on every control would be friction charged
|
||||
# to the one participant who owes no explanation (rule 24).
|
||||
#
|
||||
# The event is still written, because "the operator set this by hand" is the
|
||||
# single most useful thing the history can tell a later session — it is the one
|
||||
# entry the model must not quietly tune back.
|
||||
_OPERATOR_DEFAULT_REASON = "Set directly in Settings by the operator."
|
||||
|
||||
|
||||
@retrieval_bp.route("/surfaces", methods=["GET"])
|
||||
@login_required
|
||||
async def get_surfaces_route():
|
||||
"""Every tunable surface: its live floor and budget, what it asks and over
|
||||
what, and the reason each dial was last moved."""
|
||||
uid = get_current_user_id()
|
||||
surfaces = await current_settings(uid)
|
||||
return jsonify({"surfaces": surfaces, "total": len(surfaces)})
|
||||
|
||||
|
||||
@retrieval_bp.route("/surfaces/<surface>", methods=["PUT"])
|
||||
@login_required
|
||||
async def tune_surface_route(surface: str):
|
||||
"""Move one dial, recorded as a human change.
|
||||
|
||||
`actor` is fixed here rather than taken from the payload: this endpoint is
|
||||
reached with a session cookie from the Settings form, so the actor is known
|
||||
and accepting a claim about it would let the one field a reviewer relies on
|
||||
be set to anything.
|
||||
"""
|
||||
uid = get_current_user_id()
|
||||
data = await request.get_json()
|
||||
if not isinstance(data, dict):
|
||||
return jsonify({"error": "Expected a JSON object"}), 400
|
||||
if "dial" not in data or "value" not in data:
|
||||
return jsonify({"error": "dial and value are required"}), 400
|
||||
|
||||
try:
|
||||
value = float(data["value"])
|
||||
except (TypeError, ValueError):
|
||||
return jsonify({"error": f"value must be a number, got {data['value']!r}"}), 400
|
||||
|
||||
reason = str(data.get("reason") or "").strip() or _OPERATOR_DEFAULT_REASON
|
||||
try:
|
||||
result = await set_dial(
|
||||
uid, surface, str(data["dial"]), value, reason=reason, actor="human",
|
||||
)
|
||||
except ValueError as e:
|
||||
# Unknown surface, unknown dial — the service names the alternatives,
|
||||
# so the message is worth passing through rather than flattening.
|
||||
return jsonify({"error": str(e)}), 400
|
||||
return jsonify(result)
|
||||
|
||||
|
||||
@retrieval_bp.route("/tuning-history", methods=["GET"])
|
||||
@login_required
|
||||
async def tuning_history_route():
|
||||
"""What has been changed about retrieval on this install, newest first.
|
||||
|
||||
The review surface. Unscoped by default because the operator's question
|
||||
here is "what has been done on my behalf", not "why is this one arm set
|
||||
like that" — the per-surface scoping is for the model.
|
||||
"""
|
||||
uid = get_current_user_id()
|
||||
surface = request.args.get("surface") or None
|
||||
try:
|
||||
limit = int(request.args.get("limit", 50))
|
||||
except ValueError:
|
||||
return jsonify({"error": "limit must be a whole number"}), 400
|
||||
try:
|
||||
events = await tuning_history(uid, surface=surface, limit=limit)
|
||||
except ValueError as e:
|
||||
return jsonify({"error": str(e)}), 400
|
||||
return jsonify({"events": events, "total": len(events)})
|
||||
@@ -9,6 +9,8 @@ from quart import Blueprint, jsonify, request
|
||||
|
||||
from scribe.auth import login_required, get_current_user_id
|
||||
from scribe.config import Config
|
||||
from scribe.services.retrieval_surfaces import dial_for_key, get_surface
|
||||
from scribe.services.retrieval_tuning import set_dial
|
||||
from scribe.services.settings import (
|
||||
SECRET_MASK, delete_setting, get_all_settings, get_setting, set_settings_batch,
|
||||
)
|
||||
@@ -25,6 +27,13 @@ settings_bp = Blueprint("settings", __name__, url_prefix="/api/settings")
|
||||
# (forge_token left with 0078: forge credentials are keyring rows now, #2778.)
|
||||
_SECRET_KEYS = frozenset({"smtp_password", "forge_webhook_secret"})
|
||||
|
||||
# What the tuning history records for a dial changed through this form. The MCP
|
||||
# tool refuses a blank reason; the operator is not asked for one, because the
|
||||
# requirement exists to make the MODEL read the records before moving a number
|
||||
# on someone else's behalf — and the operator is that someone. See
|
||||
# routes/retrieval.py, which states the asymmetry in full.
|
||||
_SETTINGS_FORM_REASON = "Changed in Settings by the operator."
|
||||
|
||||
|
||||
def _masked(settings: dict) -> dict:
|
||||
return {
|
||||
@@ -52,6 +61,33 @@ async def update_settings_route():
|
||||
to_save = {}
|
||||
for k, v in data.items():
|
||||
str_v = str(v)
|
||||
# A retrieval dial is never written as a plain settings row, wherever
|
||||
# the request came from (#4102). The value would land correctly and the
|
||||
# tuning history would say nothing happened — and since milestone 416
|
||||
# those dials are moved by the model on the operator's behalf, a
|
||||
# history with holes in it is worse than none: it reads as complete.
|
||||
#
|
||||
# Routed rather than refused on purpose. Refusing would work only for
|
||||
# callers that had been updated; this way every caller that ever writes
|
||||
# one of these keys — this form, a script, an old client — leaves the
|
||||
# trail, and there is no version of "forgot to use the other endpoint".
|
||||
dial = dial_for_key(k)
|
||||
if dial:
|
||||
surface, which = dial
|
||||
s = get_surface(surface)
|
||||
# A CLEARED control means "back to the shipped starting point", and
|
||||
# that is a change like any other — it is the operator reverting
|
||||
# something, which is the single most important move this history
|
||||
# can record. So it is written as an explicit set to the default
|
||||
# rather than deleted, which would leave the same value behind and
|
||||
# no record of anyone having chosen it.
|
||||
default = s.floor_default if which == "floor" else s.budget_default
|
||||
try:
|
||||
await set_dial(uid, surface, which, float(str_v or default),
|
||||
reason=_SETTINGS_FORM_REASON, actor="human")
|
||||
except (TypeError, ValueError) as e:
|
||||
return jsonify({"error": f"{k}: {e}"}), 400
|
||||
continue
|
||||
# A masked secret round-tripping through a client is "unchanged", not
|
||||
# a request to store the mask over the real credential.
|
||||
if k in _SECRET_KEYS and str_v == SECRET_MASK:
|
||||
|
||||
@@ -213,6 +213,26 @@ def get_surface(name: str) -> Surface:
|
||||
) from None
|
||||
|
||||
|
||||
def dial_for_key(key: str) -> tuple[str, str] | None:
|
||||
"""Which `(surface, dial)` a settings key belongs to, or None.
|
||||
|
||||
The registry read backwards, and it exists for one caller: the generic
|
||||
`/api/settings` endpoint, which accepts any key at all. Without this, a
|
||||
floor written through that endpoint moves with no event recorded, and the
|
||||
tuning history says nothing happened — a trail with holes in it, which is
|
||||
worse than no trail because it reads as complete.
|
||||
|
||||
Derived rather than listed so a seventh surface is covered the moment it is
|
||||
added here, which is the only way this stays true.
|
||||
"""
|
||||
for surface in SURFACES.values():
|
||||
if key == surface.floor_key:
|
||||
return (surface.name, "floor")
|
||||
if key == surface.budget_key:
|
||||
return (surface.name, "budget")
|
||||
return None
|
||||
|
||||
|
||||
async def floor_for(user_id: int, name: str) -> float:
|
||||
"""This install's current floor for a surface, clamped to [0, 1]."""
|
||||
s = get_surface(name)
|
||||
|
||||
@@ -162,6 +162,21 @@ async def set_dial(
|
||||
applied = float(min(MAX_BUDGET, max(1, int(float(value)))))
|
||||
key, stored = s.budget_key, str(int(applied))
|
||||
|
||||
# A change to the value it already has is not a change, and must not be
|
||||
# written. The Settings form re-sends every field on every save, so without
|
||||
# this the history fills with rows saying the operator set six dials to the
|
||||
# numbers they were already on — and a history nobody can skim is one
|
||||
# nobody reads, which costs the surface its entire purpose.
|
||||
#
|
||||
# Reported rather than silently skipped, so a caller that expected to move
|
||||
# something learns that it did not.
|
||||
if abs(applied - old) < 1e-9:
|
||||
return {
|
||||
"surface": surface, "dial": dial, "previous": old,
|
||||
"applied": applied, "clamped": abs(applied - float(value)) > 1e-9,
|
||||
"reason": text, "actor": actor, "unchanged": True,
|
||||
}
|
||||
|
||||
await set_setting(user_id, key, stored)
|
||||
async with async_session() as session:
|
||||
session.add(RetrievalTuningEvent(
|
||||
@@ -181,6 +196,9 @@ async def set_dial(
|
||||
"clamped": abs(applied - float(value)) > 1e-9,
|
||||
"reason": text,
|
||||
"actor": actor,
|
||||
# Always present, both ways round: a caller that has to test for the
|
||||
# key's absence to learn the answer will eventually forget to.
|
||||
"unchanged": False,
|
||||
}
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user