fix(telemetry): the bar can only be judged from what it rejected (#3670)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / TypeScript typecheck (push) Successful in 23s
CI & Build / integration (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m6s
CI & Build / Build & push image (push) Successful in 28s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / TypeScript typecheck (push) Successful in 23s
CI & Build / integration (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m6s
CI & Build / Build & push image (push) Successful in 28s
`cleared_threshold` was documented as the number to read first. It was a tautology. The search applies the threshold before returning, so every returned result cleared it by construction and a call with no results has no top_score to compare — the condition was true exactly when `result_count > 0`. It was `calls - zero_result_calls` under a name that promised a second opinion, and `zero + cleared == calls` held on all nineteen source/window readings ever taken, today's live seven included. The reading procedure built on it asked the reader to compare a number with itself, and a threshold change was unobservable through it: raise the bar and both numbers move together, so the field could never show a bar set too high. REPLACED, NOT JUST REMOVED. The question the table exists to answer is whether the bar is in the right place, and that is only answerable from the calls that returned NOTHING: how close did the best rejected candidate come? A 0.72 bar turning away a stream of 0.71s is set too high by a hair; the same bar turning away 0.30s is working. Both render as a zero-result call today and nothing separates them, because the losing score is discarded inside the search. So both searches now rank WITHOUT the bar and apply it in Python. The qualifying set is provably identical — rows arrive ordered by distance, so every above-bar row sorts ahead of every below-bar one, and an over-fetch that returned N above-bar rows returns the same N plus some losers. What changes is that the losers are visible instead of dropped in the query. `report` carries the score out without changing what a search RETURNS: eight of eleven call sites want hits and nothing else. New column (migration 0096), nullable and unbackfilled. A row written before this genuinely does not know, and a 0.0 would read as "the corpus held nothing remotely relevant" — a claim invented out of a caller's silence, which is the substitution this whole milestone corrects. The new aggregate is a percentile_cont WITHIN GROUP over a CASE, one step from the shape that produced #2663, where a rejected query was swallowed by the broad except and every counter read zero. It carries an integration guard for that reason: only real Postgres can say it parses, and the symptom of failure is silence. Also adds a guard that no int field in a bucket equals `calls - zero_result_calls`. That identity is what `cleared_threshold` satisfied for its whole life, and it survived because it had its own name and nobody added the two numbers beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
This commit is contained in:
@@ -102,14 +102,14 @@ def test_the_readout_reports_unmeasured_suppression_as_none():
|
||||
zeroed dict: a zeroed dict states a measurement nobody made."""
|
||||
from scribe.services.retrieval_telemetry import _bucket
|
||||
|
||||
# calls, zero, cleared, p10, p50, p90, min, max, avg_n, dur,
|
||||
# measured, supp_calls, supp_zero
|
||||
unmeasured = _bucket([326, 114, 212, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0])
|
||||
# calls, zero, p10, p50, p90, min, max, avg_n, dur,
|
||||
# measured, supp_calls, supp_zero, miss_calls, miss_p50, miss_p90, miss_max
|
||||
unmeasured = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 0, None, None, None])
|
||||
assert unmeasured["suppression"] is None
|
||||
|
||||
measured = _bucket([35, 34, 1, 0.75, 0.75, 0.75, 0.75, 0.75, 0.03, 51.9,
|
||||
35, 9, 9])
|
||||
measured = _bucket([35, 34, 0.75, 0.75, 0.75, 0.75, 0.75, 0.03, 51.9,
|
||||
35, 9, 9, 0, None, None, None])
|
||||
assert measured["suppression"] == {
|
||||
"measured_calls": 35,
|
||||
"calls_with_suppression": 9,
|
||||
@@ -224,7 +224,10 @@ async def test_retrieval_summary_reads_what_the_writer_wrote(_dispose_engine):
|
||||
ai = out["sources"]["auto_inject"]
|
||||
assert ai["calls"] == 4
|
||||
assert ai["zero_result_calls"] == 1
|
||||
assert ai["cleared_threshold"] == 2 # 0.91 and 0.72, not 0.40
|
||||
assert "cleared_threshold" not in ai, (
|
||||
"the tautology is back: it was true exactly when result_count > 0, "
|
||||
"so it reported nothing zero_result_calls did not (#3670)"
|
||||
)
|
||||
# p50 over the three scored calls; the empty one contributes no score.
|
||||
assert ai["top_score"]["p50"] == pytest.approx(0.72, abs=1e-4)
|
||||
assert ai["top_score"]["min"] == pytest.approx(0.40, abs=1e-4)
|
||||
@@ -847,3 +850,176 @@ async def test_a_section_is_complete_only_from_its_latest_contributor(
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(RuleUsageEvent).where(RuleUsageEvent.user_id == UID))
|
||||
await s.commit()
|
||||
|
||||
|
||||
# ─── the bar can only be judged from what it rejected (#3670) ────────────────
|
||||
#
|
||||
# `cleared_threshold` was the number the docstring told a reader to look at
|
||||
# first. It was `calls - zero_result_calls` under another name: the search
|
||||
# applies the bar before returning, so every returned result cleared it by
|
||||
# construction and a call with nothing has no score to compare.
|
||||
# `zero_result_calls + cleared_threshold == calls` held on all nineteen
|
||||
# source/window readings ever taken — no near-misses, no exceptions.
|
||||
#
|
||||
# What replaced it cannot go the same way, and the reason is structural rather
|
||||
# than careful naming: `near_misses` is measured on the calls the bar TURNED
|
||||
# AWAY, using a score the bar never saw. No arrangement of `calls`,
|
||||
# `zero_result_calls` and `result_count` derives it.
|
||||
|
||||
|
||||
def test_a_call_that_returned_nothing_still_records_what_it_nearly_showed():
|
||||
"""The whole point, at the payload grain.
|
||||
|
||||
This is the row a threshold is tuned from and the one that used to carry no
|
||||
score at all: `top_score` and `min_score` are both null here, correctly, and
|
||||
a reader was left unable to tell a bar rejecting 0.71s from one rejecting
|
||||
0.30s. Both render as a zero-result call.
|
||||
"""
|
||||
p = _build_payload(
|
||||
user_id=1, source="pre_tool_rule", query="git push --force",
|
||||
threshold=0.72, limit=1, project_id=None, is_task=None,
|
||||
results=[], duration_ms=None, best_available=0.7104,
|
||||
)
|
||||
assert p["result_count"] == 0
|
||||
assert p["top_score"] is None, "nothing was shown, so nothing has a top score"
|
||||
assert p["best_available_score"] == 0.7104, (
|
||||
"the losing score was discarded — the only figure that survives a call "
|
||||
"returning nothing, and the only one a bar can be judged from"
|
||||
)
|
||||
|
||||
|
||||
def test_a_caller_that_did_not_measure_the_near_miss_stores_null():
|
||||
"""Null, never 0.0. A zero here reads as "the corpus held nothing remotely
|
||||
relevant" — a claim about the corpus invented out of a caller's silence,
|
||||
which is #3311's substitution in a new field."""
|
||||
p = _build_payload(
|
||||
user_id=1, source="auto_inject", query="q", threshold=0.6,
|
||||
limit=3, project_id=None, is_task=None, results=[], duration_ms=None,
|
||||
)
|
||||
assert p["best_available_score"] is None
|
||||
|
||||
|
||||
def test_the_readout_reports_unmeasured_near_misses_as_none():
|
||||
"""`_bucket`'s half of the same discipline, and the reason it is a block
|
||||
rather than three loose keys: old rows predate the column, so a window can
|
||||
legitimately contain declines nobody measured."""
|
||||
from scribe.services.retrieval_telemetry import _bucket
|
||||
|
||||
# calls, zero, p10, p50, p90, min, max, avg_n, dur,
|
||||
# measured, supp_calls, supp_zero, miss_calls, miss_p50, miss_p90, miss_max
|
||||
none_measured = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 0, None, None, None])
|
||||
assert none_measured["near_misses"] is None
|
||||
|
||||
measured = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 114, 0.61, 0.7104, 0.7189])
|
||||
assert measured["near_misses"] == {
|
||||
"measured_calls": 114,
|
||||
"p50": 0.61,
|
||||
"p90": 0.7104,
|
||||
"max": 0.7189,
|
||||
}
|
||||
|
||||
|
||||
def test_the_readout_carries_no_field_derivable_from_its_neighbours():
|
||||
"""The guard that would have caught #3670 on the day it shipped.
|
||||
|
||||
`cleared_threshold` survived because it had its own name and its own
|
||||
docstring paragraph, and nobody added the two numbers beside it. This
|
||||
asserts the identity that held on every reading ever taken — and if a
|
||||
future field reintroduces it under a new name, the sum below is where it
|
||||
shows up.
|
||||
"""
|
||||
from scribe.services.retrieval_telemetry import _bucket
|
||||
|
||||
b = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 114, 0.61, 0.71, 0.72])
|
||||
derivable = {
|
||||
k for k, v in b.items()
|
||||
if isinstance(v, int) and not isinstance(v, bool)
|
||||
and k not in ("calls", "zero_result_calls")
|
||||
and v == b["calls"] - b["zero_result_calls"]
|
||||
}
|
||||
assert not derivable, (
|
||||
f"{sorted(derivable)} equals calls - zero_result_calls on this row. "
|
||||
f"That is how `cleared_threshold` read for its whole life (#3670): a "
|
||||
f"figure presented as an independent measurement that a reader can "
|
||||
f"compute from the two numbers next to it. Either it is a tautology, "
|
||||
f"or this fixture happens to make it look like one — check which "
|
||||
f"before adding an exemption."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_near_miss_distribution_is_a_query_postgres_accepts(_dispose_engine):
|
||||
"""Integration, and NOT belt-and-braces on the unit tests above.
|
||||
|
||||
`near_misses` is a `percentile_cont(...) WITHIN GROUP` over a CASE
|
||||
expression, inside the same grouped aggregate that already carries four
|
||||
other CASEs. That is within one step of the shape that produced #2663 — a
|
||||
query the database rejected, swallowed by this module's broad `except`, so
|
||||
every counter read zero in production while the writes landed fine and the
|
||||
mocked tests passed. Only a real Postgres can say this parses, and if it
|
||||
does not, the symptom is silence rather than an error.
|
||||
|
||||
The numbers are chosen so a bar at 0.72 is visibly the wrong bar: three
|
||||
declines at 0.70, 0.71 and 0.7189, none of which a reader could see before.
|
||||
"""
|
||||
from sqlalchemy import delete
|
||||
|
||||
from scribe.models import async_session
|
||||
from scribe.models.retrieval_log import RetrievalLog
|
||||
from scribe.services.retrieval_telemetry import (
|
||||
_insert_retrieval_log, retrieval_summary,
|
||||
)
|
||||
|
||||
UID = 990079
|
||||
for best in (0.70, 0.71, 0.7189):
|
||||
await _insert_retrieval_log(_build_payload(
|
||||
user_id=UID, source="pre_tool_rule", query="git push", threshold=0.72,
|
||||
limit=1, project_id=None, is_task=None, results=[],
|
||||
duration_ms=4.0, best_available=best,
|
||||
))
|
||||
# A call that DID show something. Its best-available equals its top score,
|
||||
# so including it would drag the distribution toward the scores the bar
|
||||
# already accepts — the population has to be the declines alone.
|
||||
await _insert_retrieval_log(_build_payload(
|
||||
user_id=UID, source="pre_tool_rule", query="curl", threshold=0.72,
|
||||
limit=1, project_id=None, is_task=None,
|
||||
results=[(0.88, _note(7))], duration_ms=4.0, best_available=0.88,
|
||||
))
|
||||
# An unmeasured decline, standing in for every row written before #3670.
|
||||
await _insert_retrieval_log(_build_payload(
|
||||
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,
|
||||
))
|
||||
|
||||
try:
|
||||
out = await retrieval_summary(UID, days=30)
|
||||
assert out["read_failed"] is False, (
|
||||
"the aggregate did not execute — a rejected query here reads as "
|
||||
"zeros everywhere, which is #2663 exactly"
|
||||
)
|
||||
src = out["sources"]["pre_tool_rule"]
|
||||
assert src["calls"] == 5
|
||||
assert src["zero_result_calls"] == 4
|
||||
|
||||
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)"
|
||||
)
|
||||
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 0.70 <= nm["p50"] <= 0.7189
|
||||
finally:
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == UID))
|
||||
await s.commit()
|
||||
|
||||
Reference in New Issue
Block a user