fix(retrieval): the place arm marks its repeats too, and three tests meet the new contract (#4101)
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 41s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m2s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 41s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m2s
CI & Build / Build & push image (push) Skipped
CI on 5c64ea0 surfaced a half-measure I had shipped and three tests pinning the
semantics it replaces.
**The half-measure.** Only the semantic arm was marking repeats; the write
path's PLACE arm still dropped a nearby snippet that was on the ledger. One
menu, two rules, decided by which arm happened to find a record — and the
marker would then read as a complete account of what the session has met while
covering half the lines. `seen` now means this call's own menu and nothing
else, so both reuse arms mark and neither withholds.
**test_session_dedup_excludes_ids_from_the_reuse_arms** asserted the drop and
the ledger going into the query. Both are the defect; it is renamed and asserts
the marker instead.
**test_a_pulled_snippet_already_seen_is_evidence_not_menu** asserted #7 was
evidence and NOT menu, with the limit widened by one to pay for the drop. The
premise was the hard exclusion; #7 is now both, and the widening is no longer
needed for it.
**test_a_record_this_arm_withheld_itself_is_not_a_near_miss** is the
interesting one. Its property still holds and its setup no longer reaches it:
a ledger repeat comes back from the search and is rendered, so nothing is
withheld and `best_available` describes the bar. The one post-search filter
left is a PULLED record this same call already listed by place — kept in the
query because `resembles` needs its score, dropped from the menu because it is
already on it — so the test is rebuilt on that, and a sibling test pins the
direction that changed, so nulling the score whenever the ledger matches
cannot pass both.
Also stubs `superseded_ids` and `owner_names_for` in the new test file, which
were reaching for Postgres in a unit job.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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'.
|
||||
|
||||
Reference in New Issue
Block a user