fix(dedup): compare the artefact, not the prose describing it
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / integration (push) Successful in 19s
CI & Build / TypeScript typecheck (push) Successful in 32s
CI & Build / Python tests (push) Successful in 48s
CI & Build / Build & push image (push) Successful in 25s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / integration (push) Successful in 19s
CI & Build / TypeScript typecheck (push) Successful in 32s
CI & Build / Python tests (push) Successful in 48s
CI & Build / Build & push image (push) Successful in 25s
The snippet gate was reading the wrong field, and #2464's UI recipes made it measurable in both directions at once: .btn-danger vs .btn-danger-outline 0.92 siblings, BLOCKED .btn-primary re-recorded verbatim under a different name <0.90 a literal copy, ALLOWED The second is what settles it. Identical code at an identical repo·path·symbol sailed through because the description differed, while two deliberately parallel variants were refused because theirs did not. A snippet's embedded document is mostly prose ABOUT the code, so no threshold fixes this: lowering it blocks more siblings, raising it admits more copies. So structure decides. Two exact signals, both index-served off the notes.data mirror that already exists, no migration and no backfill: location the same named thing in the same file. Requires BOTH path and symbol — a path alone is a directory of artefacts, and matching on it would refuse every second recipe from one stylesheet. code byte-identical code anywhere, via the same fingerprint the drift check uses. The semantic arm survives as a backstop for a genuine reword that shares neither, raised to 0.96 so it sits above the 0.92 band where real variants live. Structural hits say what they matched instead of hedging with "similar", and point at merge rather than update — two records of one artefact is what merge exists to fold back together. find_duplicate_snippets gets the same correction: pairs where both snippets name a symbol, name DIFFERENT symbols, and hold different code are variants, not copies. Without it a design system's button family reports as one merge set — eight recipes, every direct pair over the floor, top score 0.92, one click from collapsing a component family. The cost is real and stated in the code: a helper recorded twice under two names no longer reports. That trade favours the report being usable, and same-symbol and unnamed duplicates — how re-recording usually looks — still surface. The filter fails open, so a lookup failure degrades to the old unfiltered report rather than to a reassuring empty one. resolve_locations extracted: compose_body, create_snippet and now the gate each had their own copy of the repo/path/symbol shorthand fallback, and the gate is the one where a disagreement would mean matching a location the record won't be stored with. Applied to both create surfaces (#33) — the web UI must not be the way to record what the agent was stopped from writing. Refs #2518, #2464
This commit is contained in:
@@ -127,12 +127,19 @@ async def create_snippet(
|
|||||||
force: Bypass the near-duplicate gate (see below).
|
force: Bypass the near-duplicate gate (see below).
|
||||||
|
|
||||||
Returns the created snippet (including a parsed `snippet` field), OR — when a
|
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
|
"existing_id": ..., "message": ...} and nothing is created. When that happens
|
||||||
and it really is the same reusable thing found in another place, prefer
|
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
|
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),
|
into ONE canonical record (which then carries every call site as a location),
|
||||||
rather than forcing a second copy with force=true.
|
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():
|
if not (name or "").strip() or not (code or "").strip():
|
||||||
raise ValueError("create_snippet requires a non-empty name and code")
|
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(
|
dup = await dedup_svc.find_duplicate_note(
|
||||||
uid, title, body, project_id=project_id or None,
|
uid, title, body, project_id=project_id or None,
|
||||||
is_task=False, note_type=snippets_svc.SNIPPET_NOTE_TYPE,
|
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:
|
if dup is not None:
|
||||||
return dedup_svc.duplicate_response(dup, "snippet")
|
return dedup_svc.duplicate_response(dup, "snippet")
|
||||||
|
|||||||
@@ -112,6 +112,15 @@ async def create_snippet_route():
|
|||||||
project_id=project_id,
|
project_id=project_id,
|
||||||
is_task=False,
|
is_task=False,
|
||||||
note_type=snippets_svc.SNIPPET_NOTE_TYPE,
|
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:
|
if dup is not None:
|
||||||
return jsonify(dedup_svc.duplicate_response(dup, "snippet")), 409
|
return jsonify(dedup_svc.duplicate_response(dup, "snippet")), 409
|
||||||
|
|||||||
+217
-20
@@ -48,6 +48,27 @@ _MIN_BODY_FOR_SEMANTIC = 200
|
|||||||
# noise). Matches the 0.90 the pre-pivot dedup settled on.
|
# noise). Matches the 0.90 the pre-pivot dedup settled on.
|
||||||
_SEMANTIC_THRESHOLD = 0.90
|
_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
|
@dataclass
|
||||||
class DuplicateMatch:
|
class DuplicateMatch:
|
||||||
@@ -58,25 +79,124 @@ class DuplicateMatch:
|
|||||||
reason: str # "title" | "semantic"
|
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:
|
def duplicate_response(dup: "DuplicateMatch", kind: str) -> dict:
|
||||||
"""Standard 'blocked — update instead' payload returned by a create tool
|
"""Standard 'blocked — update instead' payload returned by a create tool
|
||||||
when the gate finds a near-duplicate. `kind` is 'note' or 'task' (drives the
|
when the gate finds a near-duplicate. `kind` is 'note', 'task' or 'snippet'
|
||||||
update_<kind> hint)."""
|
(drives the update_<kind> 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 {
|
return {
|
||||||
"duplicate": True,
|
"duplicate": True,
|
||||||
"existing_id": dup.id,
|
"existing_id": dup.id,
|
||||||
"existing_title": dup.title,
|
"existing_title": dup.title,
|
||||||
"similarity": dup.similarity,
|
"similarity": dup.similarity,
|
||||||
"match": dup.reason,
|
"match": dup.reason,
|
||||||
"message": (
|
"message": 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."
|
|
||||||
),
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
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(
|
async def find_duplicate_note(
|
||||||
user_id: int,
|
user_id: int,
|
||||||
title: str,
|
title: str,
|
||||||
@@ -84,11 +204,19 @@ async def find_duplicate_note(
|
|||||||
project_id: int | None = None,
|
project_id: int | None = None,
|
||||||
is_task: bool | None = None,
|
is_task: bool | None = None,
|
||||||
note_type: str = "note",
|
note_type: str = "note",
|
||||||
|
code: str = "",
|
||||||
|
locations: list[dict] | None = None,
|
||||||
) -> DuplicateMatch | None:
|
) -> DuplicateMatch | None:
|
||||||
"""Best near-duplicate of (title, body) within the same owner + project +
|
"""Best near-duplicate of (title, body) within the same owner + project +
|
||||||
kind, or None. Title match first (cheap, exact), then semantic when the body
|
kind, or None. Title match first (cheap, exact), then — for snippets — the
|
||||||
is long enough to be meaningful. Never raises — embedder failure degrades to
|
structural signals, then semantic when the body is long enough to be
|
||||||
title-only (callers should still be able to create)."""
|
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()
|
norm = " ".join((title or "").split()).lower()
|
||||||
|
|
||||||
# --- Signal 1: normalized-title exact match (same scope) ---
|
# --- 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)
|
logger.debug("dedup title check skipped — query failed", exc_info=True)
|
||||||
return None
|
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:
|
if body and len(body.strip()) >= _MIN_BODY_FOR_SEMANTIC:
|
||||||
query = f"{title}\n{body}".strip()
|
query = f"{title}\n{body}".strip()
|
||||||
# Scope the semantic check the same way as the title check: a record in
|
# 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(
|
hits = await embeddings_svc.semantic_search_notes(
|
||||||
user_id, query, project_id=project_id, is_task=is_task,
|
user_id, query, project_id=project_id, is_task=is_task,
|
||||||
orphan_only=(project_id is None),
|
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
|
# Owner-only, deliberately: this gate BLOCKS a create and tells the
|
||||||
# caller to update the match instead. Matching someone else's record
|
# caller to update the match instead. Matching someone else's record
|
||||||
# would refuse their write and point them at something they may not
|
# 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]))
|
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(
|
async def find_duplicate_snippets(
|
||||||
user_id: int, *, threshold: float | None = None, limit: int = _MAX_DUPLICATE_PAIRS
|
user_id: int, *, threshold: float | None = None, limit: int = _MAX_DUPLICATE_PAIRS
|
||||||
) -> dict:
|
) -> dict:
|
||||||
@@ -280,21 +466,32 @@ async def find_duplicate_snippets(
|
|||||||
if not pairs:
|
if not pairs:
|
||||||
return {"groups": [], "pairs": [], "threshold": floor}
|
return {"groups": [], "pairs": [], "threshold": floor}
|
||||||
|
|
||||||
best: dict[tuple[int, int], float] = {(a, b): s for a, b, s in pairs}
|
# Titles + the structural fields, for presentation AND for the sibling
|
||||||
grouped = group_pairs(pairs)
|
# filter below. One fetch covers every id the scan proposed.
|
||||||
|
scanned = sorted({n for pair in pairs for n in pair[:2]})
|
||||||
# Titles for presentation. One fetch for every id in the report.
|
|
||||||
ids = sorted({n for g in grouped for n in g})
|
|
||||||
titles: dict[int, str] = {}
|
titles: dict[int, str] = {}
|
||||||
|
records: dict[int, dict] = {}
|
||||||
try:
|
try:
|
||||||
async with async_session() as session:
|
async with async_session() as session:
|
||||||
rows = (await session.execute(
|
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()
|
)).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:
|
except Exception:
|
||||||
logger.debug("duplicate report titles unavailable", exc_info=True)
|
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 = []
|
groups = []
|
||||||
for members in grouped:
|
for members in grouped:
|
||||||
scores = [
|
scores = [
|
||||||
|
|||||||
@@ -96,6 +96,27 @@ def _normalize_locations(locations: list[dict] | None) -> list[dict]:
|
|||||||
return out
|
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:
|
def _location_str(loc: dict) -> str:
|
||||||
"""`repo` · `path` · `symbol` — only the non-empty parts."""
|
"""`repo` · `path` · `symbol` — only the non-empty parts."""
|
||||||
parts = [(loc.get(k) or "").strip() for k in ("repo", "path", "symbol")]
|
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``
|
a back-compat shorthand for one location and are used only when ``locations``
|
||||||
is not given.
|
is not given.
|
||||||
"""
|
"""
|
||||||
if locations is None:
|
locs = resolve_locations(repo, path, symbol, locations)
|
||||||
locations = [{"repo": repo, "path": path, "symbol": symbol}]
|
|
||||||
locs = _normalize_locations(locations)
|
|
||||||
|
|
||||||
header: list[str] = []
|
header: list[str] = []
|
||||||
if (when_to_use or "").strip():
|
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
|
"""Create a snippet note (embedded on create for immediate recall). Returns
|
||||||
the created Note. Pass ``locations`` for the multi-location case; the single
|
the created Note. Pass ``locations`` for the multi-location case; the single
|
||||||
``repo``/``path``/``symbol`` are the one-location shorthand."""
|
``repo``/``path``/``symbol`` are the one-location shorthand."""
|
||||||
if locations is None:
|
locations = resolve_locations(repo, path, symbol, locations)
|
||||||
locations = [{"repo": repo, "path": path, "symbol": symbol}]
|
|
||||||
note = await notes_svc.create_note(
|
note = await notes_svc.create_note(
|
||||||
user_id,
|
user_id,
|
||||||
title=compose_title(name, when_to_use),
|
title=compose_title(name, when_to_use),
|
||||||
|
|||||||
@@ -110,3 +110,175 @@ def test_duplicate_response_shape():
|
|||||||
assert r["match"] == "title"
|
assert r["match"] == "title"
|
||||||
assert "force=true" in r["message"]
|
assert "force=true" in r["message"]
|
||||||
assert "update_task" 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"]
|
||||||
|
|||||||
Reference in New Issue
Block a user