fix(telemetry): write_path reported records it withheld itself as near misses (#3739)
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / integration (push) Successful in 32s
CI & Build / Python tests (push) Successful in 1m5s
CI & Build / Build & push image (push) Successful in 24s
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / integration (push) Successful in 32s
CI & Build / Python tests (push) Successful in 1m5s
CI & Build / Build & push image (push) Successful in 24s
Found by verifying the previous fix on live data — the check that fix was
meant to make possible.
write_path near_misses.max 0.822 p90 0.7521
top_score.min 0.6857 so the bar is at or below this
A "rejection" that outscored every acceptance, and not one outlier: the p90
is above the bar too.
#3739's fix keyed on `suppressed_count`, and I justified its NULL branch as
"null means the caller passed its exclusions INTO the search, so the score
is already post-exclusion". That holds for auto_inject and reuse_slot, both
of which log the RAW search output and do their Python filtering after. It
does not hold for write_path, the one note arm that filters TWICE:
`exclude_ids` takes `seen - pulled_seen` into the search, but the
pulled-and-seen ids stay in the query on purpose — the arm's query doubles
as the resemblance test — and are dropped afterwards in Python. So the row
carries a POST-filter count beside a PRE-filter score.
The suppression column cannot rescue it the way it does for the rule arms.
This arm's count would be PARTIAL — covering the drops made here and not
the ones `exclude_ids` made inside the search — and a partial number under
a name that reads as complete is the substitution this milestone exists to
stop.
So it reports null whenever its own filter removed anything: not measured
on this call, because the bar was not the only thing that turned something
away. Calls that withheld nothing keep reporting, which is most of them.
Both directions are asserted. Without the second test, setting the field to
null unconditionally would pass the first while deleting the measurement
#3670 was built for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
This commit is contained in:
@@ -1008,7 +1008,34 @@ async def build_write_path_hint(
|
||||
int(note.id): float(score) for score, note in hits
|
||||
if int(note.id) in pulled
|
||||
}
|
||||
hits = [(s, n) for s, n in hits if int(n.id) not in seen][:remaining]
|
||||
shown = [(s, n) for s, n in hits if int(n.id) not in seen]
|
||||
# WHAT THIS ARM WITHHELD AFTER THE SEARCH ANSWERED, and the reason
|
||||
# `best_available_score` cannot always be reported here (#3739 again,
|
||||
# from the side its fix did not reach).
|
||||
#
|
||||
# This arm is the one note arm that filters TWICE. `exclude_ids` takes
|
||||
# `seen - pulled_seen` into the search, but the pulled-and-seen ids stay
|
||||
# in the query deliberately — `resembles` above needs them — and are
|
||||
# dropped in the line above instead. So the score the search reported is
|
||||
# PRE that drop while the row's `result_count` is POST it, and a record
|
||||
# the session had already been shown could be logged as something the
|
||||
# BAR turned away. Live proof on the first read after #3739 shipped:
|
||||
# write_path's near-miss max was 0.822 while the lowest score it ever
|
||||
# RETURNED was 0.6857 — a "rejection" that beat every acceptance.
|
||||
#
|
||||
# The suppression column cannot rescue it the way it does for the rule
|
||||
# arms: this arm's count would be PARTIAL, covering only the drops made
|
||||
# here and not the ones `exclude_ids` made inside the search, and a
|
||||
# partial number under a name that reads as complete is the substitution
|
||||
# this whole milestone exists to stop.
|
||||
#
|
||||
# So the honest answer is null — "not measured on this call" — whenever
|
||||
# this filter removed anything, because then the bar is not the only
|
||||
# thing that turned something away and the reported score may belong to
|
||||
# a record we withheld ourselves. Calls where nothing was dropped keep
|
||||
# reporting it, which is most of them.
|
||||
withheld_here = len(hits) - len(shown)
|
||||
hits = shown[:remaining]
|
||||
record_retrieval(
|
||||
user_id=user_id, source="write_path", query=query,
|
||||
threshold=cfg["threshold"], limit=remaining,
|
||||
@@ -1016,7 +1043,9 @@ async def build_write_path_hint(
|
||||
# recording it as a notes-only retrieval would misdescribe the
|
||||
# candidate set the threshold is being tuned against.
|
||||
project_id=scope_project, is_task=None, results=hits,
|
||||
best_available=_rep_wp.get("best_available_score"),
|
||||
best_available=(
|
||||
None if withheld_here else _rep_wp.get("best_available_score")
|
||||
),
|
||||
duration_ms=(time.perf_counter() - t0) * 1000.0,
|
||||
)
|
||||
if hits:
|
||||
|
||||
@@ -1526,3 +1526,84 @@ def test_hook_keeps_the_rule_channel_apart_from_the_other_three():
|
||||
assert "(.rule_ids // [])[]?" in src # its own write-back
|
||||
# And it rides the same request as the rest, not a second round trip.
|
||||
assert "${rule_exclude_q}" in src
|
||||
|
||||
|
||||
# ── `best_available` describes the BAR, not this arm's own second filter ──────
|
||||
#
|
||||
# #3739 fixed the rule arms: a record the reader had already been shown was
|
||||
# being logged as something the ranker turned away, which made the near-miss
|
||||
# distribution report scores ABOVE the very threshold it is read against.
|
||||
#
|
||||
# The fix keyed on `suppressed_count`, and its NULL branch was justified by
|
||||
# "null means the caller passed its exclusions INTO the search, so the reported
|
||||
# score is already post-exclusion". That is true of auto_inject and reuse_slot.
|
||||
# It is NOT true here: this is the one note arm that filters twice. `exclude_ids`
|
||||
# takes `seen - pulled_seen` into the search, but the pulled-and-seen ids stay in
|
||||
# the query on purpose (the arm's query doubles as the resemblance test) and are
|
||||
# dropped afterwards in Python.
|
||||
#
|
||||
# Live proof, on the first read after that fix shipped: write_path's near-miss
|
||||
# max was 0.822 while the lowest score it ever RETURNED was 0.6857 — a
|
||||
# "rejection" that beat every acceptance.
|
||||
|
||||
|
||||
def _search_reporting(score, note):
|
||||
"""A stand-in search that fills `report` the way the real one does.
|
||||
|
||||
It ignores `exclude_ids`, which is exactly the condition being reproduced:
|
||||
a record that is in `seen` comes back from the search anyway. In production
|
||||
that happens because `pulled_seen` is deliberately left in the query; here
|
||||
it needs no ledger, and the arm's handling is the same either way.
|
||||
"""
|
||||
async def _search(uid, q, **kw):
|
||||
report = kw.get("report")
|
||||
if report is not None:
|
||||
report["best_available_score"] = score
|
||||
return [(score, note)]
|
||||
return _search
|
||||
|
||||
|
||||
async def _write_path_row(rec, **kwargs):
|
||||
from scribe.services import plugin_context as pc
|
||||
with patch.object(pc, "get_writepath_config", AsyncMock(return_value=_cfg())), \
|
||||
patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), \
|
||||
patch.object(pc, "record_retrieval", rec), \
|
||||
patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \
|
||||
patch.object(pc, "semantic_search_notes",
|
||||
_search_reporting(0.9, fake_note(
|
||||
id=7, title="scored", user_id=1, note_type="snippet"))):
|
||||
await pc.build_write_path_hint(1, "src/x.py", code=REAL_CODE, **kwargs)
|
||||
return next(c for c in rec.call_args_list
|
||||
if c.kwargs["source"] == "write_path")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_record_this_arm_withheld_itself_is_not_a_near_miss():
|
||||
"""The defect: the row's count is POST this arm's filter and the score was
|
||||
captured PRE it, so a withheld record is indistinguishable from one the bar
|
||||
rejected — while scoring higher than anything the bar ever let through."""
|
||||
rec = MagicMock()
|
||||
row = await _write_path_row(rec, exclude_ids=[7])
|
||||
|
||||
assert row.kwargs["results"] == [], "the hit was withheld, so nothing shown"
|
||||
assert row.kwargs["best_available"] is None, (
|
||||
"a 0.9 record this arm withheld itself was reported as the best thing "
|
||||
"the THRESHOLD turned away. It would read as a bar set far too high "
|
||||
"when the bar never rejected it at all (#3739)"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_call_that_withheld_nothing_still_reports_what_the_bar_refused():
|
||||
"""The other half, and what stops the fix being 'never report it'.
|
||||
|
||||
Without this, setting `best_available=None` unconditionally passes the test
|
||||
above while deleting the measurement #3670 was built for.
|
||||
"""
|
||||
rec = MagicMock()
|
||||
row = await _write_path_row(rec)
|
||||
|
||||
assert row.kwargs["best_available"] == 0.9, (
|
||||
"nothing was withheld here, so the reported score describes the bar "
|
||||
"and must survive"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user