diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 9c873c7..e5c6890 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -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: diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 72ad0d6..5b6f359 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -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" + )