feat(snippets): reverse lookup — find snippets by repo/path/symbol
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 12s
CI & Build / integration (push) Failing after 27s
CI & Build / Python tests (push) Successful in 50s
CI & Build / Build & push image (push) Successful in 1m42s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 12s
CI & Build / integration (push) Failing after 27s
CI & Build / Python tests (push) Successful in 50s
CI & Build / Build & push image (push) Successful in 1m42s
"What canonical helpers already live in this file?" was unanswerable: location lived only in the body markdown. It is now a jsonpath containment query over the `notes.data` mirror added by migration 0070. - One predicate in two dialects in services/knowledge.py: SQL (`data @?`, applied in the browse arm and the keyword arm before count/pagination, so totals stay honest) and Python (`location_matches`, for the semantic arm which post-filters candidates it already holds). Both must change together. - Parts are ANDed within a SINGLE locations entry — repo A in one entry and path B in another is not "recorded at A/B". `path` also matches as a directory prefix, via jsonpath `starts with` rather than `@>`, which the same GIN index serves. - `repo`/`path`/`symbol` reach the service, the REST list and the MCP tool under one name with one default (rule #33); the MCP docstring teaches the place form, and so does the reusing-code skill (plugin.json bumped). - UI: a Location disclosure beside the snippet search, with its own empty state — "nothing kept there, so what you're about to write is new." Settles #2083's open question (pre-0070 NULL `data`) by backfilling after all: `backfill_snippet_data` runs at startup, deriving the mirror from the body with the same parser the read path trusts. 0070's caution was about mangling a hand-edited body; this never touches the body. The alternative was a permanent second body-regex arm, or a query that silently answers "nothing here" for an old snippet and gets the helper written twice. Refs #2083, milestone #232.
This commit is contained in:
@@ -80,13 +80,26 @@ export interface SnippetInput {
|
|||||||
force?: boolean;
|
force?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** `repo`/`path`/`symbol` are the reverse lookup — "what already lives here?"
|
||||||
|
* They must all match the same recorded location; `path` also matches anything
|
||||||
|
* beneath it. Combinable with `q`. */
|
||||||
export async function listSnippets(
|
export async function listSnippets(
|
||||||
params: { q?: string; tag?: string; projectId?: number | null } = {},
|
params: {
|
||||||
|
q?: string;
|
||||||
|
tag?: string;
|
||||||
|
projectId?: number | null;
|
||||||
|
repo?: string;
|
||||||
|
path?: string;
|
||||||
|
symbol?: string;
|
||||||
|
} = {},
|
||||||
): Promise<{ snippets: SnippetListItem[]; total: number }> {
|
): Promise<{ snippets: SnippetListItem[]; total: number }> {
|
||||||
const qs = new URLSearchParams();
|
const qs = new URLSearchParams();
|
||||||
if (params.q) qs.set("q", params.q);
|
if (params.q) qs.set("q", params.q);
|
||||||
if (params.tag) qs.set("tag", params.tag);
|
if (params.tag) qs.set("tag", params.tag);
|
||||||
if (params.projectId) qs.set("project_id", String(params.projectId));
|
if (params.projectId) qs.set("project_id", String(params.projectId));
|
||||||
|
if (params.repo) qs.set("repo", params.repo);
|
||||||
|
if (params.path) qs.set("path", params.path);
|
||||||
|
if (params.symbol) qs.set("symbol", params.symbol);
|
||||||
const query = qs.toString();
|
const query = qs.toString();
|
||||||
return apiGet(`/api/snippets${query ? `?${query}` : ""}`);
|
return apiGet(`/api/snippets${query ? `?${query}` : ""}`);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -13,6 +13,27 @@ const error = ref<string | null>(null);
|
|||||||
const search = ref("");
|
const search = ref("");
|
||||||
let searchTimer: ReturnType<typeof setTimeout> | undefined;
|
let searchTimer: ReturnType<typeof setTimeout> | undefined;
|
||||||
|
|
||||||
|
// Reverse lookup — "what already lives here?" All three must match the same
|
||||||
|
// recorded location; path also matches anything beneath it.
|
||||||
|
const showLocationFilter = ref(false);
|
||||||
|
const locRepo = ref("");
|
||||||
|
const locPath = ref("");
|
||||||
|
const locSymbol = ref("");
|
||||||
|
const locationActive = computed(
|
||||||
|
() => !!(locRepo.value.trim() || locPath.value.trim() || locSymbol.value.trim()),
|
||||||
|
);
|
||||||
|
function clearLocation() {
|
||||||
|
locRepo.value = "";
|
||||||
|
locPath.value = "";
|
||||||
|
locSymbol.value = "";
|
||||||
|
loadSnippets();
|
||||||
|
}
|
||||||
|
function toggleLocationFilter() {
|
||||||
|
showLocationFilter.value = !showLocationFilter.value;
|
||||||
|
// Collapsing while filtered would hide why the list is short.
|
||||||
|
if (!showLocationFilter.value && locationActive.value) clearLocation();
|
||||||
|
}
|
||||||
|
|
||||||
// Multi-select → merge
|
// Multi-select → merge
|
||||||
const selectMode = ref(false);
|
const selectMode = ref(false);
|
||||||
const selectedIds = ref<Set<number>>(new Set());
|
const selectedIds = ref<Set<number>>(new Set());
|
||||||
@@ -70,7 +91,12 @@ async function loadSnippets() {
|
|||||||
loading.value = true;
|
loading.value = true;
|
||||||
error.value = null;
|
error.value = null;
|
||||||
try {
|
try {
|
||||||
const data = await listSnippets({ q: search.value.trim() || undefined });
|
const data = await listSnippets({
|
||||||
|
q: search.value.trim() || undefined,
|
||||||
|
repo: locRepo.value.trim() || undefined,
|
||||||
|
path: locPath.value.trim() || undefined,
|
||||||
|
symbol: locSymbol.value.trim() || undefined,
|
||||||
|
});
|
||||||
snippets.value = data.snippets;
|
snippets.value = data.snippets;
|
||||||
} catch {
|
} catch {
|
||||||
error.value = "Couldn't load your snippets.";
|
error.value = "Couldn't load your snippets.";
|
||||||
@@ -84,6 +110,9 @@ function onSearchInput() {
|
|||||||
clearTimeout(searchTimer);
|
clearTimeout(searchTimer);
|
||||||
searchTimer = setTimeout(loadSnippets, 300);
|
searchTimer = setTimeout(loadSnippets, 300);
|
||||||
}
|
}
|
||||||
|
// Same debounce for the location fields — typing a path shouldn't fire a query
|
||||||
|
// per keystroke.
|
||||||
|
const onLocationInput = onSearchInput;
|
||||||
|
|
||||||
onMounted(loadSnippets);
|
onMounted(loadSnippets);
|
||||||
|
|
||||||
@@ -127,6 +156,48 @@ function languageOf(tags: string[]): string {
|
|||||||
@input="onSearchInput"
|
@input="onSearchInput"
|
||||||
@keydown.enter="loadSnippets"
|
@keydown.enter="loadSnippets"
|
||||||
/>
|
/>
|
||||||
|
<button
|
||||||
|
class="btn-ghost"
|
||||||
|
:class="{ 'filter-on': locationActive }"
|
||||||
|
:aria-expanded="showLocationFilter"
|
||||||
|
aria-controls="location-filter"
|
||||||
|
@click="toggleLocationFilter"
|
||||||
|
>
|
||||||
|
<!-- Say it in the label, not only in the accent — the state has to reach
|
||||||
|
a screen reader too. -->
|
||||||
|
{{ locationActive ? "Location · filtering" : "Location" }}
|
||||||
|
</button>
|
||||||
|
</div>
|
||||||
|
|
||||||
|
<!-- Reverse lookup: what's already kept in this repo / file / symbol. -->
|
||||||
|
<div v-if="showLocationFilter" id="location-filter" class="location-row">
|
||||||
|
<input
|
||||||
|
v-model="locRepo"
|
||||||
|
class="loc-input"
|
||||||
|
placeholder="repo"
|
||||||
|
aria-label="Filter by repo"
|
||||||
|
@input="onLocationInput"
|
||||||
|
@keydown.enter="loadSnippets"
|
||||||
|
/>
|
||||||
|
<input
|
||||||
|
v-model="locPath"
|
||||||
|
class="loc-input loc-input-wide"
|
||||||
|
placeholder="path — matches the file or anything under it"
|
||||||
|
aria-label="Filter by path"
|
||||||
|
@input="onLocationInput"
|
||||||
|
@keydown.enter="loadSnippets"
|
||||||
|
/>
|
||||||
|
<input
|
||||||
|
v-model="locSymbol"
|
||||||
|
class="loc-input"
|
||||||
|
placeholder="symbol"
|
||||||
|
aria-label="Filter by symbol"
|
||||||
|
@input="onLocationInput"
|
||||||
|
@keydown.enter="loadSnippets"
|
||||||
|
/>
|
||||||
|
<button v-if="locationActive" class="loc-clear" @click="clearLocation">
|
||||||
|
Clear
|
||||||
|
</button>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
<div v-if="loading" class="skeleton-grid">
|
<div v-if="loading" class="skeleton-grid">
|
||||||
@@ -138,15 +209,24 @@ function languageOf(tags: string[]): string {
|
|||||||
<div v-else-if="snippets.length === 0" class="empty-state-rich">
|
<div v-else-if="snippets.length === 0" class="empty-state-rich">
|
||||||
<div class="empty-icon">❭_</div>
|
<div class="empty-icon">❭_</div>
|
||||||
<p class="empty-title">
|
<p class="empty-title">
|
||||||
{{ search.trim() ? "No snippets match your search" : "No snippets kept yet" }}
|
{{ locationActive
|
||||||
|
? "Nothing kept at that location yet"
|
||||||
|
: search.trim()
|
||||||
|
? "No snippets match your search"
|
||||||
|
: "No snippets kept yet" }}
|
||||||
</p>
|
</p>
|
||||||
<p class="empty-sub">
|
<p class="empty-sub">
|
||||||
{{ search.trim()
|
{{ locationActive
|
||||||
|
? "No recorded snippet lives there — so whatever you're about to write is new. Widen the path, or clear the filter."
|
||||||
|
: search.trim()
|
||||||
? "Try a different term, or clear the search."
|
? "Try a different term, or clear the search."
|
||||||
: "Record a reusable function or component and it will be offered back to you later." }}
|
: "Record a reusable function or component and it will be offered back to you later." }}
|
||||||
</p>
|
</p>
|
||||||
|
<button v-if="locationActive" class="empty-action" @click="clearLocation">
|
||||||
|
Clear location filter
|
||||||
|
</button>
|
||||||
<button
|
<button
|
||||||
v-if="!search.trim()"
|
v-else-if="!search.trim()"
|
||||||
class="empty-action"
|
class="empty-action"
|
||||||
@click="router.push('/snippets/new')"
|
@click="router.push('/snippets/new')"
|
||||||
>
|
>
|
||||||
@@ -278,6 +358,62 @@ function languageOf(tags: string[]): string {
|
|||||||
|
|
||||||
.search-row {
|
.search-row {
|
||||||
margin-bottom: 1.25rem;
|
margin-bottom: 1.25rem;
|
||||||
|
display: flex;
|
||||||
|
gap: 0.5rem;
|
||||||
|
align-items: center;
|
||||||
|
flex-wrap: wrap;
|
||||||
|
}
|
||||||
|
/* Filter is engaged — the accent marks "you are here", per the design system. */
|
||||||
|
.filter-on {
|
||||||
|
border-color: var(--color-primary);
|
||||||
|
color: var(--color-primary);
|
||||||
|
}
|
||||||
|
|
||||||
|
.location-row {
|
||||||
|
display: flex;
|
||||||
|
gap: 0.5rem;
|
||||||
|
align-items: center;
|
||||||
|
flex-wrap: wrap;
|
||||||
|
margin: -0.5rem 0 1.25rem;
|
||||||
|
}
|
||||||
|
.loc-input {
|
||||||
|
flex: 1 1 8rem;
|
||||||
|
min-width: 0;
|
||||||
|
max-width: 12rem;
|
||||||
|
padding: 0.4rem 0.65rem;
|
||||||
|
border: 1px solid var(--color-border);
|
||||||
|
border-radius: var(--radius-sm);
|
||||||
|
background: var(--color-bg);
|
||||||
|
color: var(--color-text);
|
||||||
|
font-size: 0.85rem;
|
||||||
|
font-family: var(--font-mono, ui-monospace, "JetBrains Mono", monospace);
|
||||||
|
box-sizing: border-box;
|
||||||
|
}
|
||||||
|
.loc-input-wide {
|
||||||
|
flex: 2 1 16rem;
|
||||||
|
max-width: 26rem;
|
||||||
|
}
|
||||||
|
.loc-input::placeholder {
|
||||||
|
font-family: inherit;
|
||||||
|
color: var(--color-text-muted);
|
||||||
|
}
|
||||||
|
.loc-input:focus {
|
||||||
|
outline: none;
|
||||||
|
border-color: var(--color-primary);
|
||||||
|
box-shadow: var(--focus-ring);
|
||||||
|
}
|
||||||
|
.loc-clear {
|
||||||
|
padding: 0.4rem 0.65rem;
|
||||||
|
border: none;
|
||||||
|
background: transparent;
|
||||||
|
color: var(--color-text-secondary);
|
||||||
|
font-size: 0.85rem;
|
||||||
|
font-family: inherit;
|
||||||
|
cursor: pointer;
|
||||||
|
text-decoration: underline;
|
||||||
|
}
|
||||||
|
.loc-clear:hover {
|
||||||
|
color: var(--color-text);
|
||||||
}
|
}
|
||||||
.search-input {
|
.search-input {
|
||||||
width: 100%;
|
width: 100%;
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
{
|
{
|
||||||
"name": "scribe",
|
"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.",
|
"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.16",
|
"version": "0.1.17",
|
||||||
"author": { "name": "Bryan Van Deusen" },
|
"author": { "name": "Bryan Van Deusen" },
|
||||||
"mcpServers": {
|
"mcpServers": {
|
||||||
"scribe": {
|
"scribe": {
|
||||||
|
|||||||
@@ -20,6 +20,14 @@ through recall/auto-inject; this skill is the active reflex around that.
|
|||||||
`list_snippets` searches every project by default; that's deliberate, since a
|
`list_snippets` searches every project by default; that's deliberate, since a
|
||||||
helper you need here was quite possibly written somewhere else. Narrow with
|
helper you need here was quite possibly written somewhere else. Narrow with
|
||||||
`project_id` only when you specifically want this project's own.
|
`project_id` only when you specifically want this project's own.
|
||||||
|
- **About to edit an existing file? Ask by place, not just by meaning.**
|
||||||
|
`list_snippets(repo="…", path="…")` answers "what canonical helpers are already
|
||||||
|
recorded here?" — `path` matches the exact file or anything beneath it, so
|
||||||
|
`path="frontend/src"` covers the whole tree. Cheaper and sharper than a
|
||||||
|
wording search when you already know where the code is going, and it catches
|
||||||
|
the helper you'd otherwise duplicate a few lines down. `symbol="…"` narrows
|
||||||
|
further, and all three must match the same recorded location. Combine with `q`
|
||||||
|
to ask both at once.
|
||||||
- If a snippet fits, pull it in full with `get_snippet(id)` and reuse it — its
|
- If a snippet fits, pull it in full with `get_snippet(id)` and reuse it — its
|
||||||
`location` points at the reference implementation. Adapt, don't re-derive.
|
`location` points at the reference implementation. Adapt, don't re-derive.
|
||||||
- If auto-inject already surfaced a snippet title that looks relevant, that's
|
- If auto-inject already surfaced a snippet title that looks relevant, that's
|
||||||
|
|||||||
@@ -170,6 +170,15 @@ def create_app() -> Quart:
|
|||||||
await backfill_note_embeddings()
|
await backfill_note_embeddings()
|
||||||
except Exception:
|
except Exception:
|
||||||
logger.warning("Embedding backfill failed", exc_info=True)
|
logger.warning("Embedding backfill failed", exc_info=True)
|
||||||
|
# Snippets written before migration 0070 have no `notes.data` mirror,
|
||||||
|
# and the location reverse lookup queries that column — an unfilled
|
||||||
|
# row would read as "no snippet here" rather than as a gap. Separate
|
||||||
|
# try block so neither backfill can skip the other.
|
||||||
|
try:
|
||||||
|
from scribe.services.snippets import backfill_snippet_data
|
||||||
|
await backfill_snippet_data()
|
||||||
|
except Exception:
|
||||||
|
logger.warning("Snippet data backfill failed", exc_info=True)
|
||||||
|
|
||||||
asyncio.create_task(_delayed_backfill())
|
asyncio.create_task(_delayed_backfill())
|
||||||
|
|
||||||
|
|||||||
@@ -19,9 +19,16 @@ from scribe.services import systems as systems_svc
|
|||||||
|
|
||||||
async def list_snippets(
|
async def list_snippets(
|
||||||
q: str = "", tag: str = "", limit: int = 50, project_id: int = 0,
|
q: str = "", tag: str = "", limit: int = 50, project_id: int = 0,
|
||||||
|
repo: str = "", path: str = "", symbol: str = "",
|
||||||
) -> dict:
|
) -> dict:
|
||||||
"""List recorded snippets (reusable functions/components).
|
"""List recorded snippets (reusable functions/components).
|
||||||
|
|
||||||
|
Two ways to ask, usable together: by MEANING (`q` — "what do I need this code
|
||||||
|
to do?") and by PLACE (`repo`/`path`/`symbol` — "what canonical helpers
|
||||||
|
already live in the file I'm about to edit?"). Reach for the place form
|
||||||
|
before you write or change code in a file: it is the cheap way to find prior
|
||||||
|
art you'd otherwise duplicate a few lines down.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
q: Free-text search across name + body (optional). Matches on meaning as
|
q: Free-text search across name + body (optional). Matches on meaning as
|
||||||
well as wording, so describe what you need the code to DO.
|
well as wording, so describe what you need the code to DO.
|
||||||
@@ -30,6 +37,16 @@ async def list_snippets(
|
|||||||
project_id: Narrow to one project. 0 (default) searches every project —
|
project_id: Narrow to one project. 0 (default) searches every project —
|
||||||
usually what you want, since a helper you need here may well have
|
usually what you want, since a helper you need here may well have
|
||||||
been written somewhere else.
|
been written somewhere else.
|
||||||
|
repo: Narrow to snippets recorded in this repo, matched exactly — the
|
||||||
|
same repo string used when recording, e.g. "Scribe".
|
||||||
|
path: Narrow to snippets recorded at this path. Matches the exact file
|
||||||
|
OR anything beneath it, so "frontend/src" finds
|
||||||
|
"frontend/src/lib/x.ts" as well as itself.
|
||||||
|
symbol: Narrow to snippets recorded under this symbol name, exactly.
|
||||||
|
|
||||||
|
`repo`/`path`/`symbol` must all match the SAME recorded location, so a
|
||||||
|
snippet that lives in repo A and, separately, at path B in another repo is
|
||||||
|
not returned for repo=A + path=B.
|
||||||
|
|
||||||
Returns {"snippets": [{id, title, tags, preview}], "total": int}. The title
|
Returns {"snippets": [{id, title, tags, preview}], "total": int}. The title
|
||||||
reads "name — when to reach for it"; open one in full with get_snippet(id).
|
reads "name — when to reach for it"; open one in full with get_snippet(id).
|
||||||
@@ -44,6 +61,7 @@ async def list_snippets(
|
|||||||
items, total = await snippets_svc.list_snippets(
|
items, total = await snippets_svc.list_snippets(
|
||||||
uid, q=q or None, tag=tag, limit=max(1, min(limit, 100)),
|
uid, q=q or None, tag=tag, limit=max(1, min(limit, 100)),
|
||||||
project_id=project_id or None,
|
project_id=project_id or None,
|
||||||
|
repo=repo, path=path, symbol=symbol,
|
||||||
)
|
)
|
||||||
return {
|
return {
|
||||||
"snippets": await access_svc.label_shared_items(uid, items),
|
"snippets": await access_svc.label_shared_items(uid, items),
|
||||||
|
|||||||
@@ -56,9 +56,15 @@ async def list_snippets_route():
|
|||||||
project_id = int(request.args.get("project_id", 0) or 0) or None
|
project_id = int(request.args.get("project_id", 0) or 0) or None
|
||||||
except (TypeError, ValueError):
|
except (TypeError, ValueError):
|
||||||
project_id = None
|
project_id = None
|
||||||
|
# Reverse lookup — "what already lives here?" Empty args are ignored, so the
|
||||||
|
# plain list is unchanged.
|
||||||
|
repo = request.args.get("repo", "")
|
||||||
|
path = request.args.get("path", "")
|
||||||
|
symbol = request.args.get("symbol", "")
|
||||||
limit, offset = parse_pagination()
|
limit, offset = parse_pagination()
|
||||||
items, total = await snippets_svc.list_snippets(
|
items, total = await snippets_svc.list_snippets(
|
||||||
uid, q=q, tag=tag, limit=limit, offset=offset, project_id=project_id,
|
uid, q=q, tag=tag, limit=limit, offset=offset, project_id=project_id,
|
||||||
|
repo=repo, path=path, symbol=symbol,
|
||||||
)
|
)
|
||||||
# Mark rows owned by someone else so the UI can show whose they are — an
|
# Mark rows owned by someone else so the UI can show whose they are — an
|
||||||
# unmarked row in your own list reads as one you recorded and vetted.
|
# unmarked row in your own list reads as one you recorded and vetted.
|
||||||
|
|||||||
@@ -15,6 +15,7 @@ The asymmetry is the point. A record that appears unasked reads as one you
|
|||||||
endorsed, so a one-off direct share has to be searched for rather than arriving
|
endorsed, so a one-off direct share has to be searched for rather than arriving
|
||||||
in your ambient lists.
|
in your ambient lists.
|
||||||
"""
|
"""
|
||||||
|
import json
|
||||||
import logging
|
import logging
|
||||||
|
|
||||||
from sqlalchemy import func, select
|
from sqlalchemy import func, select
|
||||||
@@ -28,6 +29,85 @@ logger = logging.getLogger(__name__)
|
|||||||
_SNIPPET_LEN = 200
|
_SNIPPET_LEN = 200
|
||||||
|
|
||||||
|
|
||||||
|
# --- the location filter (reverse lookup, #2083) ------------------------------
|
||||||
|
#
|
||||||
|
# ONE predicate in two dialects: "this record carries a location matching every
|
||||||
|
# part asked for." The SQL form runs in the browse arm and the keyword arm,
|
||||||
|
# before the count and the page slice, so totals stay honest; the Python form
|
||||||
|
# runs in the semantic arm, which post-filters candidates it already holds in
|
||||||
|
# memory. THE TWO MUST CHANGE TOGETHER — a filter only one arm applies makes a
|
||||||
|
# snippet findable by wording and invisible by location, which is exactly the
|
||||||
|
# split tests/test_retrieval_scopes.py exists to prevent.
|
||||||
|
#
|
||||||
|
# Semantics, both dialects:
|
||||||
|
# - parts are ANDed WITHIN a single location entry, never across the list. A
|
||||||
|
# snippet whose first location is in repo A and whose second is at path B
|
||||||
|
# does not answer repo=A + path=B — it was never in that place.
|
||||||
|
# - `path` matches exactly OR as a directory prefix: "frontend/src" finds
|
||||||
|
# "frontend/src/lib/x.ts". Prefix is the one part a GIN containment lookup
|
||||||
|
# can't serve (migration 0070), which is why this is a jsonpath `@?` rather
|
||||||
|
# than `@>` — jsonpath `starts with` is index-served by the same GIN index.
|
||||||
|
#
|
||||||
|
# The filter reads `notes.data`, so it only sees rows carrying that mirror.
|
||||||
|
# Rows written before migration 0070 are populated once at startup by
|
||||||
|
# snippets.backfill_snippet_data — without that, this query would answer "no
|
||||||
|
# snippets here" for an old snippet and the caller would write the helper again.
|
||||||
|
|
||||||
|
LOCATION_KEYS = ("repo", "path", "symbol")
|
||||||
|
|
||||||
|
|
||||||
|
def location_parts(repo: str = "", path: str = "", symbol: str = "") -> dict[str, str]:
|
||||||
|
"""The non-empty, stripped location parts asked for; {} when none were."""
|
||||||
|
given = {"repo": repo, "path": path, "symbol": symbol}
|
||||||
|
return {k: (v or "").strip() for k, v in given.items() if (v or "").strip()}
|
||||||
|
|
||||||
|
|
||||||
|
def _path_matches(have: str, want: str) -> bool:
|
||||||
|
"""Exact, or `have` sits somewhere under the `want` directory."""
|
||||||
|
return have == want or have.startswith(want.rstrip("/") + "/")
|
||||||
|
|
||||||
|
|
||||||
|
def location_matches(data: dict | None, parts: dict[str, str]) -> bool:
|
||||||
|
"""Python dialect of the location predicate. Keep in step with _location_clause."""
|
||||||
|
if not parts:
|
||||||
|
return True
|
||||||
|
for loc in (data or {}).get("locations") or []:
|
||||||
|
if all(
|
||||||
|
_path_matches((loc.get(key) or "").strip(), want)
|
||||||
|
if key == "path"
|
||||||
|
else (loc.get(key) or "").strip() == want
|
||||||
|
for key, want in parts.items()
|
||||||
|
):
|
||||||
|
return True
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
def location_jsonpath(parts: dict[str, str]) -> str:
|
||||||
|
"""The jsonpath behind the SQL dialect — one `locations` entry matching all parts.
|
||||||
|
|
||||||
|
Values are embedded as JSON string literals (jsonpath uses JSON quoting), so
|
||||||
|
a repo or path carrying a quote can't break out of the expression. The keys
|
||||||
|
are our own fixed set, never caller input.
|
||||||
|
"""
|
||||||
|
filters = []
|
||||||
|
for key in LOCATION_KEYS:
|
||||||
|
if key not in parts:
|
||||||
|
continue
|
||||||
|
want = parts[key]
|
||||||
|
literal = json.dumps(want)
|
||||||
|
if key == "path":
|
||||||
|
prefix = json.dumps(want.rstrip("/") + "/")
|
||||||
|
filters.append(f"(@.path == {literal} || @.path starts with {prefix})")
|
||||||
|
else:
|
||||||
|
filters.append(f"@.{key} == {literal}")
|
||||||
|
return f"$.locations[*] ? ({' && '.join(filters)})"
|
||||||
|
|
||||||
|
|
||||||
|
def _location_clause(parts: dict[str, str]):
|
||||||
|
"""SQL dialect of the location predicate. Keep in step with location_matches."""
|
||||||
|
return Note.data.path_exists(location_jsonpath(parts))
|
||||||
|
|
||||||
|
|
||||||
def _note_to_item(note: Note) -> dict:
|
def _note_to_item(note: Note) -> dict:
|
||||||
item: dict = {
|
item: dict = {
|
||||||
"id": note.id,
|
"id": note.id,
|
||||||
@@ -80,18 +160,24 @@ async def query_knowledge(
|
|||||||
limit: int,
|
limit: int,
|
||||||
offset: int,
|
offset: int,
|
||||||
project_id: int | None = None,
|
project_id: int | None = None,
|
||||||
|
locations: dict[str, str] | None = None,
|
||||||
) -> tuple[list[dict], int]:
|
) -> tuple[list[dict], int]:
|
||||||
"""Query knowledge objects (non-task notes) with filters.
|
"""Query knowledge objects (non-task notes) with filters.
|
||||||
|
|
||||||
`project_id` narrows to one project (None = every project).
|
`project_id` narrows to one project (None = every project).
|
||||||
|
|
||||||
|
`locations` narrows to records whose `data.locations` holds an entry matching
|
||||||
|
every part given — build it with `location_parts(repo=…, path=…, symbol=…)`.
|
||||||
|
Today only snippets carry locations, but the column is general, so the filter
|
||||||
|
lives here with the query rather than in one type's service.
|
||||||
|
|
||||||
Returns (items, total_count).
|
Returns (items, total_count).
|
||||||
"""
|
"""
|
||||||
# Semantic search path — scores take priority over sort
|
# Semantic search path — scores take priority over sort
|
||||||
if q:
|
if q:
|
||||||
return await _semantic_knowledge_search(
|
return await _semantic_knowledge_search(
|
||||||
user_id, q, note_type=note_type, tags=tags, limit=limit,
|
user_id, q, note_type=note_type, tags=tags, limit=limit,
|
||||||
offset=offset, project_id=project_id,
|
offset=offset, project_id=project_id, locations=locations,
|
||||||
)
|
)
|
||||||
|
|
||||||
# No query = browsing. Narrower scope: a record shared directly with the
|
# No query = browsing. Narrower scope: a record shared directly with the
|
||||||
@@ -107,6 +193,9 @@ async def query_knowledge(
|
|||||||
for tag in tags:
|
for tag in tags:
|
||||||
base = base.where(Note.tags.contains([tag]))
|
base = base.where(Note.tags.contains([tag]))
|
||||||
|
|
||||||
|
if locations:
|
||||||
|
base = base.where(_location_clause(locations))
|
||||||
|
|
||||||
# Count before pagination
|
# Count before pagination
|
||||||
count_stmt = select(func.count()).select_from(base.subquery())
|
count_stmt = select(func.count()).select_from(base.subquery())
|
||||||
total: int = (await session.execute(count_stmt)).scalar_one()
|
total: int = (await session.execute(count_stmt)).scalar_one()
|
||||||
@@ -134,6 +223,7 @@ async def _semantic_knowledge_search(
|
|||||||
limit: int,
|
limit: int,
|
||||||
offset: int,
|
offset: int,
|
||||||
project_id: int | None = None,
|
project_id: int | None = None,
|
||||||
|
locations: dict[str, str] | None = None,
|
||||||
) -> tuple[list[dict], int]:
|
) -> tuple[list[dict], int]:
|
||||||
"""Hybrid search: keyword matches first (title/body ILIKE), then semantic results.
|
"""Hybrid search: keyword matches first (title/body ILIKE), then semantic results.
|
||||||
|
|
||||||
@@ -167,6 +257,8 @@ async def _semantic_knowledge_search(
|
|||||||
base = base.where(Note.project_id == project_id)
|
base = base.where(Note.project_id == project_id)
|
||||||
for tag in tags:
|
for tag in tags:
|
||||||
base = base.where(Note.tags.contains([tag]))
|
base = base.where(Note.tags.contains([tag]))
|
||||||
|
if locations:
|
||||||
|
base = base.where(_location_clause(locations))
|
||||||
# Title matches first, then body-only matches, newest first within each
|
# Title matches first, then body-only matches, newest first within each
|
||||||
base = base.order_by(
|
base = base.order_by(
|
||||||
Note.title.ilike(pattern).desc(),
|
Note.title.ilike(pattern).desc(),
|
||||||
@@ -204,6 +296,11 @@ async def _semantic_knowledge_search(
|
|||||||
continue
|
continue
|
||||||
if tags and not all(t in (note.tags or []) for t in tags):
|
if tags and not all(t in (note.tags or []) for t in tags):
|
||||||
continue
|
continue
|
||||||
|
# The Python dialect of the same predicate the SQL arms apply above —
|
||||||
|
# these candidates arrive already fetched, so there's no query to
|
||||||
|
# narrow. See the comment on location_matches.
|
||||||
|
if locations and not location_matches(note.data, locations):
|
||||||
|
continue
|
||||||
semantic_notes.append(note)
|
semantic_notes.append(note)
|
||||||
except Exception:
|
except Exception:
|
||||||
logger.warning("Semantic search unavailable, using keyword results only", exc_info=True)
|
logger.warning("Semantic search unavailable, using keyword results only", exc_info=True)
|
||||||
|
|||||||
@@ -5,10 +5,10 @@ component recorded once so a later session can recall it before writing a
|
|||||||
one-off. It carries a name, language, signature, canonical location
|
one-off. It carries a name, language, signature, canonical location
|
||||||
(repo · path · symbol), a one-line "when to reach for it", and the code itself.
|
(repo · path · symbol), a one-line "when to reach for it", and the code itself.
|
||||||
|
|
||||||
Structured fields are stored as a **body-convention** — no dedicated column, so
|
Structured fields are stored as a **body-convention** — the body is the readable
|
||||||
a snippet inherits everything a note has (embeddings, ACL, project/System
|
form and the thing that gets embedded, so a snippet inherits everything a note
|
||||||
association, dedup) and, crucially, becomes eligible for semantic recall the
|
has (embeddings, ACL, project/System association, dedup) and, crucially, becomes
|
||||||
moment it's embedded:
|
eligible for semantic recall the moment it's embedded:
|
||||||
|
|
||||||
- ``title`` = ``"{name} — {when_to_use}"``. The title is exactly what the
|
- ``title`` = ``"{name} — {when_to_use}"``. The title is exactly what the
|
||||||
title-first auto-inject surfaces, so this one line self-describes the snippet
|
title-first auto-inject surfaces, so this one line self-describes the snippet
|
||||||
@@ -17,18 +17,30 @@ moment it's embedded:
|
|||||||
- ``body`` = templated markdown (When to use / Signature / Location, then a
|
- ``body`` = templated markdown (When to use / Signature / Location, then a
|
||||||
fenced code block).
|
fenced code block).
|
||||||
|
|
||||||
|
Since migration 0070 the same fields are ALSO written to ``notes.data`` (see
|
||||||
|
``compose_data``) — a queryable mirror, not a second source of truth: it carries
|
||||||
|
no code, and it exists so location/language can be indexed rather than regexed
|
||||||
|
out of every body. Reads prefer it and fall back to the body parse.
|
||||||
|
|
||||||
The public field API (name/language/signature/location/when_to_use/code) lives
|
The public field API (name/language/signature/location/when_to_use/code) lives
|
||||||
here, so the underlying storage could later move to a structured column without
|
here, so callers (MCP tool, REST route, UI) never see which of the two a field
|
||||||
changing any caller (MCP tool, REST route, UI).
|
came from.
|
||||||
"""
|
"""
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import asyncio
|
import asyncio
|
||||||
|
import logging
|
||||||
import re
|
import re
|
||||||
|
|
||||||
|
from sqlalchemy import select
|
||||||
|
|
||||||
|
from scribe.models import async_session
|
||||||
|
from scribe.models.note import Note
|
||||||
from scribe.services import knowledge as knowledge_svc
|
from scribe.services import knowledge as knowledge_svc
|
||||||
from scribe.services import notes as notes_svc
|
from scribe.services import notes as notes_svc
|
||||||
|
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
SNIPPET_NOTE_TYPE = "snippet"
|
SNIPPET_NOTE_TYPE = "snippet"
|
||||||
SNIPPET_TAG = "snippet"
|
SNIPPET_TAG = "snippet"
|
||||||
|
|
||||||
@@ -354,6 +366,63 @@ def snippet_fields(note) -> dict:
|
|||||||
return merged
|
return merged
|
||||||
|
|
||||||
|
|
||||||
|
async def backfill_snippet_data(*, batch: int = 500) -> int:
|
||||||
|
"""Populate `notes.data` for snippet rows that predate migration 0070.
|
||||||
|
|
||||||
|
Runs once at startup (see app.py). Returns how many rows were filled.
|
||||||
|
|
||||||
|
Migration 0070 deliberately left `data` NULL on existing rows, because
|
||||||
|
*reading* a snippet never needed it — `snippet_fields` falls back to parsing
|
||||||
|
the body. Querying does: the location reverse lookup (#2083) is a jsonpath
|
||||||
|
over this column, so a NULL-`data` snippet would be reported as "nothing
|
||||||
|
recorded here" and the caller would write the helper again — silently worse
|
||||||
|
than having no reverse lookup at all. Every consumer of the column
|
||||||
|
(reverse lookup, and the write-path trigger built on it) can then assume the
|
||||||
|
mirror is present instead of carrying a second body-regex arm.
|
||||||
|
|
||||||
|
This does not reverse 0070's actual caution, which was about mangling a
|
||||||
|
hand-edited *body*: the body is never touched here, only the mirror derived
|
||||||
|
from it, by the same parser the read path already trusts. Idempotent — a
|
||||||
|
filled row is skipped forever after, and a snippet with no structured fields
|
||||||
|
at all settles at `{}` rather than staying NULL and being re-scanned. Trashed
|
||||||
|
rows are included so a later restore comes back queryable.
|
||||||
|
"""
|
||||||
|
filled = 0
|
||||||
|
async with async_session() as session:
|
||||||
|
while True:
|
||||||
|
rows = list(
|
||||||
|
(
|
||||||
|
await session.execute(
|
||||||
|
select(Note)
|
||||||
|
.where(Note.note_type == SNIPPET_NOTE_TYPE)
|
||||||
|
.where(Note.data.is_(None))
|
||||||
|
.limit(batch)
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.scalars()
|
||||||
|
.all()
|
||||||
|
)
|
||||||
|
if not rows:
|
||||||
|
break
|
||||||
|
for note in rows:
|
||||||
|
fields = parse_snippet_fields(note.title, note.body, note.tags)
|
||||||
|
note.data = compose_data(
|
||||||
|
name=fields["name"],
|
||||||
|
when_to_use=fields["when_to_use"],
|
||||||
|
signature=fields["signature"],
|
||||||
|
language=fields["language"],
|
||||||
|
locations=fields["locations"],
|
||||||
|
merged_from=fields["merged_from"],
|
||||||
|
)
|
||||||
|
await session.commit()
|
||||||
|
filled += len(rows)
|
||||||
|
if len(rows) < batch:
|
||||||
|
break
|
||||||
|
if filled:
|
||||||
|
logger.info("Snippet data backfill: populated `data` for %d snippet(s)", filled)
|
||||||
|
return filled
|
||||||
|
|
||||||
|
|
||||||
def snippet_to_dict(note) -> dict:
|
def snippet_to_dict(note) -> dict:
|
||||||
"""Note serialization plus a parsed ``snippet`` sub-object of structured
|
"""Note serialization plus a parsed ``snippet`` sub-object of structured
|
||||||
fields, so callers get both the raw record and the typed view.
|
fields, so callers get both the raw record and the typed view.
|
||||||
@@ -433,12 +502,20 @@ async def list_snippets(
|
|||||||
limit: int = 50,
|
limit: int = 50,
|
||||||
offset: int = 0,
|
offset: int = 0,
|
||||||
project_id: int | None = None,
|
project_id: int | None = None,
|
||||||
|
repo: str = "",
|
||||||
|
path: str = "",
|
||||||
|
symbol: str = "",
|
||||||
) -> tuple[list[dict], int]:
|
) -> tuple[list[dict], int]:
|
||||||
"""List snippets (id/title/tags/preview dicts), most-recently-updated first.
|
"""List snippets (id/title/tags/preview dicts), most-recently-updated first.
|
||||||
|
|
||||||
``project_id`` narrows to one project; omit it to reach across every project
|
``project_id`` narrows to one project; omit it to reach across every project
|
||||||
— which is the point when the thing you're about to write was already solved
|
— which is the point when the thing you're about to write was already solved
|
||||||
somewhere else."""
|
somewhere else.
|
||||||
|
|
||||||
|
``repo`` / ``path`` / ``symbol`` are the reverse lookup: "what canonical
|
||||||
|
helpers already live here?" They narrow to snippets recorded at a matching
|
||||||
|
location, ANDed within one location entry, with ``path`` also matching as a
|
||||||
|
directory prefix. Combinable with ``q`` — search *and* place."""
|
||||||
return await knowledge_svc.query_knowledge(
|
return await knowledge_svc.query_knowledge(
|
||||||
user_id=user_id,
|
user_id=user_id,
|
||||||
note_type=SNIPPET_NOTE_TYPE,
|
note_type=SNIPPET_NOTE_TYPE,
|
||||||
@@ -448,6 +525,8 @@ async def list_snippets(
|
|||||||
limit=max(1, min(limit, 100)),
|
limit=max(1, min(limit, 100)),
|
||||||
offset=max(0, offset),
|
offset=max(0, offset),
|
||||||
project_id=project_id,
|
project_id=project_id,
|
||||||
|
locations=knowledge_svc.location_parts(repo=repo, path=path, symbol=symbol)
|
||||||
|
or None,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,299 @@
|
|||||||
|
"""Real-Postgres integration test for the snippet location reverse lookup (#2083).
|
||||||
|
|
||||||
|
Runs only in the CI integration lane (real Postgres, schema built by
|
||||||
|
`alembic upgrade head`, which includes migration 0070's `notes.data` + GIN index).
|
||||||
|
This exercises what the unit mocks cannot:
|
||||||
|
|
||||||
|
- the jsonpath `@?` containment behind `_location_clause` — including
|
||||||
|
`starts with` for a path prefix, and the fact that parts must match within a
|
||||||
|
SINGLE `locations` entry;
|
||||||
|
- that the SQL dialect and the Python dialect (`location_matches`, used by the
|
||||||
|
semantic arm) return the SAME rows. Those two must never drift, or a snippet
|
||||||
|
is findable by wording and invisible by location;
|
||||||
|
- `backfill_snippet_data`, which is what stops a pre-0070 snippet from being
|
||||||
|
silently reported as "nothing recorded here."
|
||||||
|
"""
|
||||||
|
from unittest.mock import AsyncMock, patch
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
import pytest_asyncio
|
||||||
|
from sqlalchemy import delete, select
|
||||||
|
|
||||||
|
from scribe.models import async_session, engine
|
||||||
|
from scribe.models.note import Note
|
||||||
|
from scribe.models.user import User
|
||||||
|
from scribe.services.knowledge import location_matches, location_parts
|
||||||
|
from scribe.services.snippets import (
|
||||||
|
SNIPPET_NOTE_TYPE,
|
||||||
|
backfill_snippet_data,
|
||||||
|
compose_body,
|
||||||
|
compose_data,
|
||||||
|
list_snippets,
|
||||||
|
snippet_fields,
|
||||||
|
)
|
||||||
|
|
||||||
|
pytestmark = pytest.mark.integration
|
||||||
|
|
||||||
|
|
||||||
|
@pytest_asyncio.fixture(autouse=True)
|
||||||
|
async def _dispose_engine():
|
||||||
|
# Per-loop pool: dispose after each test (see test_integration_db_maintenance).
|
||||||
|
yield
|
||||||
|
await engine.dispose()
|
||||||
|
|
||||||
|
|
||||||
|
def _loc(repo="", path="", symbol=""):
|
||||||
|
return {"repo": repo, "path": path, "symbol": symbol}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest_asyncio.fixture
|
||||||
|
async def seeded():
|
||||||
|
"""A user with four snippets covering the cases the filter has to separate.
|
||||||
|
|
||||||
|
Returns (user_id, {label: note_id}).
|
||||||
|
"""
|
||||||
|
async with async_session() as s:
|
||||||
|
user = User(username="snippet_loc_itest")
|
||||||
|
s.add(user)
|
||||||
|
await s.flush()
|
||||||
|
|
||||||
|
def _snippet(name, locations, code="return 1"):
|
||||||
|
return Note(
|
||||||
|
user_id=user.id,
|
||||||
|
note_type=SNIPPET_NOTE_TYPE,
|
||||||
|
title=f"{name} — does a thing",
|
||||||
|
body=compose_body(code=code, language="python", locations=locations),
|
||||||
|
tags=["python", "snippet"],
|
||||||
|
data=compose_data(name=name, language="python", locations=locations),
|
||||||
|
)
|
||||||
|
|
||||||
|
nested = _snippet("nested", [_loc("Scribe", "frontend/src/lib/x.ts", "helper")])
|
||||||
|
sibling = _snippet("sibling", [_loc("Scribe", "frontend/srcmap.ts", "other")])
|
||||||
|
# Two locations, deliberately crossed: repo Scribe at src/a.py and repo
|
||||||
|
# Portal at src/b.py. repo=Scribe + path=src/b.py must NOT match it.
|
||||||
|
multi = _snippet(
|
||||||
|
"multi",
|
||||||
|
[_loc("Scribe", "src/a.py", "alpha"), _loc("Portal", "src/b.py", "beta")],
|
||||||
|
)
|
||||||
|
# No structured location at all — must never satisfy a location filter,
|
||||||
|
# and must not error the query either.
|
||||||
|
placeless = _snippet("placeless", [])
|
||||||
|
# A plain note (not a snippet) with no `data` — proves the jsonpath
|
||||||
|
# operator tolerates NULL rather than raising.
|
||||||
|
plain = Note(user_id=user.id, title="plain", body="no data here")
|
||||||
|
|
||||||
|
s.add_all([nested, sibling, multi, placeless, plain])
|
||||||
|
await s.commit()
|
||||||
|
ids = (
|
||||||
|
user.id,
|
||||||
|
{
|
||||||
|
"nested": nested.id,
|
||||||
|
"sibling": sibling.id,
|
||||||
|
"multi": multi.id,
|
||||||
|
"placeless": placeless.id,
|
||||||
|
"plain": plain.id,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
yield ids
|
||||||
|
user_id = ids[0]
|
||||||
|
async with async_session() as s:
|
||||||
|
await s.execute(delete(Note).where(Note.user_id == user_id))
|
||||||
|
await s.execute(delete(User).where(User.id == user_id))
|
||||||
|
await s.commit()
|
||||||
|
|
||||||
|
|
||||||
|
async def _ids_for(user_id: int, **parts) -> set[int]:
|
||||||
|
items, total = await list_snippets(user_id, **parts)
|
||||||
|
assert total == len(items), "total must match the filtered page, not the whole list"
|
||||||
|
return {i["id"] for i in items}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_repo_filter_finds_every_snippet_recorded_in_it(seeded):
|
||||||
|
user_id, ids = seeded
|
||||||
|
found = await _ids_for(user_id, repo="Scribe")
|
||||||
|
assert found == {ids["nested"], ids["sibling"], ids["multi"]}
|
||||||
|
assert ids["placeless"] not in found
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_exact_path_finds_only_that_file(seeded):
|
||||||
|
user_id, ids = seeded
|
||||||
|
assert await _ids_for(user_id, path="frontend/src/lib/x.ts") == {ids["nested"]}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_path_prefix_reaches_into_subdirectories(seeded):
|
||||||
|
user_id, ids = seeded
|
||||||
|
# ...and stops at the directory boundary: srcmap.ts is a sibling, not a child.
|
||||||
|
assert await _ids_for(user_id, path="frontend/src") == {ids["nested"]}
|
||||||
|
assert await _ids_for(user_id, path="frontend/src/") == {ids["nested"]}
|
||||||
|
assert await _ids_for(user_id, path="frontend") == {ids["nested"], ids["sibling"]}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_parts_must_match_within_one_location(seeded):
|
||||||
|
user_id, ids = seeded
|
||||||
|
assert await _ids_for(user_id, repo="Scribe", path="src/a.py") == {ids["multi"]}
|
||||||
|
# Same snippet, but repo and path belong to two different entries.
|
||||||
|
assert await _ids_for(user_id, repo="Scribe", path="src/b.py") == set()
|
||||||
|
assert await _ids_for(user_id, repo="Portal", path="src/b.py") == {ids["multi"]}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_symbol_filter_and_an_unrecorded_location(seeded):
|
||||||
|
user_id, ids = seeded
|
||||||
|
assert await _ids_for(user_id, symbol="helper") == {ids["nested"]}
|
||||||
|
assert await _ids_for(user_id, repo="NoSuchRepo") == set()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_no_location_filter_still_lists_everything(seeded):
|
||||||
|
"""The filter is opt-in — a plain browse must not lose the placeless snippet."""
|
||||||
|
found = await _ids_for(user_id=seeded[0])
|
||||||
|
ids = seeded[1]
|
||||||
|
assert {ids["nested"], ids["sibling"], ids["multi"], ids["placeless"]} <= found
|
||||||
|
assert ids["plain"] not in found # not a snippet
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_keyword_arm_applies_the_location_filter(seeded):
|
||||||
|
"""Search AND place compose. The embedder is stubbed out so this isolates the
|
||||||
|
keyword (ILIKE) arm, which is the second place the SQL clause has to appear."""
|
||||||
|
user_id, ids = seeded
|
||||||
|
with patch(
|
||||||
|
"scribe.services.embeddings.semantic_search_notes",
|
||||||
|
AsyncMock(return_value=[]),
|
||||||
|
):
|
||||||
|
assert await _ids_for(user_id, q="nested", repo="Scribe") == {ids["nested"]}
|
||||||
|
# Matches the query but not the place.
|
||||||
|
assert await _ids_for(user_id, q="nested", repo="Portal") == set()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_semantic_arm_applies_the_location_filter(seeded):
|
||||||
|
"""The semantic arm holds its candidates in memory, so it filters in Python.
|
||||||
|
A query that matches no title or body leaves only that arm in play."""
|
||||||
|
user_id, ids = seeded
|
||||||
|
async with async_session() as s:
|
||||||
|
rows = list(
|
||||||
|
(
|
||||||
|
await s.execute(
|
||||||
|
select(Note)
|
||||||
|
.where(Note.user_id == user_id)
|
||||||
|
.where(Note.note_type == SNIPPET_NOTE_TYPE)
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.scalars()
|
||||||
|
.all()
|
||||||
|
)
|
||||||
|
candidates = [(0.9, n) for n in rows]
|
||||||
|
|
||||||
|
with patch(
|
||||||
|
"scribe.services.embeddings.semantic_search_notes",
|
||||||
|
AsyncMock(return_value=candidates),
|
||||||
|
):
|
||||||
|
# No keyword can match this, so every hit below came from the stub.
|
||||||
|
assert await _ids_for(user_id, q="zzzznomatch") == {
|
||||||
|
ids["nested"], ids["sibling"], ids["multi"], ids["placeless"],
|
||||||
|
}
|
||||||
|
assert await _ids_for(user_id, q="zzzznomatch", path="frontend/src") == {
|
||||||
|
ids["nested"],
|
||||||
|
}
|
||||||
|
assert await _ids_for(user_id, q="zzzznomatch", repo="Scribe", path="src/b.py") == set()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_sql_and_python_dialects_agree_on_the_same_rows(seeded):
|
||||||
|
"""The drift guard: whatever the SQL arm returns, the Python arm must too."""
|
||||||
|
user_id, _ids = seeded
|
||||||
|
async with async_session() as s:
|
||||||
|
rows = list(
|
||||||
|
(
|
||||||
|
await s.execute(
|
||||||
|
select(Note)
|
||||||
|
.where(Note.user_id == user_id)
|
||||||
|
.where(Note.note_type == SNIPPET_NOTE_TYPE)
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.scalars()
|
||||||
|
.all()
|
||||||
|
)
|
||||||
|
by_id = {n.id: n.data for n in rows}
|
||||||
|
|
||||||
|
cases = [
|
||||||
|
{"repo": "Scribe"},
|
||||||
|
{"path": "frontend/src"},
|
||||||
|
{"path": "frontend/src/lib/x.ts"},
|
||||||
|
{"repo": "Scribe", "path": "src/a.py"},
|
||||||
|
{"repo": "Scribe", "path": "src/b.py"},
|
||||||
|
{"symbol": "beta"},
|
||||||
|
{"repo": "Scribe", "path": "frontend", "symbol": "helper"},
|
||||||
|
]
|
||||||
|
for case in cases:
|
||||||
|
sql_ids = await _ids_for(user_id, **case)
|
||||||
|
parts = location_parts(**case)
|
||||||
|
python_ids = {nid for nid, data in by_id.items() if location_matches(data, parts)}
|
||||||
|
assert sql_ids == python_ids, f"dialects disagree on {case}"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_backfill_makes_a_pre_0070_snippet_findable_by_location(seeded):
|
||||||
|
"""A snippet stored before migration 0070 has no `data`, so the location query
|
||||||
|
can't see it. Unfilled, it reads as "nothing recorded here" — the failure the
|
||||||
|
backfill exists to prevent."""
|
||||||
|
user_id, _ids = seeded
|
||||||
|
locations = [_loc("Legacy", "old/path/y.py", "legacy_helper")]
|
||||||
|
async with async_session() as s:
|
||||||
|
old = Note(
|
||||||
|
user_id=user_id,
|
||||||
|
note_type=SNIPPET_NOTE_TYPE,
|
||||||
|
title="legacy — recorded before the data column existed",
|
||||||
|
body=compose_body(code="return 2", language="python", locations=locations),
|
||||||
|
tags=["python", "snippet"],
|
||||||
|
data=None,
|
||||||
|
)
|
||||||
|
s.add(old)
|
||||||
|
await s.commit()
|
||||||
|
old_id = old.id
|
||||||
|
|
||||||
|
assert await _ids_for(user_id, repo="Legacy") == set()
|
||||||
|
|
||||||
|
filled = await backfill_snippet_data()
|
||||||
|
assert filled >= 1
|
||||||
|
|
||||||
|
assert await _ids_for(user_id, repo="Legacy") == {old_id}
|
||||||
|
assert await _ids_for(user_id, path="old/path") == {old_id}
|
||||||
|
|
||||||
|
# The mirror agrees with the body it was derived from, and the body is untouched.
|
||||||
|
async with async_session() as s:
|
||||||
|
row = (await s.execute(select(Note).where(Note.id == old_id))).scalar_one()
|
||||||
|
assert row.data["locations"] == locations
|
||||||
|
assert snippet_fields(row)["symbol"] == "legacy_helper"
|
||||||
|
assert "old/path/y.py" in row.body
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_backfill_is_idempotent_and_settles_a_fieldless_snippet(seeded):
|
||||||
|
"""Second run fills nothing, and a snippet with no structured fields lands at
|
||||||
|
`{}` rather than staying NULL and being rescanned on every boot."""
|
||||||
|
user_id, _ids = seeded
|
||||||
|
async with async_session() as s:
|
||||||
|
bare = Note(
|
||||||
|
user_id=user_id,
|
||||||
|
note_type=SNIPPET_NOTE_TYPE,
|
||||||
|
title="",
|
||||||
|
body="",
|
||||||
|
tags=[],
|
||||||
|
data=None,
|
||||||
|
)
|
||||||
|
s.add(bare)
|
||||||
|
await s.commit()
|
||||||
|
bare_id = bare.id
|
||||||
|
|
||||||
|
await backfill_snippet_data()
|
||||||
|
assert await backfill_snippet_data() == 0
|
||||||
|
|
||||||
|
async with async_session() as s:
|
||||||
|
row = (await s.execute(select(Note).where(Note.id == bare_id))).scalar_one()
|
||||||
|
assert row.data == {}
|
||||||
@@ -72,6 +72,19 @@ def test_project_scoping_reaches_every_caller():
|
|||||||
assert "project_id" in inspect.getsource(routes.list_snippets_route)
|
assert "project_id" in inspect.getsource(routes.list_snippets_route)
|
||||||
|
|
||||||
|
|
||||||
|
def test_location_lookup_reaches_every_caller():
|
||||||
|
"""The reverse lookup — "what already lives here?" — has to be askable from
|
||||||
|
both surfaces, or an agent and a human get different answers about the same
|
||||||
|
file (rule #33)."""
|
||||||
|
from scribe.mcp.tools import snippets as tools
|
||||||
|
from scribe.routes import snippets as routes
|
||||||
|
from scribe.services import snippets as svc
|
||||||
|
for key in ("repo", "path", "symbol"):
|
||||||
|
assert key in inspect.signature(svc.list_snippets).parameters
|
||||||
|
assert key in inspect.signature(tools.list_snippets).parameters
|
||||||
|
assert f'request.args.get("{key}"' in inspect.getsource(routes)
|
||||||
|
|
||||||
|
|
||||||
def test_update_field_map_matches_service_kwargs():
|
def test_update_field_map_matches_service_kwargs():
|
||||||
"""Every field the PATCH route forwards must be a real update_snippet kwarg
|
"""Every field the PATCH route forwards must be a real update_snippet kwarg
|
||||||
(rule #33 interface-contract parity)."""
|
(rule #33 interface-contract parity)."""
|
||||||
|
|||||||
@@ -0,0 +1,205 @@
|
|||||||
|
"""Tests for the snippet location reverse lookup (#2083).
|
||||||
|
|
||||||
|
Covers the pure half of the predicate — `location_parts`, `location_matches`,
|
||||||
|
`location_jsonpath` — plus the parameter plumbing across the three surfaces
|
||||||
|
(service / REST route / MCP tool), which rule #33 requires to agree on names and
|
||||||
|
defaults.
|
||||||
|
|
||||||
|
The SQL half can only be proved against real Postgres — see
|
||||||
|
tests/test_integration_snippet_locations.py, which also pins the two dialects to
|
||||||
|
the same answers on the same rows.
|
||||||
|
"""
|
||||||
|
import inspect
|
||||||
|
import json
|
||||||
|
from unittest.mock import AsyncMock, patch
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from scribe.services.knowledge import (
|
||||||
|
LOCATION_KEYS,
|
||||||
|
location_jsonpath,
|
||||||
|
location_matches,
|
||||||
|
location_parts,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _data(*locations: dict) -> dict:
|
||||||
|
return {"locations": list(locations)}
|
||||||
|
|
||||||
|
|
||||||
|
# --- location_parts -----------------------------------------------------------
|
||||||
|
|
||||||
|
def test_location_parts_keeps_only_what_was_asked_for():
|
||||||
|
assert location_parts() == {}
|
||||||
|
assert location_parts(repo="Scribe") == {"repo": "Scribe"}
|
||||||
|
assert location_parts(repo=" Scribe ", path=" src/x.py ") == {
|
||||||
|
"repo": "Scribe", "path": "src/x.py",
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def test_location_parts_treats_whitespace_as_absent():
|
||||||
|
"""An empty query-string arg must not filter the list down to nothing."""
|
||||||
|
assert location_parts(repo=" ", path="", symbol=None or "") == {}
|
||||||
|
|
||||||
|
|
||||||
|
def test_location_parts_covers_every_location_key():
|
||||||
|
assert set(location_parts(repo="a", path="b", symbol="c")) == set(LOCATION_KEYS)
|
||||||
|
|
||||||
|
|
||||||
|
# --- location_matches ---------------------------------------------------------
|
||||||
|
|
||||||
|
def test_no_parts_matches_everything():
|
||||||
|
assert location_matches(None, {}) is True
|
||||||
|
assert location_matches(_data({"repo": "x", "path": "y", "symbol": "z"}), {}) is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_matches_exact_repo_path_and_symbol():
|
||||||
|
data = _data({"repo": "Scribe", "path": "src/scribe/x.py", "symbol": "helper"})
|
||||||
|
assert location_matches(data, {"repo": "Scribe"})
|
||||||
|
assert location_matches(data, {"path": "src/scribe/x.py"})
|
||||||
|
assert location_matches(data, {"symbol": "helper"})
|
||||||
|
assert location_matches(data, {"repo": "Scribe", "symbol": "helper"})
|
||||||
|
|
||||||
|
|
||||||
|
def test_path_matches_as_a_directory_prefix():
|
||||||
|
data = _data({"repo": "Scribe", "path": "frontend/src/lib/x.ts", "symbol": ""})
|
||||||
|
assert location_matches(data, {"path": "frontend/src"})
|
||||||
|
assert location_matches(data, {"path": "frontend/src/"})
|
||||||
|
assert location_matches(data, {"path": "frontend/src/lib/x.ts"})
|
||||||
|
|
||||||
|
|
||||||
|
def test_path_prefix_stops_at_a_directory_boundary():
|
||||||
|
"""`frontend/src` must not match `frontend/srcmap.ts` — a sibling, not a child."""
|
||||||
|
data = _data({"repo": "Scribe", "path": "frontend/srcmap.ts", "symbol": ""})
|
||||||
|
assert not location_matches(data, {"path": "frontend/src"})
|
||||||
|
|
||||||
|
|
||||||
|
def test_parts_must_all_match_the_same_location():
|
||||||
|
"""Repo A in one entry and path B in another is not "recorded at A/B"."""
|
||||||
|
data = _data(
|
||||||
|
{"repo": "Scribe", "path": "src/a.py", "symbol": ""},
|
||||||
|
{"repo": "Portal", "path": "src/b.py", "symbol": ""},
|
||||||
|
)
|
||||||
|
assert location_matches(data, {"repo": "Scribe", "path": "src/a.py"})
|
||||||
|
assert not location_matches(data, {"repo": "Scribe", "path": "src/b.py"})
|
||||||
|
|
||||||
|
|
||||||
|
def test_missing_or_empty_data_never_matches():
|
||||||
|
assert not location_matches(None, {"repo": "Scribe"})
|
||||||
|
assert not location_matches({}, {"repo": "Scribe"})
|
||||||
|
assert not location_matches({"locations": []}, {"repo": "Scribe"})
|
||||||
|
assert not location_matches({"name": "x"}, {"repo": "Scribe"})
|
||||||
|
|
||||||
|
|
||||||
|
def test_blank_recorded_part_does_not_match_a_requested_one():
|
||||||
|
data = _data({"repo": "Scribe", "path": "", "symbol": ""})
|
||||||
|
assert not location_matches(data, {"repo": "Scribe", "symbol": "helper"})
|
||||||
|
|
||||||
|
|
||||||
|
# --- location_jsonpath --------------------------------------------------------
|
||||||
|
|
||||||
|
def test_jsonpath_filters_within_one_locations_entry():
|
||||||
|
expr = location_jsonpath({"repo": "Scribe", "symbol": "helper"})
|
||||||
|
assert expr.startswith("$.locations[*] ? (")
|
||||||
|
assert '@.repo == "Scribe"' in expr
|
||||||
|
assert '@.symbol == "helper"' in expr
|
||||||
|
assert " && " in expr
|
||||||
|
|
||||||
|
|
||||||
|
def test_jsonpath_path_asks_for_exact_or_prefix():
|
||||||
|
expr = location_jsonpath({"path": "frontend/src"})
|
||||||
|
assert '@.path == "frontend/src"' in expr
|
||||||
|
assert '@.path starts with "frontend/src/"' in expr
|
||||||
|
assert " || " in expr
|
||||||
|
|
||||||
|
|
||||||
|
def test_jsonpath_does_not_double_the_prefix_slash():
|
||||||
|
assert 'starts with "frontend/src/"' in location_jsonpath({"path": "frontend/src/"})
|
||||||
|
|
||||||
|
|
||||||
|
def test_jsonpath_quotes_values_as_json_literals():
|
||||||
|
"""A quote in a repo name must stay inside the literal, not end it."""
|
||||||
|
nasty = 'we"ird'
|
||||||
|
expr = location_jsonpath({"repo": nasty})
|
||||||
|
assert json.dumps(nasty) in expr
|
||||||
|
assert '\\"' in expr
|
||||||
|
|
||||||
|
|
||||||
|
def test_jsonpath_emits_parts_in_a_fixed_key_order():
|
||||||
|
"""Stable text keeps the query plan cacheable and the tests readable."""
|
||||||
|
a = location_jsonpath({"symbol": "s", "repo": "r", "path": "p"})
|
||||||
|
b = location_jsonpath({"repo": "r", "path": "p", "symbol": "s"})
|
||||||
|
assert a == b
|
||||||
|
|
||||||
|
|
||||||
|
# --- the SQL dialect renders (shape only; behaviour is the integration test) ---
|
||||||
|
|
||||||
|
def test_location_clause_renders_a_jsonpath_containment():
|
||||||
|
"""Catches an API slip in the fast lane rather than waiting for Postgres —
|
||||||
|
the clause has to compile to `data @? :jsonpath` against the pg dialect."""
|
||||||
|
from sqlalchemy.dialects import postgresql
|
||||||
|
|
||||||
|
from scribe.services.knowledge import _location_clause
|
||||||
|
|
||||||
|
sql = str(_location_clause({"repo": "Scribe"}).compile(dialect=postgresql.dialect()))
|
||||||
|
assert "@?" in sql
|
||||||
|
assert "data" in sql
|
||||||
|
|
||||||
|
|
||||||
|
# --- surface plumbing (rule #33) ---------------------------------------------
|
||||||
|
|
||||||
|
def test_agent_and_service_surfaces_take_the_same_location_params():
|
||||||
|
from scribe.mcp.tools.snippets import list_snippets as mcp_list
|
||||||
|
from scribe.services.snippets import list_snippets as svc_list
|
||||||
|
|
||||||
|
for fn in (mcp_list, svc_list):
|
||||||
|
params = inspect.signature(fn).parameters
|
||||||
|
for key in LOCATION_KEYS:
|
||||||
|
assert key in params, f"{fn.__qualname__} is missing {key}"
|
||||||
|
assert params[key].default == "", f"{fn.__qualname__}.{key} default drifted"
|
||||||
|
|
||||||
|
|
||||||
|
def test_route_forwards_the_location_args_by_name():
|
||||||
|
"""The REST surface is the third caller; it must use the same arg names."""
|
||||||
|
from scribe.routes import snippets as routes
|
||||||
|
src = inspect.getsource(routes)
|
||||||
|
for key in LOCATION_KEYS:
|
||||||
|
assert f'request.args.get("{key}"' in src
|
||||||
|
assert f"{key}={key}" in src
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_mcp_tool_passes_locations_through_to_the_service():
|
||||||
|
with patch("scribe.services.snippets.list_snippets",
|
||||||
|
new=AsyncMock(return_value=([], 0))) as m, \
|
||||||
|
patch("scribe.services.access.label_shared_items",
|
||||||
|
new=AsyncMock(return_value=[])):
|
||||||
|
from scribe.mcp.tools.snippets import list_snippets
|
||||||
|
await list_snippets(repo="Scribe", path="src/scribe", symbol="helper")
|
||||||
|
kwargs = m.await_args.kwargs
|
||||||
|
assert kwargs["repo"] == "Scribe"
|
||||||
|
assert kwargs["path"] == "src/scribe"
|
||||||
|
assert kwargs["symbol"] == "helper"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_service_sends_no_location_filter_when_none_was_asked_for():
|
||||||
|
"""A plain list must not carry an empty filter — that would exclude every
|
||||||
|
row whose `data` has no locations at all."""
|
||||||
|
with patch("scribe.services.knowledge.query_knowledge",
|
||||||
|
new=AsyncMock(return_value=([], 0))) as m:
|
||||||
|
from scribe.services.snippets import list_snippets
|
||||||
|
await list_snippets(1)
|
||||||
|
assert m.await_args.kwargs["locations"] is None
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_service_builds_the_filter_from_the_parts_given():
|
||||||
|
with patch("scribe.services.knowledge.query_knowledge",
|
||||||
|
new=AsyncMock(return_value=([], 0))) as m:
|
||||||
|
from scribe.services.snippets import list_snippets
|
||||||
|
await list_snippets(1, q="debounce", repo="Scribe", path=" src/ ")
|
||||||
|
kwargs = m.await_args.kwargs
|
||||||
|
assert kwargs["locations"] == {"repo": "Scribe", "path": "src/"}
|
||||||
|
# Search and place compose — the location filter must not drop the query.
|
||||||
|
assert kwargs["q"] == "debounce"
|
||||||
Reference in New Issue
Block a user