diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 506033a..3c63f98 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -1611,7 +1611,13 @@ async def build_write_path_hint( # stay eligible here, which is the whole point of the split. Either way # they join `seen`, so the reuse arms (where the directory query would # surface them again) never re-list a record the sync block owns. - seen: set[int] = set(excluded) + # + # `seen` IS THIS CALL'S OWN MENU, and nothing else (#4101). It used to start + # from `excluded` — the session ledger — which folded two different claims + # into one variable: "already listed a few lines above" and "shown at some + # earlier point in the session". Only the first is a reason to stay quiet. + # The ledger is now a marker instead, on both reuse arms. + seen: set[int] = set() synced: list[dict] = [] for item in here: nid = int(item["id"]) @@ -1625,7 +1631,12 @@ async def build_write_path_hint( if nid in seen: continue seen.add(nid) - placed.append(("nearby", item)) + # MARKED HERE TOO, not just on the semantic arm, because these are one + # menu. A hint where some repeats carry `seen` and others are silently + # dropped — decided by which arm happened to find them — is worse than + # either rule applied consistently: the marker would read as a complete + # account of what the session has met before, and it would not be one. + placed.append(("nearby · seen" if nid in excluded else "nearby", item)) # The stamping feed's "actually pulled it" half (#2791). Read once, before # the semantic arm, because the arm's query doubles as the resemblance @@ -1657,20 +1668,11 @@ async def build_write_path_hint( query = concept_query(query) or query if remaining > 0 and query: t0 = time.perf_counter() - # `seen` CARRIES TWO DIFFERENT CLAIMS, and only one of them is a reason - # to withhold (#4101). It was built as the ledger plus everything this - # call has already rendered, and the arm treated both the same way: - # - # - already in THIS menu (the sync block, the place arm) — listing it - # twice in one hint is noise with no reader it could help; - # - on the LEDGER from an earlier call — a different claim entirely, - # and the one #3750 says must be rendered rather than dropped. - # - # Splitting them is the whole change here. `in_menu` keeps its - # exclusion; the ledger becomes a marker on the line. - in_menu = seen - excluded # Pulled-and-already-listed ids stay in the query (as evidence for # `resembles`) but never in the menu, so the limit has to cover them. + # `seen` is this call's own menu now, which is the only thing left that + # is a reason to withhold (#4101). + in_menu = seen pulled_in_menu = in_menu & set(pulled) _rep_wp: dict = {} hits = await semantic_search_notes( @@ -1948,11 +1950,11 @@ async def build_write_path_hint( for marker, item in menu: # A rendered repeat is not a new surfacing (#4101) — same cut the log # row takes, so this table and `retrieval_logs` keep agreeing about the - # same call (#3668). The place arm never reaches here with one: a nearby - # snippet on the ledger is skipped before it is ever placed. + # same call (#3668). if int(item["id"]) in excluded: continue - arm = "write_path_place" if marker == "nearby" else "write_path_semantic" + arm = ("write_path_place" if marker.startswith("nearby") + else "write_path_semantic") by_arm.setdefault(arm, []).append(int(item["id"])) for arm, ids in by_arm.items(): record_surfaced(user_id=user_id, note_ids=ids, source=arm) diff --git a/tests/test_ledger_references_not_silence.py b/tests/test_ledger_references_not_silence.py index 798755d..be1f161 100644 --- a/tests/test_ledger_references_not_silence.py +++ b/tests/test_ledger_references_not_silence.py @@ -77,7 +77,9 @@ async def _autoinject(hits, exclude_ids, *, rec=None, surf=None): return_value={"enabled": True, "threshold": 0.55, "top_k": 3})), \ patch.object(pc, "semantic_search_notes", search), \ patch.object(pc, "record_retrieval", rec or MagicMock()), \ - patch.object(pc, "record_surfaced", surf or MagicMock()): + patch.object(pc, "record_surfaced", surf or MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "superseded_ids", AsyncMock(return_value=set())): out = await pc.build_autoinject_hint( 1, "postgres pool", project_id=2, exclude_ids=exclude_ids) return out, search @@ -175,7 +177,8 @@ async def _write_path(hits, exclude_ids, *, here=(), rec=None, sync_exclude=()): patch.object(pc.snippets_svc, "list_snippets", listing), \ patch.object(pc, "semantic_search_notes", search), \ patch.object(pc, "record_retrieval", rec or MagicMock()), \ - patch.object(pc, "record_surfaced", MagicMock()): + patch.object(pc, "record_surfaced", MagicMock()), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})): out = await pc.build_write_path_hint( 1, "src/x.py", code=REAL_CODE, exclude_ids=list(exclude_ids), diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index a9e8044..6a4575f 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -310,9 +310,23 @@ async def test_semantic_arm_only_asks_for_the_budget_the_place_arm_left(): @pytest.mark.asyncio -async def test_session_dedup_excludes_ids_from_the_reuse_arms(): +async def test_session_dedup_marks_the_reuse_arms_rather_than_silencing_them(): """exclude_ids governs the REUSE classes — nearby and semantic. (The sync - class has its own channel; see the tests above.)""" + class has its own channel; see the tests above.) + + WHAT CHANGED AND WHY (#4101). This used to assert the opposite: the nearby + hit was dropped and its id pushed into the search's `exclude_ids`. That was + #3750's defect on the note arms — the second time a record was the best + answer, the session got silence, which reads exactly like "nothing is + recorded here". Now the repeat is rendered with a `seen` marker, and the + ledger never reaches the query, so the score the search reports describes + the bar rather than a candidate set the caller had already edited. + + Marked on BOTH reuse arms, which is what this asserts: one menu with two + rules — `seen` on the semantic hits, silence on the nearby ones — would + make the marker read as a complete account of what the session has met, + when it would only cover half the lines. + """ from scribe.services import plugin_context as pc async def _listing(uid, **kw): @@ -327,9 +341,11 @@ async def test_session_dedup_excludes_ids_from_the_reuse_arms(): patch.object(pc, "semantic_search_notes", search), \ patch.object(pc, "record_retrieval", MagicMock()): out = await pc.build_write_path_hint(1, "src/x.py", code=REAL_CODE, exclude_ids=[12]) - # The nearby hit was already surfaced this session → dropped, not repeated. - assert out["note_ids"] == [] - assert 12 in search.await_args.kwargs["exclude_ids"] + assert out["note_ids"] == [12] + assert "seen" in next( + line for line in out["context"].splitlines() if "#12" in line + ) + assert 12 not in (search.await_args.kwargs.get("exclude_ids") or set()) # --- attribution + telemetry ------------------------------------------------- @@ -1264,8 +1280,16 @@ async def test_stamping_needs_named_shapes_and_a_recent_pull(): async def test_a_pulled_snippet_already_seen_is_evidence_not_menu(): """The pulled-then-written flow IS the dedup-excluded flow: the hint offered #7 earlier (so it sits in exclude_ids), the session pulled it, and now - writes code resembling it. #7 must be scored for this payload — and handed - to the stamp as resemblance — without being re-listed in the menu.""" + writes code resembling it. #7 must be scored for this payload and handed to + the stamp as resemblance. + + THE MENU HALF INVERTED AT #4101. This asserted that #7 was evidence and NOT + menu — scored for the stamp, kept out of the lines. That followed from the + ledger being a hard exclusion; it is now a marker, so #7 is both, and the + `limit` no longer needs widening for it because nothing is dropped after + the search. What survives unchanged is the part the stamp depends on: the + pulled id stays in the query and its score reaches `resembles`. + """ from scribe.services import plugin_context as pc search = AsyncMock(return_value=[(0.91, fake_note(id=7, title="pulled", user_id=1, note_type="snippet")), (0.80, fake_note(id=8, title="fresh", user_id=1, note_type="snippet"))]) stamp = AsyncMock(return_value=[{ @@ -1283,15 +1307,15 @@ async def test_a_pulled_snippet_already_seen_is_evidence_not_menu(): 1, "src/x.py", code=REAL_CODE, project_id=4, exclude_ids=[7], stamp_shapes=[("sym", "debounce")], repo_key="git.example.com/a/b", ) - # The query kept #7 eligible (and widened the budget by one for it)... + # The query kept #7 eligible, and needs no extra budget for it now that + # nothing is dropped between the search and the menu. kw = search.call_args.kwargs assert 7 not in kw["exclude_ids"] - assert kw["limit"] == 4 - # ...but the menu still honours the session dedup. - assert out["note_ids"] == [8] - assert "#7" not in "\n".join( - line for line in out["context"].splitlines() if "[similar" in line - ) + assert kw["limit"] == 3 + # ...and the menu carries it, marked. + assert out["note_ids"] == [7, 8] + seven = next(line for line in out["context"].splitlines() if "#7" in line) + assert "seen" in seven # The stamp saw the pull and the resemblance score for this payload. skw = stamp.call_args.kwargs assert skw["pulled"] == {7: skw["pulled"][7]} @@ -1670,9 +1694,47 @@ async def _write_path_row(rec, **kwargs): 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.""" + rejected — while scoring higher than anything the bar ever let through. + + REACHED DIFFERENTLY SINCE #4101, and that is the news. The ledger used to + produce this case and no longer can: a repeat comes back from the search + and is rendered, so nothing is dropped and `best_available` describes the + bar alone. What still produces it is the one post-search filter left — a + PULLED record that this same call already listed by place. It stays in the + query because `resembles` needs its score, and is dropped from the menu + because it is already on it, which is exactly the pre/post split. + + The property is unchanged and worth as much as it ever was; only the setup + that exhibits it moved. Written this way rather than deleted, because the + filter is still there and an arm that reported a score for it would be + making the same false claim about the bar. + """ + from scribe.services import plugin_context as pc rec = MagicMock() - row = await _write_path_row(rec, exclude_ids=[7]) + # Recorded at a sibling file → listed by PLACE in this same call, and + # pulled this session → kept in the query as resemblance evidence. + async def _listing(uid, **kw): + if kw["path"] == "src": + return ([_snippet_item(7, "already listed by place")], 1) + return ([], 0) + + with patch.object(pc, "get_writepath_config", AsyncMock(return_value=_cfg())), \ + patch.object(pc.snippets_svc, "list_snippets", _listing), \ + patch.object(pc, "record_retrieval", rec), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc.shape_ledger_svc, "recent_pulls", + AsyncMock(return_value={7: _ts()})), \ + patch.object(pc.shape_ledger_svc, "stamp_write_path_instances", + 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, + stamp_shapes=[("sym", "debounce")], + ) + row = next(c for c in rec.call_args_list + if c.kwargs["source"] == "write_path") assert row.kwargs["results"] == [], "the hit was withheld, so nothing shown" assert row.kwargs["best_available"] is None, ( @@ -1682,6 +1744,27 @@ async def test_a_record_this_arm_withheld_itself_is_not_a_near_miss(): ) +@pytest.mark.asyncio +async def test_a_ledger_repeat_no_longer_produces_that_case_at_all(): + """The other direction, and the reason the setup above had to move. + + A record on the session ledger now clears the search, is rendered with a + marker, and is counted in `suppressed` — so nothing is withheld and the + reported score describes the bar. Pinned because the tempting way to keep + the old test passing would have been to null the score whenever the ledger + matched, which would delete the measurement #3670 was built for on exactly + the calls where the bar is most worth reading. + """ + rec = MagicMock() + row = await _write_path_row(rec, exclude_ids=[7]) + + assert row.kwargs["results"] == [], "a repeat is not a fresh result" + assert row.kwargs["suppressed"] == 1, "and it is counted rather than lost" + assert row.kwargs["best_available"] == 0.9, ( + "nothing was withheld, so the reported score describes the bar" + ) + + @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'.