From da2383b0794d540307492fb517b02636cea8c949 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 31 Jul 2026 23:01:31 -0400 Subject: [PATCH] fix(retrieval): verify the reserved slot's kind instead of trusting the query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught six failures on f8522fb. Five were fixtures; one was a real assumption. THE REAL ONE: _reserve_slot_for_reuse trusted that a query filtered by note_type could only return reuse kinds. It now checks _record_kind on the way in. That slot exists FOR snippets and processes — one silently spent on something else is worse than no slot at all, because the resulting line is indistinguishable from one that earned its place on score. THE FIXTURES, all the same shape: MagicMock notes with is_task left to auto-create. It is truthy, and _record_kind reads task-ness FIRST — so every mock snippet in three test modules was rendering as "task". Two of those fixtures already carried a comment explaining this exact hazard about `.data`; the same reasoning applies to `.is_task` and nobody had needed it until the menu started naming kinds. One assertion was genuinely stale rather than broken: test_write_path_trigger pinned note_type == "snippet", which was the behaviour the widening replaced. Updated to the new contract, including the task_kind="issue" filter that keeps the open to-do list out. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- src/scribe/services/plugin_context.py | 11 ++++++++--- tests/test_note_usage.py | 3 +++ tests/test_services_plugin_context.py | 3 +++ tests/test_write_path_trigger.py | 13 ++++++++++--- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 32ecbe7..192c3b8 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -334,10 +334,15 @@ async def _reserve_slot_for_reuse( note_type=_REUSE_KINDS, scope="browse", ) - # Belt and braces on top of exclude_ids: a record already on the menu must - # never appear twice, and one slot is all this is entitled to. + # Verify the kind rather than trusting the query that asked for it, and + # dedup on top of exclude_ids. This slot exists FOR reuse kinds — a slot + # silently spent on something else is worse than no slot, because the line + # is indistinguishable from one that earned its place on score. kept_ids = {int(n.id) for _s, n in kept} - fresh = [(s, n) for s, n in reuse if int(n.id) not in kept_ids][:1] + fresh = [ + (s, n) for s, n in reuse + if _record_kind(n) in _REUSE_KINDS and int(n.id) not in kept_ids + ][:1] if not fresh: return kept diff --git a/tests/test_note_usage.py b/tests/test_note_usage.py index 92dd7d9..b57f6b3 100644 --- a/tests/test_note_usage.py +++ b/tests/test_note_usage.py @@ -25,6 +25,9 @@ def _note(nid, title, user_id=1): # See the same note in test_write_path_trigger: an auto-created mock on # `.data` is truthy and would leak into a rendered menu line (#2244). n.data = None + # And on `.is_task`, which the kind marker reads FIRST — truthy there makes + # every one of these snippets read as a task (#2246). + n.is_task, n.task_kind = False, "work" return n diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 5754686..82b96e1 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -19,6 +19,9 @@ def _note(nid, title, user_id=1, note_type="note", is_task=False, task_kind="wor # auto-MagicMock is truthy, so every line would read as another user's task. n.user_id = user_id n.note_type, n.is_task, n.task_kind = note_type, is_task, task_kind + # The write-path menu reads note.data for a language tag; an auto-mock there + # is truthy and renders its repr into the marker. + n.data = None return n diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 2426315..6f709b5 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -23,13 +23,17 @@ def _snippet_item(nid, title, user_id=1): return {"id": nid, "title": title, "user_id": user_id, "note_type": "snippet"} -def _note(nid, title, user_id=1): +def _note(nid, title, user_id=1, note_type="snippet", is_task=False, task_kind="work"): n = MagicMock() n.id, n.title, n.user_id = nid, title, user_id # Explicitly None, not left to MagicMock's auto-attribute: the semantic arm # reads `note.data` for the snippet's language (#2244), and an auto-created # mock there is truthy, so it would render its repr into the menu line. n.data = None + # Same reasoning, second instance: since #2246 this arm returns issues and + # dev-logs too, so the line names the kind. An auto-mock `is_task` is truthy, + # which would label every snippet here "task". + n.note_type, n.is_task, n.task_kind = note_type, is_task, task_kind return n @@ -183,8 +187,11 @@ async def test_semantic_arm_is_snippet_only_and_browse_scoped(): patch.object(pc, "record_retrieval", MagicMock()): await pc.build_write_path_hint(1, "src/x.py", code=REAL_CODE) kwargs = search.await_args.kwargs - # Prior art means snippets — not the dev-log that happens to mention one. - assert kwargs["note_type"] == "snippet" + # Prior art is snippets AND recorded experience (#2246) — an issue saying + # "we tried this and it broke" belongs here. What stays out is the open + # to-do list, which resembles the code and answers nothing. + assert kwargs["note_type"] == ("snippet", "note") + assert kwargs["task_kind"] == "issue" # Nobody asked for this, so it must not reach a one-to-one direct share. assert kwargs["scope"] == "browse"