diff --git a/src/scribe/services/retrieval_tuning.py b/src/scribe/services/retrieval_tuning.py index 787f166..f6a8682 100644 --- a/src/scribe/services/retrieval_tuning.py +++ b/src/scribe/services/retrieval_tuning.py @@ -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), diff --git a/tests/test_calibration_stamp.py b/tests/test_calibration_stamp.py index 09434e6..6e2d530 100644 --- a/tests/test_calibration_stamp.py +++ b/tests/test_calibration_stamp.py @@ -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