diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index a5e3c55..187792a 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -127,12 +127,19 @@ async def create_snippet( force: Bypass the near-duplicate gate (see below). Returns the created snippet (including a parsed `snippet` field), OR — when a - near-duplicate snippet already exists and force is false — {"duplicate": true, + duplicate already exists and force is false — {"duplicate": true, "existing_id": ..., "message": ...} and nothing is created. When that happens and it really is the same reusable thing found in another place, prefer merge_snippets(existing_id, [new...]) — or record then merge — to unify them into ONE canonical record (which then carries every call site as a location), rather than forcing a second copy with force=true. + + WHAT THE GATE MATCHES ON. Exact identity first — an existing snippet at the + same repo · path · symbol, or holding byte-identical code. Those are certain, + and force is almost never the right answer to them. Only then a semantic + check, held to a high bar so that VARIANTS of one component are not refused: + `.btn-primary` and `.btn-secondary` read alike and are two different things, + so record both (#2518). """ if not (name or "").strip() or not (code or "").strip(): raise ValueError("create_snippet requires a non-empty name and code") @@ -148,6 +155,10 @@ async def create_snippet( dup = await dedup_svc.find_duplicate_note( uid, title, body, project_id=project_id or None, is_task=False, note_type=snippets_svc.SNIPPET_NOTE_TYPE, + # The artefact itself, not just its description — the gate compares + # location and code before it compares prose (#2518). + code=code, + locations=snippets_svc.resolve_locations(repo, path, symbol, locations), ) if dup is not None: return dedup_svc.duplicate_response(dup, "snippet") diff --git a/src/scribe/routes/snippets.py b/src/scribe/routes/snippets.py index 0fb013c..b5f3b7c 100644 --- a/src/scribe/routes/snippets.py +++ b/src/scribe/routes/snippets.py @@ -112,6 +112,15 @@ async def create_snippet_route(): project_id=project_id, is_task=False, note_type=snippets_svc.SNIPPET_NOTE_TYPE, + # Matched on the artefact — location and code — before prose, the + # same way the MCP create path does (#2518). Both surfaces must + # apply the identical gate or the web UI becomes the way to record + # a duplicate the agent would have been stopped from writing. + code=data.get("code", ""), + locations=snippets_svc.resolve_locations( + data.get("repo", ""), data.get("path", ""), data.get("symbol", ""), + data.get("locations"), + ), ) if dup is not None: return jsonify(dedup_svc.duplicate_response(dup, "snippet")), 409 diff --git a/src/scribe/services/dedup.py b/src/scribe/services/dedup.py index fd91894..524c72d 100644 --- a/src/scribe/services/dedup.py +++ b/src/scribe/services/dedup.py @@ -48,6 +48,27 @@ _MIN_BODY_FOR_SEMANTIC = 200 # noise). Matches the 0.90 the pre-pivot dedup settled on. _SEMANTIC_THRESHOLD = 0.90 +# SNIPPETS ARE MEASURED DIFFERENTLY, and #2518 is why. A snippet's embedded +# document is mostly PROSE ABOUT the code — name, when-to-reach-for-it, +# signature, the comments explaining the choice — with the artefact itself a +# minority of the text. Two measurements on the same corpus: +# +# .btn-danger vs .btn-danger-outline 0.92 siblings, blocked (false positive) +# .btn-primary re-recorded verbatim +# under a different name <0.90 a literal copy, ALLOWED THROUGH +# +# The second is the one that settles it. Identical code at an identical +# repo·path·symbol sailed past the gate because the description differed, while +# two deliberately-parallel variants were refused because theirs did not. The +# arm is not mis-tuned; it is reading the wrong field, and no threshold fixes +# that — lowering it blocks more siblings, raising it allows more copies. +# +# So: STRUCTURE decides, and the semantic arm becomes a backstop set above the +# band where legitimate variants live (0.92 observed). It still catches a +# genuine reword that shares neither location nor code, which is the case the +# structural signals cannot see. +_SNIPPET_SEMANTIC_THRESHOLD = 0.96 + @dataclass class DuplicateMatch: @@ -58,25 +79,124 @@ class DuplicateMatch: reason: str # "title" | "semantic" +# How each signal describes itself when it blocks a write. The structural ones +# are CERTAIN, so they say what was matched instead of hedging with "similar" — +# and they point at merge, not update, because two records of one artefact is +# what merge exists to fold back together. +_REASON_PHRASING = { + "location": ( + "is already recorded at that exact repo · path · symbol", + "Update it (update_{kind}), or if you meant to record a second call " + "site, use merge_snippets so one record carries both locations.", + ), + "code": ( + "already holds identical code", + "Update it (update_{kind}) rather than keeping two copies that must " + "then be kept in step by hand.", + ), +} + + def duplicate_response(dup: "DuplicateMatch", kind: str) -> dict: """Standard 'blocked — update instead' payload returned by a create tool - when the gate finds a near-duplicate. `kind` is 'note' or 'task' (drives the - update_ hint).""" + when the gate finds a near-duplicate. `kind` is 'note', 'task' or 'snippet' + (drives the update_ hint).""" + phrasing = _REASON_PHRASING.get(dup.reason) + if phrasing: + claim, advice = phrasing + message = ( + f'An existing {kind} (id {dup.id}: "{dup.title}") {claim}. ' + f"{advice.format(kind=kind)} If this really is a distinct {kind}, " + f"retry with force=true." + ) + else: + message = ( + f'A {dup.reason}-similar {kind} already exists (id {dup.id}: ' + f'"{dup.title}"). Prefer UPDATING it (update_{kind}) over creating a ' + f"near-duplicate. If this really is a distinct {kind}, retry with " + f"force=true." + ) return { "duplicate": True, "existing_id": dup.id, "existing_title": dup.title, "similarity": dup.similarity, "match": dup.reason, - "message": ( - f'A {dup.reason}-similar {kind} already exists (id {dup.id}: ' - f'"{dup.title}"). Prefer UPDATING it (update_{kind}) over creating a ' - f"near-duplicate. If this really is a distinct {kind}, retry with " - f"force=true." - ), + "message": message, } +async def _find_snippet_by_structure( + user_id: int, + code: str, + locations: list[dict] | None, + project_id: int | None, +) -> DuplicateMatch | None: + """Exact-identity duplicate of an incoming snippet, or None. + + Two signals, both index-served off `notes.data` and both CERTAIN rather than + probabilistic — which is what the semantic arm could not be (#2518): + + location the same named thing in the same file. Requires BOTH path and + symbol: a path alone is a directory of many artefacts, and + matching on it would refuse every second snippet from one file. + code byte-identical code, wherever it lives. Uses the same + fingerprint the drift check uses, so "identical" means the same + thing in both places. + + Fail-open like the rest of this module: a failed lookup lets the write + through rather than blocking on an infrastructure problem. + """ + from scribe.services.knowledge import location_jsonpath + from scribe.services.snippets import code_sha + + identifying = [ + loc for loc in (locations or []) + if (loc.get("path") or "").strip() and (loc.get("symbol") or "").strip() + ] + if not identifying and not (code or "").strip(): + return None # nothing to match on — don't open a session for it + + def _scoped(stmt): + stmt = stmt.where( + Note.user_id == user_id, + Note.deleted_at.is_(None), + Note.note_type == SNIPPET_NOTE_TYPE, + ) + # Same scoping rule as the title and semantic arms: a project's records + # compare only within that project, orphans only to orphans. + return (stmt.where(Note.project_id == project_id) if project_id is not None + else stmt.where(Note.project_id.is_(None))) + + try: + async with async_session() as session: + for loc in identifying: + parts = { + "path": (loc["path"]).strip(), + "symbol": (loc["symbol"]).strip(), + } + repo = (loc.get("repo") or "").strip() + if repo: + parts["repo"] = repo + stmt = _scoped(select(Note)).where( + Note.data.path_exists(location_jsonpath(parts)) + ) + hit = (await session.execute(stmt.limit(1))).scalars().first() + if hit is not None: + return DuplicateMatch(hit.id, hit.title, 1.0, "location") + + if (code or "").strip(): + stmt = _scoped(select(Note)).where( + Note.data["code_sha"].astext == code_sha(code) + ) + hit = (await session.execute(stmt.limit(1))).scalars().first() + if hit is not None: + return DuplicateMatch(hit.id, hit.title, 1.0, "code") + except Exception: + logger.debug("snippet structural dedup skipped — query failed", exc_info=True) + return None + + async def find_duplicate_note( user_id: int, title: str, @@ -84,11 +204,19 @@ async def find_duplicate_note( project_id: int | None = None, is_task: bool | None = None, note_type: str = "note", + code: str = "", + locations: list[dict] | None = None, ) -> DuplicateMatch | None: """Best near-duplicate of (title, body) within the same owner + project + - kind, or None. Title match first (cheap, exact), then semantic when the body - is long enough to be meaningful. Never raises — embedder failure degrades to - title-only (callers should still be able to create).""" + kind, or None. Title match first (cheap, exact), then — for snippets — the + structural signals, then semantic when the body is long enough to be + meaningful. Never raises — embedder failure degrades to title-only (callers + should still be able to create). + + `code` and `locations` are the snippet's structured fields. They are ignored + for every other kind, and passing them is what lets the gate compare + ARTEFACTS rather than descriptions of artefacts (#2518). + """ norm = " ".join((title or "").split()).lower() # --- Signal 1: normalized-title exact match (same scope) --- @@ -118,7 +246,17 @@ async def find_duplicate_note( logger.debug("dedup title check skipped — query failed", exc_info=True) return None - # --- Signal 2: semantic similarity (only with a substantial body) --- + # --- Signal 2: structural identity (snippets only) --- + # Ahead of the semantic arm because it is exact: when it fires there is + # nothing to weigh, and its verdict is the one worth showing. + if note_type == SNIPPET_NOTE_TYPE: + structural = await _find_snippet_by_structure( + user_id, code, locations, project_id + ) + if structural is not None: + return structural + + # --- Signal 3: semantic similarity (only with a substantial body) --- if body and len(body.strip()) >= _MIN_BODY_FOR_SEMANTIC: query = f"{title}\n{body}".strip() # Scope the semantic check the same way as the title check: a record in @@ -129,7 +267,9 @@ async def find_duplicate_note( hits = await embeddings_svc.semantic_search_notes( user_id, query, project_id=project_id, is_task=is_task, orphan_only=(project_id is None), - limit=3, threshold=_SEMANTIC_THRESHOLD, + limit=3, + threshold=(_SNIPPET_SEMANTIC_THRESHOLD + if note_type == SNIPPET_NOTE_TYPE else _SEMANTIC_THRESHOLD), # Owner-only, deliberately: this gate BLOCKS a create and tells the # caller to update the match instead. Matching someone else's record # would refuse their write and point them at something they may not @@ -222,6 +362,52 @@ def group_pairs(pairs: list[tuple[int, int, float]]) -> list[list[int]]: key=lambda g: (-len(g), g[0])) +def _symbols(data: dict | None) -> set[str]: + """Every symbol a snippet claims, from its indexed location mirror.""" + return { + (loc.get("symbol") or "").strip() + for loc in (data or {}).get("locations") or [] + if (loc.get("symbol") or "").strip() + } + + +def _drop_sibling_pairs( + pairs: list[tuple[int, int, float]], records: dict[int, dict] +) -> list[tuple[int, int, float]]: + """Remove pairs that are VARIANTS of one thing rather than copies of it. + + Two snippets that both name a symbol, name DIFFERENT symbols, and hold + different code are two artefacts. The author asserted that by naming them + apart, and merging them would destroy a distinction someone made on purpose. + + Without this, a design system's button family reports as a single merge set: + eight recipes, every direct pair over the floor, top score 0.92 (#2518). + They resemble each other because variants of one component are SUPPOSED to — + same selector prefix, same token families, deliberately parallel prose. The + similarity is read correctly; it just does not mean "duplicate". + + THE COST, stated plainly: a helper genuinely recorded twice under two names + — `debounce` and `useDebouncedRef` — is no longer reported. That is real + recall lost. It is the better trade because the report is a merge PROPOSAL: + a missed pair costs a duplicate nobody was going to notice anyway, while a + wrong set invites an operator to collapse a component family in one click. + Same-symbol and no-symbol duplicates, which is how re-recording usually + looks, still report. + """ + kept = [] + for left, right, score in pairs: + left_data, right_data = records.get(left) or {}, records.get(right) or {} + left_syms, right_syms = _symbols(left_data), _symbols(right_data) + shas = (left_data.get("code_sha"), right_data.get("code_sha")) + identical_code = shas[0] is not None and shas[0] == shas[1] + # Both named, no name in common, and the code differs → siblings. + if (left_syms and right_syms and not (left_syms & right_syms) + and not identical_code): + continue + kept.append((left, right, score)) + return kept + + async def find_duplicate_snippets( user_id: int, *, threshold: float | None = None, limit: int = _MAX_DUPLICATE_PAIRS ) -> dict: @@ -280,21 +466,32 @@ async def find_duplicate_snippets( if not pairs: return {"groups": [], "pairs": [], "threshold": floor} - best: dict[tuple[int, int], float] = {(a, b): s for a, b, s in pairs} - grouped = group_pairs(pairs) - - # Titles for presentation. One fetch for every id in the report. - ids = sorted({n for g in grouped for n in g}) + # Titles + the structural fields, for presentation AND for the sibling + # filter below. One fetch covers every id the scan proposed. + scanned = sorted({n for pair in pairs for n in pair[:2]}) titles: dict[int, str] = {} + records: dict[int, dict] = {} try: async with async_session() as session: rows = (await session.execute( - select(Note.id, Note.title).where(Note.id.in_(ids)) + select(Note.id, Note.title, Note.data).where(Note.id.in_(scanned)) )).all() - titles = {int(i): t for i, t in rows} + titles = {int(i): t for i, t, _ in rows} + records = {int(i): (d or {}) for i, _, d in rows} except Exception: logger.debug("duplicate report titles unavailable", exc_info=True) + # Fails OPEN, and the direction matters: with `records` empty the filter + # below keeps every pair, so a lookup failure degrades to the unfiltered + # report rather than to an empty one. A report that silently returns + # nothing reads as "your corpus is clean", which is the wrong lie. + pairs = _drop_sibling_pairs(pairs, records) + if not pairs: + return {"groups": [], "pairs": [], "threshold": floor} + + best: dict[tuple[int, int], float] = {(a, b): s for a, b, s in pairs} + grouped = group_pairs(pairs) + groups = [] for members in grouped: scores = [ diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 5be773a..b4db170 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -96,6 +96,27 @@ def _normalize_locations(locations: list[dict] | None) -> list[dict]: return out +def resolve_locations( + repo: str = "", path: str = "", symbol: str = "", + locations: list[dict] | None = None, +) -> list[dict]: + """The location list a caller meant, from either calling convention. + + `locations` is the general form (one entry per call site); repo/path/symbol + are the single-location shorthand and apply only when `locations` was not + given — passing both is not a merge, it is the caller having decided. + + Extracted because compose_body, create_snippet and the dedup gate must all + read the shorthand the SAME way. They each had their own copy of the + `if locations is None` fallback, which is fine until one of them gains a + rule the others don't — and the gate (#2518) is the one where a disagreement + would mean comparing a location the record won't actually be stored with. + """ + if locations is None: + locations = [{"repo": repo, "path": path, "symbol": symbol}] + return _normalize_locations(locations) + + def _location_str(loc: dict) -> str: """`repo` · `path` · `symbol` — only the non-empty parts.""" parts = [(loc.get(k) or "").strip() for k in ("repo", "path", "symbol")] @@ -186,9 +207,7 @@ def compose_body( a back-compat shorthand for one location and are used only when ``locations`` is not given. """ - if locations is None: - locations = [{"repo": repo, "path": path, "symbol": symbol}] - locs = _normalize_locations(locations) + locs = resolve_locations(repo, path, symbol, locations) header: list[str] = [] if (when_to_use or "").strip(): @@ -601,8 +620,7 @@ async def create_snippet( """Create a snippet note (embedded on create for immediate recall). Returns the created Note. Pass ``locations`` for the multi-location case; the single ``repo``/``path``/``symbol`` are the one-location shorthand.""" - if locations is None: - locations = [{"repo": repo, "path": path, "symbol": symbol}] + locations = resolve_locations(repo, path, symbol, locations) note = await notes_svc.create_note( user_id, title=compose_title(name, when_to_use), diff --git a/tests/test_services_dedup.py b/tests/test_services_dedup.py index 17490c1..a0a9e30 100644 --- a/tests/test_services_dedup.py +++ b/tests/test_services_dedup.py @@ -110,3 +110,175 @@ def test_duplicate_response_shape(): assert r["match"] == "title" assert "force=true" in r["message"] assert "update_task" in r["message"] + + +# --- snippet structural identity (#2518) ------------------------------------- +# +# The gate used to compare a snippet's rendered DOCUMENT, which is mostly prose +# about the code. Measured on the button corpus, that failed in both directions +# at once: two deliberately-parallel variants were refused at 0.92, while a +# verbatim re-record of one snippet under a different name scored below 0.90 and +# was created. These tests pin the structural signals that replaced it. + + +def _session_sequence(results): + """A mocked async_session() whose successive execute() calls yield `results`. + + The single-result helper above can't express this: the structural check runs + a location query and then a code query, and the whole point is that they + answer differently. + """ + s = AsyncMock() + s.__aenter__ = AsyncMock(return_value=s) + s.__aexit__ = AsyncMock(return_value=False) + wrapped = [] + for note in results: + r = MagicMock() + r.scalars.return_value.first.return_value = note + wrapped.append(r) + s.execute = AsyncMock(side_effect=wrapped) + return s + + +@pytest.mark.asyncio +async def test_same_location_is_a_duplicate_however_it_is_described(): + """The measured false NEGATIVE: identical code at an identical + repo·path·symbol was created because the prose around it differed.""" + existing = _fake_note(id=30, title=".btn-primary — a page's main action", + note_type="snippet") + sem = AsyncMock() + with patch("scribe.services.dedup.async_session", + return_value=_session_sequence([None, existing])), \ + patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", sem): + dup = await find_duplicate_note( + 7, "primaryButton — something else entirely", body="x" * 400, + project_id=2, is_task=False, note_type="snippet", + code=".btn-primary { color: red; }", + locations=[{"repo": "Scribe", "path": "a/b.css", "symbol": ".btn-primary"}], + ) + assert dup is not None + assert dup.reason == "location" + assert dup.similarity == 1.0 + # Structural identity is certain, so it must not be diluted by asking the + # embedder for a second opinion. + sem.assert_not_called() + + +@pytest.mark.asyncio +async def test_identical_code_is_a_duplicate_at_a_different_location(): + existing = _fake_note(id=31, title="group_pairs", note_type="snippet") + with patch("scribe.services.dedup.async_session", + return_value=_session_sequence([None, None, existing])), \ + patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", + AsyncMock(return_value=[])): + dup = await find_duplicate_note( + 7, "unionFind", body="x" * 400, project_id=2, is_task=False, + note_type="snippet", code="def f():\n return 1", + locations=[{"repo": "Scribe", "path": "z.py", "symbol": "f"}], + ) + assert dup is not None + assert dup.reason == "code" + + +@pytest.mark.asyncio +async def test_a_location_without_a_symbol_is_not_an_identity(): + """A path alone is a DIRECTORY of artefacts. Matching on it would refuse + every second snippet recorded from one file — which is exactly the corpus + the button recipes form.""" + session = _session_sequence([None]) + sem = AsyncMock(return_value=[]) + with patch("scribe.services.dedup.async_session", return_value=session), \ + patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", sem): + dup = await find_duplicate_note( + 7, "Some recipe", body="x" * 400, project_id=2, is_task=False, + note_type="snippet", code="", + locations=[{"repo": "Scribe", "path": "a/b.css", "symbol": ""}], + ) + assert dup is None + # Exactly one query — the title check. With no symbol and no code there is + # nothing to match structurally, and the sequence above would raise + # StopIteration if a second query were issued. + assert session.execute.await_count == 1 + + +@pytest.mark.asyncio +async def test_snippets_use_the_raised_semantic_threshold(): + """Variants of one component legitimately reach 0.92. The semantic arm has + to sit above that band or it refuses the corpus it exists to protect.""" + from scribe.services.dedup import ( + _SEMANTIC_THRESHOLD, + _SNIPPET_SEMANTIC_THRESHOLD, + ) + sem = AsyncMock(return_value=[]) + with patch("scribe.services.dedup.async_session", + return_value=_session_sequence([None, None, None])), \ + patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", sem): + await find_duplicate_note( + 7, "A recipe", body="x" * 400, project_id=2, is_task=False, + note_type="snippet", code="x", + locations=[{"repo": "R", "path": "p", "symbol": "s"}], + ) + assert sem.await_args.kwargs["threshold"] == _SNIPPET_SEMANTIC_THRESHOLD + assert _SNIPPET_SEMANTIC_THRESHOLD > 0.92, ( + "the observed sibling band tops out at 0.92 (.btn-danger vs " + ".btn-danger-outline); a threshold at or below it blocks legitimate " + "variants again" + ) + assert _SNIPPET_SEMANTIC_THRESHOLD > _SEMANTIC_THRESHOLD + + +def test_sibling_variants_are_not_reported_as_merge_candidates(): + """The measured false POSITIVE: eight button recipes, every direct pair over + the floor, proposed as ONE merge set.""" + from scribe.services.dedup import _drop_sibling_pairs + + records = { + 1: {"locations": [{"repo": "S", "path": "c.css", "symbol": ".btn-primary"}], + "code_sha": "aaa"}, + 2: {"locations": [{"repo": "S", "path": "c.css", "symbol": ".btn-secondary"}], + "code_sha": "bbb"}, + } + assert _drop_sibling_pairs([(1, 2, 0.87)], records) == [] + + +def test_identical_code_still_reports_even_with_different_symbols(): + """The filter keys on "the author named these apart", but a shared code + fingerprint overrides that — the same code under two names IS the + copy-paste the report exists to surface.""" + from scribe.services.dedup import _drop_sibling_pairs + + records = { + 1: {"locations": [{"repo": "S", "path": "a.py", "symbol": "debounce"}], + "code_sha": "same"}, + 2: {"locations": [{"repo": "S", "path": "b.py", "symbol": "useDebounced"}], + "code_sha": "same"}, + } + assert _drop_sibling_pairs([(1, 2, 0.9)], records) == [(1, 2, 0.9)] + + +def test_unnamed_snippets_still_report(): + """A snippet with no recorded symbol made no identity claim, so the filter + must not protect it — re-recording without a location is a common way to + duplicate.""" + from scribe.services.dedup import _drop_sibling_pairs + + records = {1: {"code_sha": "aaa"}, 2: {"code_sha": "bbb"}} + assert _drop_sibling_pairs([(1, 2, 0.9)], records) == [(1, 2, 0.9)] + + +def test_duplicate_response_names_what_matched_for_structural_hits(): + """A structural hit is certain, so the message must not hedge with + "similar" — and it points at merge, which is what two records of one + artefact actually need.""" + r = duplicate_response( + DuplicateMatch(id=9, title=".btn-primary", similarity=1.0, reason="location"), + "snippet", + ) + assert "repo · path · symbol" in r["message"] + assert "merge_snippets" in r["message"] + assert "similar" not in r["message"] + + r = duplicate_response( + DuplicateMatch(id=9, title="x", similarity=1.0, reason="code"), "snippet", + ) + assert "identical code" in r["message"]