From a165483b92494e49a419ec0a361aab7c51e994fa Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 8 Sep 2026 16:42:51 -0400 Subject: [PATCH] fix(telemetry): a repeat is not a rejection, and near_misses counted it as one (#3739) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught on the first live read after deploying #3670. The readout contradicted itself: pre_tool_rule top_score.min 0.7204 the lowest score ever RETURNED near_misses.max 0.7457 "rejected", but scored higher `best_available_score` is measured pre-threshold, which is right, but for the rule arms it is also PRE-EXCLUSION, which is not. The note arms pass `exclude_ids` into semantic_search_notes so their score is already post-exclusion and clean; `semantic_search_rules` takes no such parameter, so the rule arms filter in Python after the search and a rule that cleared the bar and was dropped as a repeat still reported its score on a zero-result row. That is #3497's distinction — a ranker decline versus a reader already ahead of it — reintroduced one level up, inside the field built to replace a tautology. The population now also requires `suppressed_count IS NULL OR = 0`. The NULL arm is principled rather than permissive: null means the caller filtered INSIDE the search, which is exactly the case where the reported score cannot be contaminated. Deliberately conservative — a call carrying both a repeat and a lower genuine miss is dropped whole, losing that point. It undercounts; it cannot corrupt, which is the right way round for a number read against a bar. It also makes `near_misses.max < threshold` true BY CONSTRUCTION rather than by fixture: an above-bar candidate nobody excluded would have been returned, so its call is not in the population at all. THE TEST DID NOT CATCH THIS, and that is the part worth keeping. The assertion `nm["max"] < 0.72` was already there, with exactly the right intent. It passed because the fixture contained no suppressed call — the guard held because the breaking shape was absent, not because the code was right. Rule 167's stated failure mode, in a test written while citing rule 167. The fixture now builds that shape: a 0.9 hit dropped as a repeat, which lands in the population and drags `max` above the threshold unless the predicate excludes it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ --- src/scribe/mcp/tools/search.py | 10 +++-- src/scribe/services/retrieval_telemetry.py | 48 ++++++++++++++++++---- tests/test_services_retrieval_telemetry.py | 31 ++++++++++---- 3 files changed, 69 insertions(+), 20 deletions(-) diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 20cd999..bccd208 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -169,10 +169,12 @@ async def retrieval_telemetry(days: int = 30) -> dict: `avg_result_count` and `p90_duration_ms`. THE NUMBER TO READ FIRST IS `near_misses.p90`, AGAINST THE THRESHOLD IN - FORCE FOR THAT SURFACE. It is measured only on the calls that returned - NOTHING, on the best score the ranker reached before the bar rejected it — - so it is the one figure here that says something the bar cannot make true - by construction. A bar at 0.72 turning away a stream of 0.71s is set too + FORCE FOR THAT SURFACE. It is measured on the calls the BAR turned away — + zero-result calls, minus the ones whose zero was a repeat the reader had + already been shown — using the best score the ranker reached before the bar + rejected it. So it is the one figure here that says something the bar + cannot make true by construction, and `max` is always below the threshold: + an above-bar candidate nobody excluded would have been returned. A bar at 0.72 turning away a stream of 0.71s is set too high by a hair and the surface is losing hits it should have had. The same bar turning away 0.30s is working, and the corpus simply had nothing. Both render as a zero-result call, and nothing else in this readout tells them diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index 18e94df..577aac8 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -360,15 +360,45 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict: } zero = case((RetrievalLog.result_count == 0, 1), else_=0) - # THE NEAR-MISS POPULATION: calls that returned nothing AND recorded what - # the bar turned away. Both conditions matter. Restricting to zero-result - # calls is what makes the number say something the bar cannot fix by - # construction — on a call that returned something, `best_available_score` - # equals `top_score` and adds nothing. Requiring the column to be non-null - # keeps rows written before #3670 out of the sample rather than letting - # them read as scoreless declines. - declined = (RetrievalLog.result_count == 0) & ( - RetrievalLog.best_available_score.isnot(None) + # THE NEAR-MISS POPULATION: calls that returned nothing BECAUSE THE BAR + # TURNED SOMETHING AWAY, and recorded what it was. Three conditions, and + # the third was missing for one deploy (#3739). + # + # Zero-result only: on a call that returned something, + # `best_available_score` equals `top_score` and adds nothing. + # + # Non-null only: rows written before #3670 genuinely do not know, and must + # not read as scoreless declines. + # + # AND NOT A REPEAT. A zero-result call is two unrelated events — the ranker + # found nothing above the bar, or it found only what this session had + # already been shown — and just the first says anything about the bar. That + # is the whole of #3497, and #3670 reintroduced the conflation one level up: + # the rule arms filter exclusions in PYTHON, after the search, so a rule + # that cleared the bar and was dropped as a repeat still reported a high + # `best_available_score` on a zero-result row. Live proof, first read after + # deploy: pre_tool_rule's near-miss max was 0.7457 while the lowest score it + # ever RETURNED was 0.7204 — a "rejection" that outscored acceptances. + # + # The NULL arm is principled, not permissive: `suppressed_count IS NULL` + # means the caller passed its exclusions INTO the search, which is exactly + # the case where the reported score is already post-exclusion and cannot be + # contaminated. Note arms stay measured; rule arms get cleaned. + # + # Deliberately conservative: a call carrying both a repeat and a lower + # genuine miss is dropped whole, losing that point. It undercounts; it + # cannot corrupt — the right way round for a number read against a bar. + # + # This also makes `near_misses.max < threshold` true BY CONSTRUCTION. An + # above-bar candidate that was not excluded would have been returned, so + # its call is not in this population at all. + declined = ( + (RetrievalLog.result_count == 0) + & (RetrievalLog.best_available_score.isnot(None)) + & ( + RetrievalLog.suppressed_count.is_(None) + | (RetrievalLog.suppressed_count == 0) + ) ) miss = case((declined, 1), else_=0) # `best_available_score` only for those rows; NULL elsewhere, and diff --git a/tests/test_services_retrieval_telemetry.py b/tests/test_services_retrieval_telemetry.py index ff5b8fd..243744c 100644 --- a/tests/test_services_retrieval_telemetry.py +++ b/tests/test_services_retrieval_telemetry.py @@ -994,6 +994,17 @@ async def test_the_near_miss_distribution_is_a_query_postgres_accepts(_dispose_e user_id=UID, source="pre_tool_rule", query="ls", threshold=0.72, limit=1, project_id=None, is_task=None, results=[], duration_ms=4.0, )) + # THE CASE WHOSE ABSENCE LET THIS GUARD PASS OVER BROKEN CODE (#3739). + # A zero-result call whose zero was a REPEAT, not a rejection: the ranker + # cleared the bar at 0.9 and the session had already been shown that rule, + # so the arm dropped it in Python after the search. Without the suppression + # arm of the predicate this row lands in the near-miss population and drags + # `max` to 0.9 — above the very threshold the field is read against. + await _insert_retrieval_log(_build_payload( + user_id=UID, source="pre_tool_rule", query="git commit", threshold=0.72, + limit=1, project_id=None, is_task=None, results=[], duration_ms=4.0, + best_available=0.9, suppressed=1, + )) try: out = await retrieval_summary(UID, days=30) @@ -1002,22 +1013,28 @@ async def test_the_near_miss_distribution_is_a_query_postgres_accepts(_dispose_e "zeros everywhere, which is #2663 exactly" ) src = out["sources"]["pre_tool_rule"] - assert src["calls"] == 5 - assert src["zero_result_calls"] == 4 + assert src["calls"] == 6 + assert src["zero_result_calls"] == 5 nm = src["near_misses"] assert nm is not None, "the near-miss block did not survive the query" assert nm["measured_calls"] == 3, ( - "the population is declines that RECORDED a score: three measured, " - "one unmeasured (excluded, not counted as a scoreless decline), and " - "one call that showed something (excluded — its best-available is " - "just its top score and says nothing about the bar)" + "the population is declines the BAR caused, that recorded a score. " + "Three qualify. Excluded: the unmeasured row (predates the column, " + "not a scoreless decline), the call that showed something (its " + "best-available is just its top score), and the REPEAT — a zero " + "the reader caused, not the bar (#3739)" ) assert nm["max"] == pytest.approx(0.7189, abs=1e-4), ( "the closest thing the bar turned away — 0.7189 against a 0.72 " "threshold, which is the reading the whole field exists to give" ) - assert nm["max"] < 0.72, "a near miss that cleared the bar is not a miss" + assert nm["max"] < 0.72, ( + "a rejection that outscores the bar is not a rejection. This is " + "structural once the suppression arm is in the predicate: an " + "above-bar candidate that was not excluded would have been " + "RETURNED, so its call cannot be in this population (#3739)" + ) assert 0.70 <= nm["p50"] <= 0.7189 finally: async with async_session() as s: