fix(tests): two readers moved, and the settings guard now checks the registry (#4102)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 43s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m2s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 43s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m2s
CI & Build / Build & push image (push) Skipped
CI on 09b4845 caught both, and the second is an improvement rather than a
repair.
`test_services_reply_preferences` patched `reply_preferences.get_setting`,
which the registry refactor removed — its floor and budget are resolved through
`retrieval_surfaces` now. Its key-independence test also asserted the arm asks
for exactly one key; it asks for two, because both of its numbers are its own
since this step, so the assertion names both and adds a check that the registry
and the module constant still agree about the floor key. They writing different
keys is the failure where the Settings form saves one string and the arm reads
another.
`test_settings_defaults_agree` parsed `plugin_context.py` for a bare
module-level float, and those constants now alias the registry. Rather than
teach the regex about aliases, the six retrieval floors are keyed on their
SURFACE NAME and read from the registry directly — which is strictly better for
this guard: a surface name is also its telemetry source, so a row names the same
arm the readout does, and a floor cannot be checked against a stale constant
that happened to keep its old value. `PLAN_MATCH_DEFAULT_THRESHOLD` is not a
push surface and keeps the older shape, with a note saying why.
Added while there: `test_every_tunable_surface_has_a_control`, derived from the
registry, so a seventh surface arrives as a failing test rather than as a number
only the model can reach (rules 25, 27).
Also lands the audit trail this step needs — `retrieval_tuning_events` (model +
migration 0103) and `services/retrieval_tuning.py`. The tool layer on top is
the next commit; the table is here because `reason` being REQUIRED is the whole
guardrail, and the schema is where that starts. A number moved silently leaves
nothing for the operator to review or disagree with, and the operator's decision
is that the model moves these "9 times out of 10".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
Reference in New Issue
Block a user