fix(telemetry): a search that never ran is not a decline (#3765)
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / Python lint (push) Successful in 4s
CI & Build / TypeScript typecheck (push) Successful in 46s
CI & Build / integration (push) Successful in 52s
CI & Build / Python tests (push) Successful in 1m21s
CI & Build / Build & push image (push) Successful in 36s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / Python lint (push) Successful in 4s
CI & Build / TypeScript typecheck (push) Successful in 46s
CI & Build / integration (push) Successful in 52s
CI & Build / Python tests (push) Successful in 1m21s
CI & Build / Build & push image (push) Successful in 36s
`best_available_score` was added by #3670 so a bar could be judged from what it rejected, and it arrived null on four unrelated causes: the corpus offered nothing, the query was empty, the embedder was down, or the DATABASE QUERY FAILED. Only the first is a measurement. The fourth is the #2663 shape — a swallowed failure rendering as a clean zero — inside the field added to fix an instance of the #2663 shape. Found while trying to explain why reuse_slot returned nothing on 45 of 45 calls, and auto_inject on 153 of 161. That investigation is still open; what it established first is that the readout could not answer it. THE FIX IS NOT A NEW COLUMN. A call that never searched writes no row, so every remaining null means one thing: searched, and nothing came close. That is the convention the pre-tool arm already follows for a blank command — "a row here would report a call that never happened and drag the clear-rate down with phantom declines" — extended from the case a caller can see in advance to the ones only the search knows about. Both searches stamp `report["searched"]` FALSE before anything can return and True only where a real result set exists, so every early return leaves it false. It has to be the first thing done to the dict: a return added above that line would leave the key absent. ABSENT IS A THIRD STATE AND IT DEFAULTS TO TRUE. A caller that passes no report cannot know, and the safe reading there is the old behaviour. Only a real search can report False, so absent means "nobody asked" and never "it failed" — which is also why 66 existing mocked searches across twelve test files keep working unchanged rather than being rewritten to simulate a flag they do not care about. A FAILURE IS NOT MADE INVISIBLE. semantic_search_notes already logs a WARNING on a query failure, which is where a broken search belongs: a counter cannot say "I am broken" without a reader already trusting it. Three tests, and the middle one is what makes them discriminate — a blanket `return` passes the first and fails the second, because a call that searched and came back empty is the only evidence a threshold is too high (#3497). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
This commit is contained in:
@@ -1040,3 +1040,83 @@ async def test_the_near_miss_distribution_is_a_query_postgres_accepts(_dispose_e
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == UID))
|
||||
await s.commit()
|
||||
|
||||
|
||||
# ─── a search that never ran is not a decline (#3765) ────────────────────────
|
||||
#
|
||||
# `best_available_score` was added by #3670 so a bar could be judged from what
|
||||
# it rejected, and it arrived null on four unrelated causes: the corpus offered
|
||||
# nothing, the query was empty, the embedder was down, or the DATABASE QUERY
|
||||
# FAILED. Only the first is a measurement. The fourth is the #2663 shape — a
|
||||
# swallowed failure rendering as a clean zero — inside the field added to fix
|
||||
# an instance of the #2663 shape.
|
||||
#
|
||||
# The fix is not a new column. A call that never searched writes no row, so
|
||||
# every remaining null means one thing. That is the convention the pre-tool arm
|
||||
# already follows for a blank command, extended from the case a caller can see
|
||||
# in advance to the ones only the search knows about.
|
||||
|
||||
|
||||
def test_a_search_that_never_ran_writes_no_row():
|
||||
"""The whole fix, at the one place it is enforced.
|
||||
|
||||
`record_retrieval` is fire-and-forget and returns None either way, so the
|
||||
observable is the payload never being built — asserted through the builder
|
||||
rather than the scheduler, which needs a running loop.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
import scribe.services.retrieval_telemetry as rt
|
||||
|
||||
with patch.object(rt, "_build_payload") as build:
|
||||
rt.record_retrieval(
|
||||
user_id=1, source="auto_inject", query="q", threshold=0.6,
|
||||
limit=3, project_id=None, is_task=None, results=[],
|
||||
searched=False,
|
||||
)
|
||||
assert not build.called, (
|
||||
"a search that never ran was recorded as a retrieval. It would "
|
||||
"read as a ranker decline — evidence about a threshold, from a "
|
||||
"call where no threshold was ever applied (#3765)"
|
||||
)
|
||||
|
||||
|
||||
def test_a_search_that_ran_and_found_nothing_still_writes_its_row():
|
||||
"""The half that stops the fix from being 'log less'.
|
||||
|
||||
A call that searched and came back empty is the ONLY evidence a threshold
|
||||
is too high (#3497). Dropping it too would trade one silent distortion for
|
||||
another, and this assertion is what makes the pair discriminate: a blanket
|
||||
`return` passes the test above and fails this one.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
import scribe.services.retrieval_telemetry as rt
|
||||
|
||||
with patch.object(rt, "_build_payload") as build:
|
||||
rt.record_retrieval(
|
||||
user_id=1, source="auto_inject", query="q", threshold=0.6,
|
||||
limit=3, project_id=None, is_task=None, results=[],
|
||||
searched=True,
|
||||
)
|
||||
assert build.called, "a genuine zero-result call must still be recorded"
|
||||
|
||||
|
||||
def test_a_caller_that_never_asked_is_assumed_to_have_searched():
|
||||
"""`searched` defaults True, and the default is load-bearing.
|
||||
|
||||
A caller that passes no `report` cannot know, and the safe reading there is
|
||||
the old behaviour — log it. Only a REAL search can report False, because it
|
||||
stamps the key before anything can return. An absent key therefore means
|
||||
"nobody asked", never "it failed".
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
import scribe.services.retrieval_telemetry as rt
|
||||
|
||||
with patch.object(rt, "_build_payload") as build:
|
||||
rt.record_retrieval(
|
||||
user_id=1, source="mcp_search", query="q", threshold=0.45,
|
||||
limit=10, project_id=None, is_task=None, results=[],
|
||||
)
|
||||
assert build.called
|
||||
|
||||
Reference in New Issue
Block a user