diff --git a/frontend/src/views/ProjectView.vue b/frontend/src/views/ProjectView.vue index 4dd1bd0..f3047ec 100644 --- a/frontend/src/views/ProjectView.vue +++ b/frontend/src/views/ProjectView.vue @@ -956,13 +956,25 @@ async function confirmDelete() { .stat-notes { background: color-mix(in srgb, var(--color-primary) 8%, transparent); color: var(--color-primary); border-color: color-mix(in srgb, var(--color-primary) 22%, transparent); } /* ── Two-column body ─────────────────────────────────────────── */ +/* `minmax(0, 1fr)`, not `1fr`. A bare `1fr` track has an AUTO minimum, so it + cannot shrink below its content — one wide descendant anywhere in the + content column widens the whole column past the grid, and everything inside + it then overflows the page and gets cut by `.project-view`'s + `overflow-x: clip`. + This is the same property the header nav relies on and wants (neither side + squeezed under its content); here it is exactly wrong, because the column + holds a kanban whose own tracks push outward. `min-width: 0` on the item is + the twin half — a grid item's default `min-width: auto` refuses to shrink + even when its track will. */ .project-body { display: grid; - grid-template-columns: 248px 1fr; + grid-template-columns: 248px minmax(0, 1fr); gap: 1.25rem; align-items: start; } +.content-area { min-width: 0; } + /* ── Edit panel ──────────────────────────────────────────────── */ .edit-panel { background: var(--color-bg-card); @@ -1190,7 +1202,10 @@ async function confirmDelete() { /* ── Kanban ──────────────────────────────────────────────────── */ .kanban { display: grid; - grid-template-columns: repeat(3, 1fr); + /* Same reason as .project-body: three auto-minimum tracks add up to more + than the column when a card title or a column header won't compress, and + the excess pushes the whole milestone card wider than the page. */ + grid-template-columns: repeat(3, minmax(0, 1fr)); gap: 0.75rem; align-items: start; padding: 0.75rem; diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 98685c7..18b1602 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "0.1.23", + "version": "0.1.24", "author": { "name": "Bryan Van Deusen" }, "mcpServers": { "scribe": { diff --git a/plugin/hooks/scribe_static_context.md b/plugin/hooks/scribe_static_context.md index dd5a313..a92049b 100644 --- a/plugin/hooks/scribe_static_context.md +++ b/plugin/hooks/scribe_static_context.md @@ -51,5 +51,17 @@ for the operator's work, and as your own working memory across sessions. `/compact` (name what you logged). You can't run it yourself — surface the recommendation and let them decide. Suggest it at seams, not every turn. +**If two Scribe instruction surfaces disagree** — this file, the MCP server's +tool instructions, the `using-scribe` skill — **follow the one that assumes +least about its own delivery.** This file is the floor: it ships with the +plugin and needs no API key and no network, so it still applies in exactly the +session where the others never arrived. The others may elaborate on what is +written here; they must not contradict it. Weigh a disagreement by which way it +fails, not by which surface said more: doing something a push would also have +covered costs one redundant call, while skipping it because you expected a push +that never came means working without the operator's rules and not knowing. +A contradiction between surfaces is a defect in the product — say so, so it +gets recorded and fixed rather than silently arbitrated again next session. + If the Scribe tools are unavailable, say so rather than silently falling back to local notes. diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 7a7f55b..9bec58e 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -174,19 +174,30 @@ written as a rule. A repeatable procedure is a PROCESS. Reusable code is a SNIPPET. Reach for a rule when the thing genuinely is a standing instruction about how to work, and nothing else can hold it. +Rules are pull-only: CALL list_always_on_rules() YOURSELF at the start of a +session, before substantive work. Do not wait to be handed them. The plugin's +SessionStart hook also pushes the always-on rules + active-project context, but +that is a delivery optimisation, not the bridge — it can be absent (instance +unreachable, key not configured, or the hook itself silently inert, which is +what #2198 was) and its absence does not announce itself. When the push did +arrive, the pull costs one redundant call; when it didn't, skipping the pull +means working without the operator's binding rules and having no signal that +you are. Those two costs are not comparable, so pull. + One thing NOT to do: don't bridge Scribe into a session by writing to the -host's native memory. Rules are pull-only, so a fresh session won't reach for -them unless its always-loaded context says to — but the bridge for that is the -Scribe plugin's SessionStart hook, which pushes the always-on rules + -active-project context into each session directly. So do NOT create or refresh -a "rules live in Scribe" pointer in CLAUDE.md / AGENTS.md / ~/.claude memory, -and do NOT keep rules, recall, or plans in those stores in parallel with Scribe -— Scribe holds the single copy. Native auto-memory stays for facts about the -user; CLAUDE.md for codebase onboarding. Never make Scribe's correctness depend -on the operator disabling a native function (e.g. autoMemoryEnabled): the -plugin must work with auto-memory at its default. If the plugin is ever removed -the session loses this push and rebuilds context over time — an acceptable cost, -and far better than a silent settings change the operator may not know about. +host's native memory. That explicit pull IS the bridge — it is written into +every instruction surface the session already loads, so it needs no pointer on +disk. So do NOT create or refresh a "rules live in Scribe" pointer in +CLAUDE.md / AGENTS.md / ~/.claude memory, and do NOT keep rules, recall, or +plans in those stores in parallel with Scribe — Scribe holds the single copy, +and a pointer Scribe cannot update is one that goes stale without anyone +noticing. Native auto-memory stays for facts about the user; CLAUDE.md for +codebase onboarding. Never make Scribe's correctness depend on the operator +disabling a native function (e.g. autoMemoryEnabled): the plugin must work with +auto-memory at its default. If the plugin is ever removed the session loses both +the push and these instructions, and rebuilds context over time — an acceptable +cost, and far better than a silent settings change the operator may not know +about. When you are working on a specific project, call enter_project(project_id) ONCE at session start (or whenever the active project changes). It returns the @@ -225,7 +236,11 @@ Scribe stores reusable Processes — saved prompts/workflows (note_type X process" or otherwise references a saved process, call list_processes() / get_process(name) and follow the returned prompt verbatim, including any "clarify first" steps it contains. Author a new one with create_process(title, -body); edit with update_process. +body); edit with update_process; retire one with delete_process (recoverable — +it goes to the trash like anything else). A near-duplicate is refused at create +time, because every Process becomes a skill file that auto-surfaces on the +operator's machine: two near-identical procedures don't merely bloat the record, +they compete to be followed. Scribe also stores Snippets — reusable functions/components recorded once for recall (note_type "snippet"): a name, language, signature, canonical location @@ -267,6 +282,20 @@ operator. "Works for one user" is not done. # Tools a read-only API key may call. Anything not listed is treated as a # write for read keys (default-deny), so a newly-added tool is locked down # until explicitly classified here. +# +# The list stays EXPLICIT rather than being derived from the name. A read key is +# what you hand to something you don't fully trust — a dashboard, a CI job, a +# shared integration — and a boundary inferred from a naming convention grants +# access to whatever a future author happens to call `get_*`. Enumerating it is +# the point; staleness is the cost, and test_mcp_auth covers that (a read-shaped +# tool must appear here or in _DELIBERATELY_WRITE_SCOPED below, so adding one +# forces a decision instead of silently denying it). +# +# Membership means "reads the operator's data and mutates none of it". Several +# getters record a retrieval event via record_pulled; that is telemetry about +# the read itself, not a change to what was read, and it must keep working for a +# read key or the corpus's surfaced:pulled ratio silently under-counts whichever +# consumers hold one. _READ_ONLY_TOOLS = frozenset({ "get_note", "get_project", "get_rule", "get_rulebook", "get_task", "get_milestone", "get_recent", "enter_project", @@ -277,8 +306,30 @@ _READ_ONLY_TOOLS = frozenset({ # Reports on the snippet corpus. Reads only — the merge it suggests is a # separate, explicitly-called write. "find_duplicate_snippets", + # Snippets and processes are notes with a kind. A key that may read a note + # but not a snippet inverts the sensitivity ordering: it exposes the + # free-text records and withholds the structured ones (#2496). + "get_snippet", "list_snippets", + "get_process", "list_processes", + # Design systems: read, resolve (inheritance + mode), render, and compare + # against recorded snippets. All four compute from stored records and write + # nothing — the drift report is a report, and applying it is a separate + # explicit call. + "get_design_system", "list_design_systems", "resolve_design_system", + "get_design_system_stylesheet", "list_design_tokens", + "check_snippets_against_design_system", "list_starter_role_groups", + # Which repos map to which project. Read-only by nature; bind_repo / + # unbind_repo are the writes. + "list_repo_bindings", }) +# Read-SHAPED tools that must NOT be reachable with a read key — a getter that +# creates on miss, a list that has a side effect. Empty today, and deliberately +# kept as a declared escape hatch rather than left implicit: without it, the +# completeness test would push a future `get_or_create_*` into the allow-list +# above, which is exactly the wrong way to make a test pass. +_DELIBERATELY_WRITE_SCOPED: frozenset[str] = frozenset() + async def _buffer_request_body(receive): """Drain the ASGI request body and return (body_bytes, replay_receive). diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index c54e00e..a9b1abb 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/src/scribe/mcp/tools/processes.py @@ -8,8 +8,11 @@ from __future__ import annotations from scribe.mcp._context import current_user_id from scribe.services import access as access_svc +from scribe.services import dedup as dedup_svc from scribe.services import knowledge as knowledge_svc from scribe.services import notes as notes_svc +from scribe.services import trash as trash_svc +from scribe.services.note_usage import record_pulled async def list_processes(q: str = "", tag: str = "", limit: int = 50) -> dict: @@ -41,17 +44,38 @@ async def list_processes(q: str = "", tag: str = "", limit: int = 50) -> dict: return {"processes": procs, "total": total} -async def create_process(title: str, body: str, tags: list[str] | None = None) -> dict: +async def create_process( + title: str, body: str, tags: list[str] | None = None, force: bool = False, +) -> dict: """Create a stored process (a reusable saved prompt). Args: title: Process name, e.g. "Drift Audit" (required). body: The full prompt to run later (markdown). Required. tags: Plain-string tags, no # prefix. + force: Bypass the near-duplicate gate. By default, if a title- or + meaning-similar process already exists, creation is BLOCKED and the + existing one's id is returned so you update it instead. Set true + only for a genuinely distinct procedure. + + Returns the created process, OR — when a near-duplicate is found and force + is false — {"duplicate": true, "existing_id": ..., "message": ...} (nothing + created). + + The gate matters more here than for other kinds: every process becomes a + skill file that auto-surfaces on the operator's machine, so two near-identical + procedures don't merely bloat the corpus — they compete to be followed, and + which one wins is decided by a slug. """ if not (title or "").strip() or not (body or "").strip(): raise ValueError("create_process requires a non-empty title and body") uid = current_user_id() + if not force: + dup = await dedup_svc.find_duplicate_note( + uid, title, body, is_task=False, note_type="process", + ) + if dup is not None: + return dedup_svc.duplicate_response(dup, "process") note = await notes_svc.create_note( uid, title=title.strip(), body=body, note_type="process", tags=tags, ) @@ -82,6 +106,12 @@ async def get_process(name_or_id: str) -> dict: if candidates: out["other_matches"] = candidates out.update(await access_svc.describe_provenance(uid, note)) + # A process is embedded like any other note, so auto-inject can surface one — + # and its menu header names THIS tool as the way to open that kind. Without + # this, the getter the product points at is the one getter that records + # nothing, and every process sits permanently at zero pulls looking like dead + # weight beside kinds that merely had a counter (#2476, the repeat of #2245). + record_pulled(user_id=uid, note_id=int(note.id), source="mcp_get_process") return out @@ -119,6 +149,44 @@ async def update_process(process_id: int, title: str = "", body: str = "", return out +async def delete_process(process_id: int) -> dict: + """Retire a stored process — it moves to the trash and is recoverable. + + Reach for this when a procedure is wrong, superseded, or was never worth + keeping. A stored process is installed as a skill file on the operator's + machine and auto-surfaces there, so a bad one is followed rather than merely + ignored — it costs more than a missing one. + + Deletion was always possible through `delete_note` (a process is a note, and + the trash is kind-agnostic), but nothing said so, and a kind whose own tools + offer create/read/update reads as one you cannot retire (#2250). + """ + uid = current_user_id() + loaded = await notes_svc.get_note_for_user(uid, process_id) + note = loaded[0] if loaded else None + # Check the KIND before deleting: this tool is reached for by name, and + # letting it trash an ordinary note because the id happened to resolve would + # be a destructive action taken on a mistyped argument. + if note is None or note.note_type != "process" or note.deleted_at is not None: + raise ValueError(f"process {process_id} not found") + batch = await trash_svc.delete(uid, "note", process_id) + if batch is None: + raise ValueError(f"process {process_id} not found") + return { + "deleted_batch_id": batch, + "message": ( + f"Process {process_id} moved to trash. Restore with restore('{batch}'). " + f"Its skill stub disappears on the operator's next process sync." + ), + } + + def register(mcp) -> None: - for fn in (list_processes, create_process, get_process, update_process): + for fn in ( + list_processes, + create_process, + get_process, + update_process, + delete_process, + ): mcp.tool(name=fn.__name__)(fn) 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/models/note_usage.py b/src/scribe/models/note_usage.py index cb3671c..85eb5d4 100644 --- a/src/scribe/models/note_usage.py +++ b/src/scribe/models/note_usage.py @@ -51,10 +51,23 @@ class NoteUsageEvent(Base): note_id: Mapped[int] = mapped_column(Integer, nullable=False) # 'surfaced' | 'pulled' event: Mapped[str] = mapped_column(Text, nullable=False) - # Which surface produced it: 'auto_inject' | 'write_path_place' | - # 'write_path_semantic' | 'mcp_get_snippet' | 'mcp_get_note' | 'rest_note'. - # Kept granular so the place arm and the semantic arm can be compared — - # that comparison is the whole reason the place arm needed logging at all. + # Which surface produced it. Kept granular so the place arm and the + # semantic arm can be compared — that comparison is the whole reason the + # place arm needed logging at all. + # + # A CONVENTION, not a fixed vocabulary: `mcp_` for an agent call, + # `rest_` for a human opening a detail view, and a bare name for a + # hook or background surface ('auto_inject', 'write_path_place', + # 'write_path_semantic'). This comment deliberately no longer lists the + # members — the previous list had gone stale, naming 'rest_note' that + # nothing wrote while omitting sources that existed, and a half-true + # enumeration reads as authoritative in exactly the way that misleads + # (#2476). `grep -rn record_pulled\\\|record_surfaced src/` is the + # authoritative list, and unlike a comment it cannot drift. + # + # The mcp_/rest_ split is load-bearing. "Is this dead weight?" is served by + # any pull; "was that injected line useful?" is served by AGENT pulls only, + # so never aggregate across the prefix without saying why (#1038, #2085). source: Mapped[str] = mapped_column(Text, nullable=False) __table_args__ = ( diff --git a/src/scribe/routes/notes.py b/src/scribe/routes/notes.py index 3dcca5a..9e48ded 100644 --- a/src/scribe/routes/notes.py +++ b/src/scribe/routes/notes.py @@ -22,6 +22,7 @@ from scribe.services.notes import ( update_note, ) from scribe.services.note_drafts import upsert_draft, get_draft, delete_draft +from scribe.services.note_usage import record_pulled from scribe.services.note_versions import list_versions, get_version logger = logging.getLogger(__name__) @@ -178,6 +179,13 @@ async def get_note_route(note_id: int): note, permission = result data = note.to_dict() data["permission"] = permission + # Opening the detail view IS a pull — the operator chose to look. Tagged by + # SURFACE, not by the record's kind, matching rest_snippet: the kind is a + # join away, but which surface asked is not recoverable after the fact. + # Keeping rest_* apart from mcp_* is load-bearing, not tidiness — "was that + # injected line useful?" is answered by agent pulls alone, and a human + # clicking a link would inflate exactly the number #1038 and #2085 gate on. + record_pulled(user_id=uid, note_id=note_id, source="rest_note") return jsonify(data) 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/routes/tasks.py b/src/scribe/routes/tasks.py index d564b55..52662f9 100644 --- a/src/scribe/routes/tasks.py +++ b/src/scribe/routes/tasks.py @@ -13,6 +13,7 @@ from scribe.services.notes import ( list_notes, update_note, ) +from scribe.services.note_usage import record_pulled from scribe.services.planning import start_planning as svc_start_planning from scribe.services.recurrence import calculate_next_due, validate_recurrence_rule @@ -186,6 +187,9 @@ async def get_task_route(task_id: int): parent = await get_note_for_user(uid, task.parent_id) data["parent_title"] = parent[0].title if parent else None data["systems"] = [s.to_dict() for s in await systems_svc.list_record_systems(uid, task_id)] + # Opening the detail view IS a pull — see the note beside rest_note in + # routes/notes.py for why the rest_* and mcp_* prefixes stay separable. + record_pulled(user_id=uid, note_id=task_id, source="rest_task") return jsonify(data) 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_instruction_surfaces_agree.py b/tests/test_instruction_surfaces_agree.py new file mode 100644 index 0000000..87358c7 --- /dev/null +++ b/tests/test_instruction_surfaces_agree.py @@ -0,0 +1,104 @@ +"""The instruction surfaces must agree that the agent pulls the rules itself. + +WHY THIS EXISTS + +Rule #119 makes the instruction surfaces the SPECIFICATION for product +behaviour — there is no other place the "load the operator's rules" obligation +is written down, and no code path enforces it. So a surface that states it +differently isn't a documentation slip; it is the product behaving differently. + +That happened (#2497). `_INSTRUCTIONS` said the SessionStart hook "is the +bridge" for getting rules into a session, while the `using-scribe` skill said to +pull them yourself and treat any push as a bonus. An agent weighting the first +would reasonably skip the pull. + +#2198 is the case where that is wrong: every plugin hook was silently inert for +an extended period, and nothing announced it. An agent trusting the push would +have run with no binding rules and no signal — while those rules govern branch, +commit, push and other hard-to-reverse actions. + +The asymmetry is the whole argument, and it is what these tests pin: pulling +when a push also arrived costs one redundant call; not pulling when the push +never came costs the operator's rules entirely. + +WHAT THIS DOES NOT DO + +It cannot tell whether two surfaces contradict each other in prose generally — +that needs a reader. It pins the one instruction whose absence is known to be +load-bearing, and the specific shape the #2497 defect took: naming the push +without also stating the pull. +""" +from __future__ import annotations + +import pathlib + +ROOT = pathlib.Path(__file__).resolve().parents[1] + +# The pull instruction, however a surface phrases the surrounding prose. +PULL = "list_always_on_rules" + +# Surfaces a session loads before substantive work. Hand-written because +# "is this a session-start surface?" is an editorial fact, not a derivable one — +# but each entry is asserted to EXIST, so a move or rename fails loudly here +# instead of quietly dropping that surface from the check. +SESSION_START_SURFACES = ( + ROOT / "src" / "scribe" / "mcp" / "server.py", + ROOT / "plugin" / "hooks" / "scribe_static_context.md", + ROOT / "plugin" / "skills" / "using-scribe" / "SKILL.md", +) + + +def _all_surfaces() -> list[tuple[str, str]]: + """(label, text) for every file a SESSION loads as instructions. + + Deliberately not every markdown file under plugin/: `README.md` describes + the push channel to the operator installing the plugin, and telling a human + what the hook does is not the same act as telling an agent it need not pull. + The boundary is "does a session read this", which is skills (loaded by + description match), the hook-injected static context, and the MCP server's + own instructions. + """ + found = [(str(p.relative_to(ROOT)), p.read_text()) + for p in (ROOT / "plugin" / "skills").rglob("SKILL.md")] + found += [(str(p.relative_to(ROOT)), p.read_text()) + for p in (ROOT / "plugin" / "hooks").glob("*.md")] + server = ROOT / "src" / "scribe" / "mcp" / "server.py" + found.append((str(server.relative_to(ROOT)), server.read_text())) + return found + + +def test_every_session_start_surface_states_the_pull(): + missing = [] + for path in SESSION_START_SURFACES: + assert path.exists(), ( + f"{path.relative_to(ROOT)} is gone — it was one of the surfaces " + f"carrying the load-the-rules instruction. If it moved, update " + f"SESSION_START_SURFACES; if it was retired, check the instruction " + f"still lives somewhere a fresh session reads." + ) + if PULL not in path.read_text(): + missing.append(str(path.relative_to(ROOT))) + assert not missing, ( + f"these surfaces no longer tell the agent to call {PULL}(): {missing}. " + f"The rules are pull-only and the push is best-effort, so a surface " + f"that omits this leaves a session bound by nothing (#2198, #2497)." + ) + + +def test_no_surface_names_the_push_without_stating_the_pull(): + """The exact shape #2497 took. + + Mentioning the SessionStart hook is fine and often useful. Mentioning it + *instead of* the pull is the defect: it reads as "this is handled", and the + surface that says so is the one an agent has least reason to doubt. + """ + offenders = [ + label for label, text in _all_surfaces() + if "SessionStart" in text and PULL not in text + ] + assert not offenders, ( + f"these surfaces describe the SessionStart push but never state the " + f"explicit pull: {offenders}. The push is a delivery optimisation, not " + f"the bridge — it can be absent without saying so. Name it if it helps, " + f"but say to call {PULL}() regardless." + ) diff --git a/tests/test_mcp_auth.py b/tests/test_mcp_auth.py index 3a8203d..6c3a32c 100644 --- a/tests/test_mcp_auth.py +++ b/tests/test_mcp_auth.py @@ -92,3 +92,68 @@ def test_body_calls_write_tool_classifies_correctly(): json.dumps({"method": "tools/list"}).encode() ) is False assert _body_calls_write_tool(b"not json") is False + + +def test_every_read_shaped_tool_is_explicitly_classified(): + """A read-shaped tool must be classified, not left to default-deny. + + `_READ_ONLY_TOOLS` is hand-maintained, and default-deny means a getter + omitted from it fails CLOSED — safe, but silent. That is how a read key + ended up able to `get_note` and not `get_snippet`, both pure reads of the + same table, while design systems were unreachable entirely (#2496). The + `find_duplicate_snippets` entry was the tell: someone classified the report + and missed the getters beside it. + + This is the same shape as #2476 (record_pulled on three of four getters) — + a hand-written enumeration that missed the members added after it. The fix + there and here is the same: derive the CANDIDATES, keep the DECISION + explicit. Deriving the decision itself would be worse than a stale list — + it would make a security boundary follow a naming convention, so any future + `get_*` grants itself access. + + So: every tool whose name reads like a read must appear in one of the two + sets. Adding a getter then forces a choice at review time. + """ + import ast + import pathlib + + from scribe.mcp.server import _DELIBERATELY_WRITE_SCOPED, _READ_ONLY_TOOLS + + tools_dir = (pathlib.Path(__file__).resolve().parents[1] + / "src" / "scribe" / "mcp" / "tools") + read_shaped = { + node.name + for path in tools_dir.glob("*.py") if path.name != "__init__.py" + for node in ast.parse(path.read_text()).body + if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and node.name.startswith(("get_", "list_", "search", "resolve_", + "check_", "find_")) + } + assert read_shaped, "found no read-shaped tools — the tools package moved" + + unclassified = sorted(read_shaped - _READ_ONLY_TOOLS + - _DELIBERATELY_WRITE_SCOPED) + assert not unclassified, ( + f"these read-shaped tools are classified by neither set: {unclassified}. " + f"They currently fail closed for read-only keys, silently. Add each to " + f"_READ_ONLY_TOOLS if it mutates nothing, or to " + f"_DELIBERATELY_WRITE_SCOPED with a comment saying what it writes." + ) + + # The reverse: a name in either set that no longer exists is a rename or a + # deletion, and a stale grant is worth surfacing even though it grants + # access to nothing. `enter_project` is the one read tool without a read + # prefix, so it is checked against the full tool set, not `read_shaped`. + all_tools = { + node.name + for path in tools_dir.glob("*.py") if path.name != "__init__.py" + for node in ast.parse(path.read_text()).body + if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and not node.name.startswith("_") and node.name != "register" + } + phantom = sorted((_READ_ONLY_TOOLS | _DELIBERATELY_WRITE_SCOPED) - all_tools) + assert not phantom, ( + f"these names are classified but are not tools: {phantom}. They were " + f"renamed or removed — drop them, and check whatever replaced them got " + f"classified." + ) diff --git a/tests/test_mcp_tool_processes.py b/tests/test_mcp_tool_processes.py index 658ca9a..e71688c 100644 --- a/tests/test_mcp_tool_processes.py +++ b/tests/test_mcp_tool_processes.py @@ -37,7 +37,9 @@ async def test_create_process_requires_title_and_body(): @pytest.mark.asyncio async def test_create_process_sets_note_type(): created = _fake_note() - with patch("scribe.services.notes.create_note", + with patch("scribe.mcp.tools.processes.dedup_svc.find_duplicate_note", + AsyncMock(return_value=None)), \ + patch("scribe.services.notes.create_note", AsyncMock(return_value=created)) as mock_create: from scribe.mcp.tools.processes import create_process out = await create_process(title="Drift Audit", body="the prompt", tags=["audit"]) @@ -47,6 +49,39 @@ async def test_create_process_sets_note_type(): assert mock_create.await_args.kwargs["title"] == "Drift Audit" +@pytest.mark.asyncio +async def test_create_process_blocks_a_near_duplicate(): + """The gate matters more for processes than for other kinds: each one becomes + a skill file that auto-surfaces, so two near-identical procedures don't just + bloat the corpus — they compete to be followed (#2250).""" + from scribe.services.dedup import DuplicateMatch + + # The real dataclass, not a MagicMock: a mock answers every attribute, so it + # would pass whatever field names this test happened to guess and prove + # nothing about the payload the tool actually returns. + match = DuplicateMatch(id=42, title="Drift Audit", similarity=0.94, reason="semantic") + with patch("scribe.mcp.tools.processes.dedup_svc.find_duplicate_note", + AsyncMock(return_value=match)), \ + patch("scribe.services.notes.create_note", AsyncMock()) as mock_create: + from scribe.mcp.tools.processes import create_process + out = await create_process(title="Drift Audit", body="the prompt") + assert out["duplicate"] is True + assert out["existing_id"] == 42 + mock_create.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_create_process_force_bypasses_the_gate(): + created = _fake_note() + with patch("scribe.mcp.tools.processes.dedup_svc.find_duplicate_note", + AsyncMock()) as find_mock, \ + patch("scribe.services.notes.create_note", + AsyncMock(return_value=created)): + from scribe.mcp.tools.processes import create_process + await create_process(title="Drift Audit", body="the prompt", force=True) + find_mock.assert_not_awaited() + + @pytest.mark.asyncio async def test_get_process_returns_body_and_candidates(): note = _fake_note(id=7) @@ -117,7 +152,42 @@ async def test_update_process_refuses_a_read_only_share_with_the_reason(): mock_update.assert_not_awaited() -def test_register_attaches_four_tools(): +@pytest.mark.asyncio +async def test_delete_process_trashes_it_recoverably(): + proc = _fake_note(id=4) + proc.deleted_at = None + with patch("scribe.services.notes.get_note_for_user", + AsyncMock(return_value=(proc, "owner"))), \ + patch("scribe.mcp.tools.processes.trash_svc.delete", + AsyncMock(return_value="batch-1")) as mock_delete: + from scribe.mcp.tools.processes import delete_process + out = await delete_process(process_id=4) + assert out["deleted_batch_id"] == "batch-1" + # Through the trash, not a hard delete — restorable like every other kind. + assert mock_delete.await_args.args[1] == "note" + + +@pytest.mark.asyncio +async def test_delete_process_refuses_a_plain_note(): + """This tool is reached for by name. Letting it trash an ordinary note + because the id happened to resolve would be a destructive action taken on a + mistyped argument.""" + plain = _fake_note(id=3, note_type="note") + plain.deleted_at = None + with patch("scribe.services.notes.get_note_for_user", + AsyncMock(return_value=(plain, "owner"))), \ + patch("scribe.mcp.tools.processes.trash_svc.delete", AsyncMock()) as mock_delete: + from scribe.mcp.tools.processes import delete_process + with pytest.raises(ValueError): + await delete_process(process_id=3) + mock_delete.assert_not_awaited() + + +def test_register_attaches_every_tool_in_the_module(): + """Derived from the module rather than listed: a tool written but never + registered is invisible to an agent, and nothing else would notice.""" + import inspect + from scribe.mcp.tools import processes names: list[str] = [] @@ -129,6 +199,8 @@ def test_register_attaches_four_tools(): return deco processes.register(FakeMcp()) - assert set(names) == { - "list_processes", "create_process", "get_process", "update_process", + public = { + name for name, obj in vars(processes).items() + if inspect.iscoroutinefunction(obj) and not name.startswith("_") } + assert set(names) == public diff --git a/tests/test_pull_telemetry.py b/tests/test_pull_telemetry.py new file mode 100644 index 0000000..cffddd9 --- /dev/null +++ b/tests/test_pull_telemetry.py @@ -0,0 +1,203 @@ +"""Every surface that opens ONE note-backed record must record the pull. + +Covers both halves: the MCP getters an agent calls, and the REST detail views a +human opens. They are one ledger with two prefixes (`mcp_*` / `rest_*`), and a +gap on either side makes the same records look untouched. + +WHY THIS EXISTS + +`note_usage_events` answers "did anyone ever actually open this?" — the +surfaced:pulled ratio is what makes dead weight visible and prunable. A getter +that opens a record without recording it leaves that kind permanently at zero +pulls, so it looks like dead weight beside kinds that merely had a counter. + +That has now happened twice: + + #2245 `get_task` recorded nothing while auto-inject surfaced mostly tasks. + Fixed by adding the call to notes, tasks and snippets. + #2476 `get_process` recorded nothing — and the auto-inject menu header names + `get_process` as the way to open that kind. Processes were embedded + when #2245 was fixed; the fix enumerated the kinds someone thought of + rather than the kinds that exist. The same issue found the REST half: + `rest_snippet` was recorded, note and task detail were not, and the + model's own comment named a `'rest_note'` source nothing wrote. + +A missing call is the shape no behavioural test catches: it changes no return +value (#2278, shape 4). Source inspection is the only thing that sees it. + +WHAT MAKES THIS DERIVED RATHER THAN A LIST + +The getters are not enumerated here. They are discovered from the tool modules +by AST, and the ones that must record are identified by the loader they call — +so a `get_` added tomorrow is covered the moment it loads a note the +way every other getter does. + +The loader names ARE a list, and that is the residual weakness. The second test +pins them against a RENAME — the failure mode that would silently empty the +candidate set and let this pass while checking nothing. + +It does not discover NEW loaders, and an earlier draft that tried to failed for +the wrong reason: `create_note` and `update_note` also return a `Note`, so an +annotation scan finds writers, not readers. Distinguishing them needs more than +a type, so the honest position is a pinned list plus a non-empty assertion, +and this paragraph saying so. +""" +from __future__ import annotations + +import ast +import inspect +import pathlib +import pkgutil + +# Loaders that return ONE note-backed record in full. A getter calling any of +# these is opening a record, which is the act `pulled` describes. +# +# `list_notes` is deliberately absent: `get_milestone` calls it to list a +# milestone's steps, and that is a LIST — the milestone itself is not a note, +# and its steps are surfaced rather than opened. +SINGLE_NOTE_LOADERS = ( + "get_note_for_user", + "resolve_process", + "get_snippet", +) + +_SRC = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe" +TOOLS_DIR = _SRC / "mcp" / "tools" +ROUTES_DIR = _SRC / "routes" + + +def _getters(): + """(module name, function name, source) for every `get_*` MCP tool.""" + for mod in pkgutil.iter_modules([str(TOOLS_DIR)]): + path = TOOLS_DIR / f"{mod.name}.py" + source = path.read_text() + for node in ast.parse(source).body: + if isinstance(node, ast.AsyncFunctionDef) and node.name.startswith("get_"): + yield mod.name, node.name, ast.get_source_segment(source, node) or "" + + +def test_every_single_record_getter_records_a_pull(): + missing = [] + checked = [] + for module, name, body in _getters(): + if not any(loader in body for loader in SINGLE_NOTE_LOADERS): + continue + checked.append(f"{module}.{name}") + if "record_pulled" not in body: + missing.append(f"{module}.{name}") + + # If this ever drops to zero the test has stopped testing anything — a + # renamed loader would silently empty the candidate set and pass. + assert checked, "found no note-backed getters; the loader names must have moved" + assert not missing, ( + f"these getters open a record without recording the pull: {missing}. " + f"Add record_pulled(user_id=…, note_id=…, source='mcp_') before " + f"returning — see mcp/tools/notes.py:get_note." + ) + + +def _detail_routes(): + """(module, handler, expanded source) for every bare-id GET route. + + A route registered at exactly `/` for GET is the DETAIL view of one + record — that shape is what distinguishes it from a list, a sub-resource + (`//versions`) or a write. Nothing else about the handler has to be + guessed. + + The source is expanded one level through module-private helpers, because + `get_snippet_route` loads via `_load_snippet` rather than calling the loader + itself. Without the expansion the snippet route — the one that already got + this right — would drop out of the check. + """ + import re + bare_id = re.compile(r"^/$") + for path in sorted(ROUTES_DIR.glob("*.py")): + source = path.read_text() + tree = ast.parse(source) + helpers = { + node.name: ast.get_source_segment(source, node) or "" + for node in tree.body + if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and node.name.startswith("_") + } + for node in tree.body: + if not isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)): + continue + for dec in node.decorator_list: + if not isinstance(dec, ast.Call): + continue + route = next((a.value for a in dec.args + if isinstance(a, ast.Constant)), None) + if not isinstance(route, str) or not bare_id.match(route): + continue + methods = [ + e.value for kw in dec.keywords if kw.arg == "methods" + and isinstance(kw.value, ast.List) + for e in kw.value.elts if isinstance(e, ast.Constant) + ] + if "GET" not in methods: + continue + body = ast.get_source_segment(source, node) or "" + expanded = body + "".join( + src for name, src in helpers.items() if name in body + ) + yield path.name, node.name, expanded + + +def test_every_rest_detail_view_records_a_pull(): + """The human half of the same ledger. + + `rest_snippet` was recorded; note and task detail recorded nothing, so the + UI's most direct evidence of interest — someone opened the record — existed + for one kind out of three. The model's own comment listed `'rest_note'` as + a source, which means the design intended it and the implementation stopped + at snippets. + + Same derivation as the MCP test above, over the other surface: the routes + are discovered, and the ones that must record are identified by the loader + they reach. A `/` GET added for a fourth note-backed kind is covered + the day it is written. + """ + missing = [] + checked = [] + for module, handler, body in _detail_routes(): + if not any(loader in body for loader in SINGLE_NOTE_LOADERS): + continue # not note-backed — groups and projects land here + checked.append(f"{module}:{handler}") + if "record_pulled" not in body: + missing.append(f"{module}:{handler}") + + assert checked, ( + "found no note-backed detail routes; the route shape or the loader " + "names must have moved" + ) + assert not missing, ( + f"these detail views open a record without recording the pull: " + f"{missing}. Add record_pulled(user_id=…, note_id=…, source='rest_') " + f"before returning — see routes/snippets.py:get_snippet_route." + ) + + +def test_every_named_loader_still_exists(): + """Pins the hand-written list against a rename. + + A renamed loader is the failure that matters: the candidate set above would + quietly empty and the first test would pass while checking nothing. The + `assert checked` there catches it too; this says WHICH name moved, which is + the difference between a five-minute fix and a puzzle. + """ + from scribe.services import notes as notes_svc + from scribe.services import snippets as snippets_svc + + available = { + name + for svc in (notes_svc, snippets_svc) + for name, obj in vars(svc).items() + if inspect.iscoroutinefunction(obj) + } + gone = [name for name in SINGLE_NOTE_LOADERS if name not in available] + assert not gone, ( + f"SINGLE_NOTE_LOADERS names {gone} that no longer exist — they were " + f"renamed or moved. Update the list, or the pull check silently stops " + f"covering whatever used them." + ) 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"]