fix(retrieval): the newest two rows are not the newest row of each dial (#4104)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 42s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / Python tests (push) Successful in 1m33s
CI & Build / Build & push image (push) Successful in 33s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 42s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / Python tests (push) Successful in 1m33s
CI & Build / Build & push image (push) Successful in 33s
`current_settings` read a surface's history with one query and `limit(2)`, then keyed the rows by dial. That is only the same thing while both dials have moved equally often — and they do not. Floors get walked; budgets rarely move. Three floor changes and one budget change returns two floor rows, and the budget change vanishes. Under #4102 that cost a missing reason. Since #4104 it is worse: the dial then reports `source: "shipped"` — still on the value Scribe shipped — for a number somebody deliberately tuned. A wrong calibration answer, in the direction a reader has no cause to double-check. One query per dial, `limit(1)` each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -153,18 +153,30 @@ async def current_settings(user_id: int) -> 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,
|
||||
# ONE QUERY PER DIAL, not one `limit(len(DIALS))` over both.
|
||||
# "The newest two rows" is not "the newest row of each kind": a
|
||||
# surface whose floor was moved three times and whose budget was
|
||||
# moved once returns two floor rows, and the budget change
|
||||
# disappears. That was a missing reason when this only fed
|
||||
# `last_change`; since #4104 it is also a WRONG calibration answer —
|
||||
# a tuned dial reporting as "still on the shipped default", which is
|
||||
# the one state a reader would not think to check.
|
||||
last = {}
|
||||
for dial in DIALS:
|
||||
row = (
|
||||
await session.execute(
|
||||
select(RetrievalTuningEvent)
|
||||
.where(
|
||||
RetrievalTuningEvent.surface == name,
|
||||
RetrievalTuningEvent.user_id == user_id,
|
||||
RetrievalTuningEvent.dial == dial,
|
||||
)
|
||||
.order_by(RetrievalTuningEvent.created_at.desc())
|
||||
.limit(1)
|
||||
)
|
||||
.order_by(RetrievalTuningEvent.created_at.desc())
|
||||
.limit(len(DIALS))
|
||||
)
|
||||
).scalars().all()
|
||||
last = {r.dial: r for r in rows}
|
||||
).scalars().first()
|
||||
if row is not None:
|
||||
last[dial] = row
|
||||
out.append({
|
||||
"surface": name,
|
||||
"floor": await floor_for(user_id, name),
|
||||
|
||||
@@ -189,3 +189,50 @@ def test_the_tool_says_nothing_auto_retunes():
|
||||
assert "calibration" in doc
|
||||
assert "NOTHING IS RETUNED AUTOMATICALLY" in doc
|
||||
assert "unstamped" in doc
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_dial_is_read_per_dial_not_from_the_newest_two_rows():
|
||||
"""The floor's history must not be able to bury the budget's.
|
||||
|
||||
"The newest two rows" and "the newest row of each dial" differ the moment
|
||||
one dial moves more often than the other — which is the normal case, since
|
||||
floors get walked and budgets rarely do. Before this was one query per dial,
|
||||
a surface with three floor changes and one budget change reported the budget
|
||||
as untouched: a WRONG calibration answer rather than a missing one, and
|
||||
wrong in the direction a reader would not think to check.
|
||||
"""
|
||||
session = make_mock_session()
|
||||
per_dial = {
|
||||
"floor": MagicMock(dial="floor", embedding_model="old/model",
|
||||
shape_version=1,
|
||||
to_dict=MagicMock(return_value={"dial": "floor"})),
|
||||
"budget": MagicMock(dial="budget", embedding_model="old/model",
|
||||
shape_version=1,
|
||||
to_dict=MagicMock(return_value={"dial": "budget"})),
|
||||
}
|
||||
calls = []
|
||||
|
||||
def execute(stmt):
|
||||
# The dial is whichever the WHERE clause names; a query that did not
|
||||
# scope by dial would render this stand-in unable to answer, which is
|
||||
# the point.
|
||||
sql = str(stmt.compile(compile_kwargs={"literal_binds": True}))
|
||||
dial = "budget" if "'budget'" in sql else "floor"
|
||||
calls.append(dial)
|
||||
result = MagicMock()
|
||||
result.scalars.return_value.first.return_value = per_dial[dial]
|
||||
return result
|
||||
|
||||
session.execute = AsyncMock(side_effect=execute)
|
||||
with patch.object(rt, "async_session", MagicMock(return_value=session)), \
|
||||
patch.object(rt, "floor_for", AsyncMock(return_value=0.7)), \
|
||||
patch.object(rt, "budget_for", AsyncMock(return_value=3)):
|
||||
out = await rt.current_settings(1)
|
||||
|
||||
assert calls.count("floor") == len(SURFACES)
|
||||
assert calls.count("budget") == len(SURFACES)
|
||||
for row in out:
|
||||
# Both dials tuned under a model that is not the live one.
|
||||
assert row["calibration"]["budget"]["source"] == "tuned"
|
||||
assert row["calibration"]["budget"]["stale"] is True
|
||||
|
||||
Reference in New Issue
Block a user