From 75ec3b818b15a30eededf7852f87cfc744aa4055 Mon Sep 17 00:00:00 2001 From: phernandez Date: Mon, 31 Aug 2026 19:15:28 -0500 Subject: [PATCH 01/12] feat(mcp): add metadata predicates and field projection to POSIX find MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit find(1) gains predicates and a SELECT clause: --meta status=active, confidence>0.6, review.approved=true parse onto the existing search metadata_filters grammar (strict subset, verified test-side against the server-side parser — unsupported operators fail fast naming the set), and --fields returns the requested frontmatter per hit so callers stop reading N notes to check one field. Without --meta, find stays the byte-identical directory listing. Closes the measured 2x metadata gap from A/B run 6, where POSIX agents brute-force-read notes that rich answered in one metadata_filters call. A parity test pins that find --meta and search_notes with equivalent filters return the same note set through the real ASGI stack. Field projection hydrates entities concurrently (semaphore-bounded gather) because the search index carries only note_type, not full frontmatter; a batch entity read or a wider search projection would remove the round trips entirely — follow-up, not a blocker. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/cli/commands/posix.py | 99 +++- src/basic_memory/man/bm.1 | 3 +- src/basic_memory/man/man1/find(1).md | 81 +++- src/basic_memory/mcp/tools/posix_tools.py | 367 ++++++++++++++- tests/cli/test_cli_posix_verbs.py | 186 ++++++++ tests/cli/test_man_command.py | 38 ++ tests/mcp/test_tool_contracts.py | 12 +- tests/mcp/test_tool_posix.py | 531 +++++++++++++++++++++- 8 files changed, 1296 insertions(+), 21 deletions(-) diff --git a/src/basic_memory/cli/commands/posix.py b/src/basic_memory/cli/commands/posix.py index 8f38634d4..2679b0295 100644 --- a/src/basic_memory/cli/commands/posix.py +++ b/src/basic_memory/cli/commands/posix.py @@ -23,6 +23,7 @@ on ``--json`` or when piped, undecorated text with ``--plain``. """ +import json import sys from dataclasses import dataclass, field from typing import Annotated, Any, Optional @@ -265,6 +266,63 @@ def _plain_find(result: dict[str, Any]) -> None: print(str(node.get("file_path") or node.get("directory_path") or "")) +# --- find --meta --fields rendering --- +# Metadata predicates flip find's payload to the search response shape; without +# --fields the shared search renderers (grep's) apply, with --fields these two +# small renderers add the projected columns so the shared ones stay untouched. + + +def _search_page_summary(result: dict[str, Any]) -> str: + """Describe a search results page without inventing a final page.""" + summary = f"page {result.get('current_page', 1)} • total {result.get('total', 0)}" + if result.get("has_more") is True: + summary += " • more available (--page)" + return summary + + +def _field_cell(value: Any) -> str: + """Render one projected field value: strings bare, null empty, the rest compact JSON.""" + if value is None: + return "" + if isinstance(value, str): + return value + return json.dumps(value, separators=(",", ":")) + + +def _display_find_fields( + result: dict[str, Any], path: str, meta: list[str], fields: list[str] +) -> None: + """Render metadata hits with their projected fields as a Rich table.""" + rows: list[dict[str, Any]] = list(result.get("results", [])) + title = ( + f"find [bold cyan]{markup_escape(path)}[/bold cyan]" + f" [dim]--meta {markup_escape(' AND '.join(meta))}[/dim]" + ) + subtitle = _search_page_summary(result) + + if not rows: + console.print(Panel(Text("No matches.", style="dim"), title=title, subtitle=subtitle)) + return + + table = Table(show_header=True, header_style="bold", expand=False) + table.add_column("Path", style="bold cyan") + for field_name in fields: + table.add_column(markup_escape(field_name)) + for row in rows: + projected = row.get("fields") or {} + cells = [markup_escape(str(row.get("file_path") or ""))] + cells.extend(markup_escape(_field_cell(projected.get(field_name))) for field_name in fields) + table.add_row(*cells) + console.print(Panel(table, title=title, subtitle=subtitle, expand=False)) + + +def _plain_find_fields(result: dict[str, Any]) -> None: + """Render metadata hits as file-pathcompact-JSON-fields lines.""" + for row in result.get("results", []): + fields_json = json.dumps(row.get("fields") or {}, separators=(",", ":")) + print(f"{row.get('file_path', '')}\t{fields_json}") + + # --- tail rendering --- # tail's row shape ({type, title, permalink, file_path, created_at}) differs # from recent-activity's payload, so it gets its own small renderers rather @@ -681,6 +739,26 @@ def find( page_size: Annotated[ int, typer.Option("--page-size", help="Nodes per page") ] = DEFAULT_DIRECTORY_PAGE_SIZE, + meta: Annotated[ + Optional[list[str]], + typer.Option( + "--meta", + help=( + "Metadata predicate, repeatable: 'status=active', 'confidence>0.6', " + "'priority in high,critical', 'tags has security', 'score between 0.3,0.8'" + ), + ), + ] = None, + fields: Annotated[ + Optional[str], + typer.Option( + "--fields", + help=( + 'Comma-separated frontmatter fields to show per hit, e.g. "title,priority" ' + "(requires --meta)" + ), + ), + ] = None, json_output: JsonOption = False, plain: PlainOption = False, project: ProjectOption = None, @@ -688,13 +766,14 @@ def find( local: LocalOption = False, cloud: CloudOption = False, ) -> None: - """Recursively list files under a directory, optionally filtered by name glob. + """Recursively list files by name glob, or query notes by frontmatter metadata. Examples: bm find --name "*.md" bm find /specs --depth 3 bm find /notes --name "auth*" --plain + bm find --meta "status=active" --meta "confidence>0.6" --fields title,priority """ # Deferred: loading the MCP tool stack at module import slows CLI startup (#886). from basic_memory.mcp.tools import find as mcp_find @@ -703,6 +782,9 @@ def find( try: validate_routing_flags(local, cloud) _validate_output_flags(json_output, plain) + # The CLI only splits the comma form; validation (non-empty names, + # requires --meta) lives in the shared tool layer. + field_list = [item.strip() for item in fields.split(",")] if fields is not None else None with force_routing(local=local, cloud=cloud): result = run_with_cleanup( @@ -712,6 +794,8 @@ def find( depth=depth, page=page, page_size=page_size, + meta=meta, + fields=field_list, project=project, project_id=project_id, ) @@ -719,6 +803,19 @@ def find( mode = _resolve_output_mode(json_output, plain) if mode == "json": _print_json(result) + elif "results" in result: + # --meta flips the payload to the search response shape: projected + # fields get the dedicated renderers, otherwise grep's search + # renderers apply. + if field_list: + if mode == "plain": + _plain_find_fields(result) + else: + _display_find_fields(result, path, meta or [], field_list) + elif mode == "plain": + _plain_search_results(result, query=" AND ".join(meta or [])) + else: + _display_search_results(result, query=" AND ".join(meta or [])) elif mode == "plain": _plain_find(result) else: diff --git a/src/basic_memory/man/bm.1 b/src/basic_memory/man/bm.1 index 120d5493a..8abc6cebd 100644 --- a/src/basic_memory/man/bm.1 +++ b/src/basic_memory/man/bm.1 @@ -42,7 +42,8 @@ Search note content (semantic by default; \-F for literal matching). List one directory level of a project. .TP .B bm find -Recursively list files matching a name glob. +Recursively list files by name glob, or query notes by frontmatter +metadata (\-\-meta). .TP .B bm tree Show a directory hierarchy. diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index c6e795ab3..95425afec 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -3,7 +3,7 @@ title: find(1) type: manpage section: 1 name: find -summary: recursively list files matching a name glob +summary: recursively list files, or query notes by frontmatter metadata generated: hand --- @@ -11,7 +11,7 @@ generated: hand ## NAME -**find** — recursively list files matching a name glob +**find** — recursively list files, or query notes by frontmatter metadata ## SYNOPSIS @@ -19,21 +19,76 @@ generated: hand bm find [PATH] [--name GLOB] [--depth N] [--page N] [--page-size N] [--json | --plain] [--project NAME | --project-id UUID] [--local | --cloud] + +bm find [PATH] --meta PREDICATE [--meta PREDICATE ...] [--fields LIST] + [--page N] [--page-size N] [--json | --plain] + [--project NAME | --project-id UUID] [--local | --cloud] ``` ## DESCRIPTION -Recursively lists files under a directory (default: the project root), -optionally filtered by a file-name glob. Depth is bounded 1-10 by the -directory API. On a TTY results render as a table; `--plain` prints one -path per line, find(1) style; `--json` (or piped output) emits the listing -with pagination and totals. +Two modes, chosen by `--meta`. + +Without `--meta`, find recursively lists files under a directory (default: +the project root), optionally filtered by a file-name glob. Depth is bounded +1-10 by the directory API. On a TTY results render as a table; `--plain` +prints one path per line, find(1) style; `--json` (or piped output) emits the +listing with pagination and totals. + +With `--meta`, find queries notes by their frontmatter instead: every +predicate must hold, and `PATH` still scopes the results — server-side, by +permalink prefix, so the totals are exact and every page is reachable. The +payload becomes the search response shape (the same one `bm grep` returns), +not the directory listing. Non-markdown files carry no frontmatter and are +never metadata hits. + +`--fields` is the SELECT to the predicates' WHERE: it adds a `fields` object +to each hit carrying the named frontmatter values, so a filtered set can be +tabulated without reading every note. A field a hit does not carry renders as +null; the row is never dropped. + +`--name` and `--depth` are refused alongside `--meta`. The metadata query +matches slugified permalinks, where a filename glob has no faithful +translation, and its subtree scope is all-or-nothing, where a depth bound is +not expressible — refusing beats silently ignoring either and misreporting +the match set. Scope with `PATH` instead. `--fields` without `--meta` is +refused for the same honesty: without predicates there is nothing to project. + +## PREDICATE GRAMMAR + +One predicate per `--meta`, one predicate per key; repeated flags AND +together. A repeated key is an error, not last-wins — use `between` for a +range. + +``` +status=active equality +confidence>0.6 comparison: > >= < <= +priority in high,critical any of the listed values +tags has security,oauth array contains ALL listed values +score between 0.3,0.8 inclusive range +``` + +Values are JSON-scalar inferred: `true`/`false`/`null` and numbers become +booleans, null, and numbers. Quote a token to force the literal string — +`status="true"` matches the four-character string. Quoting also protects a +comma inside a list element: `label in "a,b",c` matches `a,b` or `c`. + +Keys accept dot-paths into nested frontmatter (`review.approved`), and +`note_type` is accepted as a spelling of the frontmatter `type` key, matching +`search-notes(3)`. Any other operator (`!=` among them — the search API has +no not-equals) fails fast, naming the supported set. ## OPTIONS -- **--name** — file-name glob, e.g. `"*.md"`; omitted matches everything -- **--depth** — recursion depth, 1-10 (default 10) -- **--page, --page-size** — node pagination (defaults 1 and 10) +- **--name** — file-name glob, e.g. `"*.md"`; omitted matches everything. + Cannot combine with `--meta` +- **--depth** — recursion depth, 1-10 (default 10). A non-default depth + cannot combine with `--meta` +- **--meta** — frontmatter predicate, repeatable; see PREDICATE GRAMMAR. + Switches the payload to the search response shape +- **--fields** — comma-separated frontmatter fields to show per hit, e.g. + `"title,priority"`; dot-paths allowed. Requires `--meta` +- **--page, --page-size** — pagination (defaults 1 and 10) ## EXAMPLES @@ -41,10 +96,16 @@ with pagination and totals. bm find --name "*.md" bm find /specs --depth 3 bm find /notes --name "auth*" --plain +bm find --meta "status=active" +bm find /specs --meta "status=active" --meta "confidence>0.6" +bm find --meta "tags has security,oauth" --fields title,priority +bm find --meta "status=active" --fields title --plain ``` ## SEE ALSO - see_also [[ls(1)]] - see_also [[tree(1)]] +- see_also [[grep(1)]] - see_also [[list-directory(3)]] +- see_also [[search-notes(3)]] diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index d905a1361..39abd0179 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -28,11 +28,15 @@ ``project`` param names the manual project, not a data project. """ +import asyncio +import json import os -from typing import Any, Optional +import re +from typing import Annotated, Any, Optional from fastmcp import Context from fastmcp.exceptions import ToolError +from pydantic import BeforeValidator from basic_memory.config import ConfigManager from basic_memory.man import bundled_pages, find_page, parse_page_ref, render_index @@ -44,6 +48,7 @@ get_project_client, resolve_project_path_route, ) +from basic_memory.mcp.project_context_identifiers import canonical_memory_path_for_active_route from basic_memory.mcp.server import POSIX_TOOLS_TAG, mcp, set_posix_tools_visibility from basic_memory.schemas.directory import ( DEFAULT_DIRECTORY_PAGE_SIZE, @@ -52,7 +57,7 @@ DirectoryNode, ) from basic_memory.schemas.search import SearchItemType, SearchQuery, SearchRetrievalMode -from basic_memory.utils import generate_permalink +from basic_memory.utils import coerce_list, generate_permalink # --- Round-trip coherence --- # A path a routed verb returns must be a path the resolver accepts. When a call @@ -144,6 +149,15 @@ def qualify_listing_paths(payload: dict[str, Any], route: ProjectPathRoute) -> d # recent_activity's page-size cap; tail's `lines` maps onto it. _MAX_TAIL_LINES = 100 +# In-flight entity reads while projecting `find --fields`. The knowledge API has +# no bulk entity read, so a full page costs page_size GETs; this bounds how many +# are open at once — enough to hide per-request latency on a cloud-routed +# project, small enough not to flood the API with one tool call's fan-out. +# Deliberately not max_tokens-sliced: a slice param 404s on an entity with no +# markdown content (knowledge_router._apply_note_slice), which would turn a +# projected row into a failed find. +_FIELD_PROJECTION_CONCURRENCY = 8 + @mcp.tool( title="Cat", @@ -525,9 +539,147 @@ async def find_listing( return payload, routed_listing_root(path, route) +# --- find metadata predicates --- +# find's `meta` strings translate onto the search API's metadata_filters dict — +# the exact grammar parse_metadata_filters supports (eq, $gt/$gte/$lt/$lte, $in, +# array-contains-all, $between), nothing more. Word ops need whitespace around +# them and symbol ops exclude the key character class, so exactly one regex can +# match any given predicate. Two-char symbols sit first in the alternation so +# ">=" never parses as ">" plus a value starting with "=". +_PREDICATE_WORD_RE = re.compile(r"^([A-Za-z0-9_.-]+)\s+(in|has|between)\s+(.+)$") +_PREDICATE_SYMBOL_RE = re.compile(r"^([A-Za-z0-9_.-]+)\s*(>=|<=|=|>|<)\s*(.*)$") +_SYMBOL_OPERATORS = {">": "$gt", ">=": "$gte", "<": "$lt", "<=": "$lte"} +_SUPPORTED_PREDICATE_OPS = "= > >= < <= in has between" +# Mirrors search_notes' alias: "note_type" (the entity model column) means the +# frontmatter "type" key, so the two surfaces accept the same spelling. +_METADATA_KEY_ALIASES = {"note_type": "type"} + + +def _predicate_scalar(token: str) -> Any: + """JSON-scalar-infer one predicate value token. + + "true"/"false"/"null"/numbers become bool/None/int/float so the produced + filters dict is byte-equal to what a rich search_notes caller passes as + JSON; a JSON-quoted token ('"true"') forces a literal string; anything that + is not a JSON scalar stays the raw string. + """ + text = token.strip() + try: + value = json.loads(text) + except json.JSONDecodeError: + return text + if value is None or isinstance(value, (bool, int, float, str)): + return value + return text + + +def _split_predicate_items(raw_value: str, predicate: str) -> list[str]: + """Split a list-op value on its top-level commas, refusing empty elements. + + A comma inside a JSON-quoted token belongs to the value, not to the list, so + the quoting escape hatch the scalar operators document works for `in`, `has` + and `between` too: 'label in "a,b",c' yields ['"a,b"', 'c'], which + _predicate_scalar then reads as the literal strings "a,b" and "c". Splitting + the raw string first would sever the quoted token into '"a' and 'b"' and + filter for values nothing carries — wrong, and silent. An unterminated quote + is a typo rather than a value, so it fails fast for the same reason. + """ + items: list[str] = [] + current: list[str] = [] + in_quotes = False + escaped = False + for char in raw_value: + current.append(char) + if escaped: + escaped = False + elif in_quotes and char == "\\": + escaped = True + elif char == '"': + in_quotes = not in_quotes + elif char == "," and not in_quotes: + current.pop() + items.append("".join(current)) + current = [] + if in_quotes: + raise ValueError( + f"find: predicate '{predicate}' has an unterminated quoted value; " + 'quote a list element as "text with, commas"' + ) + items.append("".join(current)) + stripped = [item.strip() for item in items] + if any(not item for item in stripped): + raise ValueError(f"find: predicate '{predicate}' has an empty list element") + return stripped + + +def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: + """Translate POSIX-style predicate strings into the search API metadata_filters dict. + + One predicate per string; predicates AND together. Exactly one predicate + per key — the API admits one operator per key, so a repeated key fails fast + instead of last-wins. Raises ValueError (surfaced to MCP callers as + ToolError) on any operator outside the supported set. + """ + filters: dict[str, Any] = {} + for predicate in predicates: + match = _PREDICATE_WORD_RE.match(predicate.strip()) or _PREDICATE_SYMBOL_RE.match( + predicate.strip() + ) + if match is None: + raise ValueError( + f"find: unsupported predicate operator in '{predicate}'; " + f"supported: {_SUPPORTED_PREDICATE_OPS}" + ) + raw_key, op, raw_value = match.groups() + key = _METADATA_KEY_ALIASES.get(raw_key, raw_key) + if key in filters: + raise ValueError( + f"find: duplicate predicate key '{key}' in '{predicate}'; " + "use 'between' for ranges (e.g. 'score between 0.3,0.8')" + ) + if op in ("in", "has", "between"): + items = [ + _predicate_scalar(item) for item in _split_predicate_items(raw_value, predicate) + ] + if op == "between" and len(items) != 2: + raise ValueError(f"find: 'between' needs exactly min,max in '{predicate}'") + filters[key] = ( + {"$in": items} if op == "in" else items if op == "has" else {"$between": items} + ) + else: + if not raw_value.strip(): + raise ValueError(f"find: predicate '{predicate}' has no value") + value = _predicate_scalar(raw_value) + filters[key] = value if op == "=" else {_SYMBOL_OPERATORS[op]: value} + return filters + + +def _project_metadata_fields( + entity_metadata: dict[str, Any] | None, fields: list[str] +) -> dict[str, Any]: + """Project requested frontmatter fields out of an entity's metadata. + + Field names are echoed verbatim as keys (dot-paths walk nested dicts). A + missing key or non-dict intermediate yields None — never a dropped row. + """ + projected: dict[str, Any] = {} + for field_name in fields: + value: Any = entity_metadata + for part in field_name.split("."): + if not isinstance(value, dict): + value = None + break + value = value.get(part) + projected[field_name] = value + return projected + + @mcp.tool( title="Find", - description="Recursively list files matching a name glob. Paths accept '/path'.", + description=( + 'Recursively list files by name glob or metadata predicates (e.g. "status=active"). ' + "Paths accept '/path'." + ), tags={POSIX_TOOLS_TAG, "navigation"}, annotations={ "title": "Find", @@ -542,27 +694,143 @@ async def find( depth: int = _MAX_FIND_DEPTH, page: int = 1, page_size: int = DEFAULT_DIRECTORY_PAGE_SIZE, + meta: Annotated[Optional[list[str]], BeforeValidator(coerce_list)] = None, + fields: Annotated[Optional[list[str]], BeforeValidator(coerce_list)] = None, project: Optional[str] = None, project_id: Optional[str] = None, context: Context | None = None, ) -> dict[str, Any]: - """Recursively list files under a directory, optionally filtered by name glob. + """Recursively list files by name glob, or query notes by frontmatter metadata. + + Without `meta`, this is a recursive directory listing. With `meta`, find + routes through the metadata search instead: predicates AND together and + `path` still scopes the results — server-side, by slugified-permalink + prefix, so non-markdown files (which have no permalink) are never metadata + hits. `name` and `depth` are refused alongside `meta`: the search API has + no filename-glob or depth-bound facility, and silently ignoring either + would misreport the match set. Args: path: Directory to start from (default: project root). '/path' - routes into that project. + routes into that project. With `meta`, scopes the search to this + subtree. name: File-name glob to match, e.g. "*.md". None matches everything. - depth: How many levels to recurse (1-10, default: 10). + Cannot combine with `meta` — scope with `path` instead. + depth: How many levels to recurse (1-10, default: 10). A non-default + depth cannot combine with `meta` (subtree scope is all-or-nothing). page: Page number (1-indexed). page_size: Nodes per page. + meta: Frontmatter metadata predicates, repeatable; every predicate must + hold. One predicate per string, one predicate per key, at least one + predicate (omit `meta` for the directory listing): + "status=active" equality + "confidence>0.6" comparison: > >= < <= + "priority in high,critical" any of the listed values + "tags has security,oauth" array contains ALL listed values + "score between 0.3,0.8" inclusive range + Values are JSON-scalar inferred ("true"/"false"/"null"/numbers + become booleans/None/numbers); quote a token to force a literal + string (e.g. 'status="true"'), including inside a list, where the + quotes also protect a comma ('label in "a,b",c' matches "a,b" or + "c"). Keys accept dot-paths ("review.approved"); "note_type" + aliases the frontmatter "type" key. Any other operator fails fast + naming the supported set. + fields: Frontmatter fields to return per hit (dot-paths allowed), e.g. + ["title", "priority"]. Requires `meta`. A field missing on a hit + renders as null — rows are never dropped. project: Project name. Optional - qualified paths route themselves; unqualified paths refuse when several projects are addressable. project_id: Project external_id (UUID); takes precedence over `project`. context: Optional FastMCP context. Returns: - The directory listing as JSON: nodes, pagination, and totals. + Without `meta`: the directory listing as JSON (nodes, pagination, + totals). With `meta`: the search response as JSON (results, pagination, + totals); each result carries a `fields` object when `fields` was + requested. """ + # Combination rules, before any I/O. The metadata search matches slugified + # permalinks, where a filename glob has no faithful translation, and its + # permalink-prefix scope is whole-subtree, so a depth bound is not + # expressible — refuse both rather than silently ignoring either. `fields` + # is the SELECT to the predicates' WHERE; without predicates the directory + # listing stays byte-identical to today. + if meta is not None: + # Trigger: 'meta' present but carrying no predicates. + # Why: an empty list parses to an empty filters dict, which is not None + # and would route into the metadata search with no predicate at + # all — an unfiltered project-wide match where the caller asked for + # a filtered set, and not the directory listing either. + # Outcome: refuse, exactly as 'fields' refuses an empty list. + if not meta: + raise ValueError( + "find: 'meta' must carry at least one predicate — omit 'meta' entirely " + "for the plain directory listing" + ) + if name is not None: + raise ValueError( + "find: 'name' cannot combine with 'meta' — the metadata search matches " + "slugified permalinks, not filenames; scope with 'path' instead" + ) + if depth != _MAX_FIND_DEPTH: + raise ValueError( + "find: 'depth' cannot combine with 'meta' — the metadata search scopes " + "by whole subtree; scope with 'path' instead" + ) + if fields is not None: + if meta is None: + raise ValueError( + "find: 'fields' requires 'meta' predicates — without predicates find " + "returns the plain directory listing" + ) + fields = [field_name.strip() for field_name in fields] + if not fields or any(not field_name for field_name in fields): + raise ValueError("find: 'fields' entries must be non-empty frontmatter field names") + metadata_filters = _parse_meta_predicates(meta) if meta is not None else None + + # Trigger: predicates are present, so this call queries metadata rather than + # walking directories. + # Why: the two arms call different project-scoped APIs, and each resolves the + # route exactly once — find_listing already owns validation and + # resolution for the listing arm, so branching before resolving keeps a + # routed call to one project-list round trip (which is why find_listing + # exists at all). + # Outcome: the metadata arm answers with search results; otherwise the + # directory listing comes back unchanged. + if metadata_filters is not None: + # Pagination is an argument check, so it refuses here with the rest of + # them, before any I/O. The listing arm inherits these same bounds from + # find_listing, which the metadata arm never reaches — stated once per + # arm rather than once for both, so neither can drift onto the other's + # error message. + if page < 1: + raise ValueError(f"page must be >= 1, got {page}") + if page_size < 1: + raise ValueError(f"page_size must be >= 1, got {page_size}") + if page_size > MAX_DIRECTORY_PAGE_SIZE: + raise ValueError(f"page_size must be <= {MAX_DIRECTORY_PAGE_SIZE}, got {page_size}") + + # The search API is project-scoped too, so cross-project find does not + # exist here either: an unqualified path in a multi-project config + # refuses, teaching the per-project '/path' form instead. + route = await resolve_project_path_route( + path, project=project, project_id=project_id, context=context + ) + return await _find_by_metadata( + route_project=route.project, + # route.path is the caller's input verbatim when no project prefix + # was recognized, so one strip covers both routed and raw spellings. + scope=route.path.strip("/"), + metadata_filters=metadata_filters, + fields=fields, + page=page, + page_size=page_size, + # The route's id, not the caller's raw param: it is what keeps a + # cloud mount bound to the workspace whose listing advertised it. + project_id=route.project_id, + context=context, + ) + listing, _ = await find_listing( path, name=name, @@ -576,6 +844,91 @@ async def find( return listing +async def _find_by_metadata( + *, + route_project: Optional[str], + scope: str, + metadata_filters: dict[str, Any], + fields: Optional[list[str]], + page: int, + page_size: int, + project_id: Optional[str], + context: Context | None, +) -> dict[str, Any]: + """find's metadata arm: one search call, plus per-hit field projection. + + The listing arm's counterpart to ``find_listing``; `find` has already bounded + the pagination and refused `name` and `depth` against `meta`. + + The path scope composes server-side as a permalink-prefix GLOB, built in + the indexed permalink namespace with the same slug transform used at index + time — so a spaced directory name scopes correctly — and ANDed with the + metadata filters in the search WHERE, keeping totals exact and pagination + honest. + """ + async with get_project_client(route_project, context=context, project_id=project_id) as ( + client, + active_project, + ): + # Import here to avoid circular import + from basic_memory.mcp.clients import KnowledgeClient, SearchClient + + # Indexed permalinks carry the project (and, hosted, the workspace) + # route prefix, so the subtree GLOB must be built in that same + # namespace — the one memory:// paths resolve into — or a + # project-relative pattern like "specs/*" would match nothing. + permalink_match = ( + canonical_memory_path_for_active_route( + active_project, + f"{generate_permalink(scope, split_extension=False)}/*", + include_project=ConfigManager().config.permalinks_include_project, + ) + if scope + else None + ) + query = SearchQuery( + permalink_match=permalink_match, + metadata_filters=metadata_filters, + entity_types=[SearchItemType.ENTITY], + ) + search_client = SearchClient(client, active_project.external_id) + response = await search_client.search(query.model_dump(), page=page, page_size=page_size) + payload = response.model_dump(mode="json", exclude_none=True) + if not fields: + return payload + + # Field projection hydrates from the entity's full normalized + # frontmatter — the search hit's own `metadata` is index-row metadata, + # not the canonical projection source. One GET per hit is unavoidable + # (the knowledge API has no bulk entity read), so the cost that matters + # is whether they serialize: page_size is capped at + # MAX_DIRECTORY_PAGE_SIZE, and under per-project cloud routing that + # would be up to 200 round trips end to end inside one find call. + # Bounded concurrency turns the wall time into ceil(hits / limit) + # round trips while keeping the server load predictable. + knowledge_client = KnowledgeClient(client, active_project.external_id) + hit_ids: list[str] = [] + for result in response.results: + if result.external_id is None: + raise ToolError( + "find: search hit carries no external_id — server too old for field projection" + ) + hit_ids.append(result.external_id) + + limiter = asyncio.Semaphore(_FIELD_PROJECTION_CONCURRENCY) + + async def entity_metadata(entity_external_id: str) -> dict[str, Any] | None: + async with limiter: + entity = await knowledge_client.get_entity(entity_external_id) + return entity.entity_metadata + + hydrated = await asyncio.gather(*(entity_metadata(hit_id) for hit_id in hit_ids)) + # Injected post-dump so null field values survive exclude_none. + for row, metadata in zip(payload["results"], hydrated, strict=True): + row["fields"] = _project_metadata_fields(metadata, fields) + return payload + + @mcp.tool( title="Tail", description="Show recently changed notes. Requires 'project' when several are addressable.", diff --git a/tests/cli/test_cli_posix_verbs.py b/tests/cli/test_cli_posix_verbs.py index e209ce55b..ab15277d6 100644 --- a/tests/cli/test_cli_posix_verbs.py +++ b/tests/cli/test_cli_posix_verbs.py @@ -180,6 +180,39 @@ def _dir_node(**overrides): FIND_RESULT_EMPTY = {"nodes": [], "page": 1, "page_size": 10, "total": 0, "has_more": False} +# --meta flips find's payload to the search response shape (the same contract +# grep returns); --fields adds a projected `fields` object per hit, with null +# for a field the hit does not carry. +FIND_META_RESULT = GREP_RESULT + +FIND_META_FIELDS_RESULT = { + **GREP_RESULT, + "results": [ + { + **GREP_RESULT["results"][0], + "fields": { + "title": "Spec [draft] v2", + "priority": "high", + "approved": True, + "missing": None, + }, + }, + { + **GREP_RESULT["results"][1], + "fields": { + "title": "Another Note", + "priority": None, + "approved": False, + "missing": None, + }, + }, + ], +} + +FIND_META_FIELDS_EMPTY = {**GREP_RESULT_EMPTY, "results": []} + +FIND_META_FIELDS_MORE = {**FIND_META_FIELDS_RESULT, "has_more": True} + # tail rows: {type, title, permalink, file_path, created_at}, newest first. TAIL_RESULT = [ { @@ -706,6 +739,159 @@ def test_find_name_and_depth_passthrough(mock_find): assert mock_find.call_args.kwargs["depth"] == 3 +# --------------------------------------------------------------------------- +# find --meta / --fields +# --------------------------------------------------------------------------- + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_RESULT) +def test_find_without_meta_sends_no_predicates(mock_find): + """The plain listing call is unchanged: both new params default to None.""" + result = _invoke(["find", "/specs"]) + + assert result.exit_code == 0, result.output + assert mock_find.call_args.kwargs["meta"] is None + assert mock_find.call_args.kwargs["fields"] is None + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_FIELDS_RESULT) +def test_find_meta_is_repeatable_and_fields_splits_on_commas(mock_find): + """--meta collects one predicate per flag; --fields is the comma form the + tool receives as a list. The CLI only splits — validation stays in the tool.""" + result = _invoke( + [ + "find", + "/specs", + "--meta", + "status=active", + "--meta", + "confidence>0.6", + "--fields", + "title, priority", + ] + ) + + assert result.exit_code == 0, result.output + assert mock_find.call_args.args == ("/specs",) + assert mock_find.call_args.kwargs["meta"] == ["status=active", "confidence>0.6"] + assert mock_find.call_args.kwargs["fields"] == ["title", "priority"] + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_FIELDS_RESULT) +def test_find_meta_json_is_the_tool_payload_verbatim(mock_find): + """JSON stability is the tool payload's stability, projected fields included.""" + result = _invoke(["find", "--meta", "status=active", "--fields", "title", "--json"]) + + assert result.exit_code == 0, result.output + assert json.loads(result.stdout) == FIND_META_FIELDS_RESULT + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_RESULT) +def test_find_meta_without_fields_uses_the_search_renderers(mock_find): + """A metadata payload with no projection renders like grep's results, with + the predicates as the query label.""" + result = _tty_invoke(["find", "--meta", "status=active", "--meta", "confidence>0.6", "--plain"]) + + assert result.exit_code == 0, result.output + assert "Search: status=active AND confidence>0.6" in result.stdout + assert "1. Spec [draft] v2" in result.stdout + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_RESULT) +def test_find_meta_rich_without_fields_shows_the_predicates(mock_find): + result = _tty_invoke(["find", "--meta", "status=active"]) + + flat = _flattened(result.output) + assert result.exit_code == 0, result.output + _assert_not_json(result.output) + assert "status=active" in flat + # User-sourced bracketed titles survive Rich markup parsing. + assert "[draft]" in result.output + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_FIELDS_RESULT) +def test_find_meta_fields_plain_is_path_tab_json(mock_find): + """Plain projection output is one line per hit: path, TAB, compact JSON.""" + result = _tty_invoke( + [ + "find", + "--meta", + "status=active", + "--fields", + "title,priority,approved,missing", + "--plain", + ] + ) + + assert result.exit_code == 0, result.output + assert result.stdout == ( + 'specs/Spec [draft] v2.md\t{"title":"Spec [draft] v2","priority":"high",' + '"approved":true,"missing":null}\n' + 'notes/Another Note.md\t{"title":"Another Note","priority":null,' + '"approved":false,"missing":null}\n' + ) + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_FIELDS_RESULT) +def test_find_meta_fields_rich_adds_one_column_per_field(mock_find): + """The projected table keeps the requested field order; null renders empty + and a non-string value renders as compact JSON.""" + result = _tty_invoke( + ["find", "--meta", "status=active", "--fields", "title,priority,approved,missing"] + ) + + flat = _flattened(result.output) + assert result.exit_code == 0, result.output + _assert_not_json(result.output) + header = next(line for line in result.output.splitlines() if "priority" in line) + columns = ["Path", "title", "priority", "approved", "missing"] + assert [header.index(name) for name in columns] == sorted(header.index(n) for n in columns) + assert "specs/Spec [draft] v2.md" in flat + assert "high" in flat + assert "true" in flat + assert "false" in flat + # total/page summary comes from the search shape (current_page, not page). + assert "page 1" in flat + assert "total 2" in flat + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_FIELDS_MORE) +def test_find_meta_fields_rich_reports_more_pages(mock_find): + """The projected table's summary reads the search shape's current_page and + points at --page when the result set continues.""" + result = _tty_invoke(["find", "--meta", "status=active", "--fields", "title"]) + + flat = _flattened(result.output) + assert result.exit_code == 0, result.output + assert "page 1" in flat + # The panel subtitle is clipped to the table width, so match its opening. + assert "more available" in flat + + +@patch("basic_memory.mcp.tools.find", new_callable=AsyncMock, return_value=FIND_META_FIELDS_EMPTY) +def test_find_meta_fields_rich_no_matches(mock_find): + result = _tty_invoke(["find", "--meta", "status=nope", "--fields", "title"]) + + assert result.exit_code == 0, result.output + assert "No matches." in result.output + + +@patch( + "basic_memory.mcp.tools.find", + new_callable=AsyncMock, + side_effect=ValueError("find: 'fields' requires 'meta' predicates"), +) +def test_find_fields_without_meta_reports_the_tool_refusal(mock_find): + """The combination rules live in the shared tool layer; the CLI passes the + flags through and reports the refusal.""" + result = _invoke(["find", "--fields", "title"]) + + assert result.exit_code == 1 + assert "Error: find: 'fields' requires 'meta' predicates" in result.stderr + assert mock_find.call_args.kwargs["fields"] == ["title"] + assert mock_find.call_args.kwargs["meta"] is None + + # --------------------------------------------------------------------------- # tail # --------------------------------------------------------------------------- diff --git a/tests/cli/test_man_command.py b/tests/cli/test_man_command.py index b4856fd5c..dbbdd3dec 100644 --- a/tests/cli/test_man_command.py +++ b/tests/cli/test_man_command.py @@ -1,13 +1,20 @@ """Tests for `bm man` (#952 / #610): reading bundled pages and making `man bm` work.""" +import re import subprocess from unittest.mock import AsyncMock, patch import pytest +import typer.main from fastmcp.exceptions import ToolError +from typer.core import TyperGroup from typer.testing import CliRunner from basic_memory.cli.app import app + +# Importing main registers every command group, including the POSIX verbs the +# section-1 pages document. +from basic_memory.cli.main import app as full_app from basic_memory.man import bundled_pages # Importing the module registers the man command group on the top-level app. @@ -149,6 +156,37 @@ def fake_run(*args, **kwargs): assert "not on your manpath" not in _flattened(result.output) +# Section 1 is the CLI's own documentation, so a page and its command must not +# drift: an option a user can type that the page never names is undocumented. +# find(1) went stale exactly this way when `bm find` grew --meta/--fields. +PAGES_WITHOUT_TOP_LEVEL_COMMANDS = {"apropos"} # `bm man apropos`, not `bm apropos` +DOCUMENTED_VERBS = {"cat", "find", "grep", "head", "ls", "tail", "tree"} + + +def test_section_1_pages_document_every_option_of_their_command(): + """Every long option of a documented verb appears in its manual page.""" + # typer vendors its own click, so the group type comes from typer.core. + cli = typer.main.get_command(full_app) + assert isinstance(cli, TyperGroup) + checked: set[str] = set() + + for page in bundled_pages(): + if page.section != 1 or page.name in PAGES_WITHOUT_TOP_LEVEL_COMMANDS: + continue + body = page.body() + for param in cli.commands[page.name].params: + for option in param.opts: + if not option.startswith("--"): + continue + # Boundary match so --page cannot stand in for --page-size. + assert re.search(rf"{re.escape(option)}(?![\w-])", body), ( + f"{page.title} does not document {option}; the page is stale" + ) + checked.add(page.name) + + assert checked == DOCUMENTED_VERBS + + def test_man_install_skips_app_initialization(tmp_path, monkeypatch): """man install must not touch the database (PR #971 review). diff --git a/tests/mcp/test_tool_contracts.py b/tests/mcp/test_tool_contracts.py index f3e7909ec..758d75d1b 100644 --- a/tests/mcp/test_tool_contracts.py +++ b/tests/mcp/test_tool_contracts.py @@ -59,7 +59,17 @@ "output_format", ], "fetch": ["id"], - "find": ["path", "name", "depth", "page", "page_size", "project", "project_id"], + "find": [ + "path", + "name", + "depth", + "page", + "page_size", + "meta", + "fields", + "project", + "project_id", + ], "grep": ["pattern", "literal", "page", "page_size", "project", "project_id"], "list_directory": [ "dir_name", diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index f6684b54d..0e50c002f 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -5,10 +5,13 @@ assert on the JSON shapes the canonical `output_format="json"` paths produce. """ +import asyncio +import re from pathlib import Path from types import SimpleNamespace import pytest +import pytest_asyncio import yaml from fastmcp.exceptions import ToolError @@ -17,7 +20,8 @@ ProjectPrefixConflictError, UnqualifiedPathRefusedError, ) -from basic_memory.mcp.tools import cat, find, grep, ls, man, tail, write_note +from basic_memory.mcp.tools import cat, find, grep, ls, man, search_notes, tail, write_note +from basic_memory.repository.metadata_filters import parse_metadata_filters from basic_memory.schemas.search import SearchRetrievalMode @@ -506,6 +510,484 @@ async def test_find_rejects_bad_arguments(kwargs, message): await find(**kwargs) +# --- find --meta: predicate parsing --- +# Each predicate string translates onto exactly one metadata_filters entry, in +# the grammar the search API's parse_metadata_filters already supports. + +PREDICATE_GRAMMAR = [ + ("status=active", {"status": "active"}), + ("status = active", {"status": "active"}), + ("confidence>0.6", {"confidence": {"$gt": 0.6}}), + ("confidence >= 0.6", {"confidence": {"$gte": 0.6}}), + ("confidence<0.6", {"confidence": {"$lt": 0.6}}), + ("confidence<=0.6", {"confidence": {"$lte": 0.6}}), + ("priority in high,critical", {"priority": {"$in": ["high", "critical"]}}), + ("priority in high, critical", {"priority": {"$in": ["high", "critical"]}}), + ("tags has security,oauth", {"tags": ["security", "oauth"]}), + ("score between 0.3,0.8", {"score": {"$between": [0.3, 0.8]}}), + # Quoting is the documented escape for literal values, and it holds inside + # a list: the comma it protects belongs to the value, not to the list. + ('label in "a,b",c', {"label": {"$in": ["a,b", "c"]}}), + ('tags has "red, green"', {"tags": ["red, green"]}), + ('name in "quoted"', {"name": {"$in": ["quoted"]}}), + # Dot-paths address nested frontmatter and pass through verbatim. + ("review.approved=true", {"review.approved": True}), + # The one alias search_notes carries, so both surfaces accept one spelling. + ("note_type=spec", {"type": "spec"}), +] + + +@pytest.mark.parametrize(("predicate", "expected"), PREDICATE_GRAMMAR) +def test_parse_meta_predicate_grammar(predicate, expected): + assert posix_tools._parse_meta_predicates([predicate]) == expected + + +@pytest.mark.parametrize(("predicate", "expected"), PREDICATE_GRAMMAR) +def test_parsed_predicates_are_valid_api_metadata_filters(predicate, expected): + """Every predicate the parser accepts is a filter the search API accepts. + + parse_metadata_filters is the server-side authority; running the produced + dict through it proves the CLI/MCP grammar is a strict subset rather than a + parallel dialect that only fails at request time. + """ + assert parse_metadata_filters(posix_tools._parse_meta_predicates([predicate])) + + +@pytest.mark.parametrize( + ("predicate", "expected_value"), + [ + ("done=true", True), + ("done=false", False), + ("owner=null", None), + ("count=3", 3), + ("ratio=1.5", 1.5), + ("status=active", "active"), + # A JSON-quoted token forces the literal string, escaping the inference. + ('status="true"', "true"), + # Non-scalar JSON is not a filter value; the raw text stays a string. + ("shape=[1,2]", "[1,2]"), + ], +) +def test_predicate_values_are_json_scalar_inferred(predicate, expected_value): + """Values type-infer so a predicate string produces the same dict a rich + search_notes caller would pass as JSON.""" + key = predicate.split("=", 1)[0] + + assert posix_tools._parse_meta_predicates([predicate]) == {key: expected_value} + + +def test_parse_meta_predicates_and_together(): + assert posix_tools._parse_meta_predicates(["status=active", "confidence>0.6"]) == { + "status": "active", + "confidence": {"$gt": 0.6}, + } + + +@pytest.mark.parametrize( + ("predicates", "message"), + [ + # The API has no $ne, so != is deliberately absent from the grammar. + (["status!=active"], "unsupported predicate operator in 'status!=active'"), + (["status ~= active"], "unsupported predicate operator"), + (["priority gte 3"], "unsupported predicate operator"), + (["priority in"], "unsupported predicate operator"), + (["nothing"], "unsupported predicate operator"), + (["score between 0.3"], "'between' needs exactly min,max"), + (["score between 0.1,0.2,0.3"], "'between' needs exactly min,max"), + (["priority in high,,low"], "empty list element"), + # A severed quote would silently filter for values nothing carries. + (['priority in "high,low'], "unterminated quoted value"), + (['tags has red,"green'], "unterminated quoted value"), + (["status="], "has no value"), + (["status=active", "status=draft"], "duplicate predicate key 'status'"), + # The alias collapses onto the same key, so the collision is still caught. + (["note_type=note", "type=spec"], "duplicate predicate key 'type'"), + ], +) +def test_parse_meta_predicates_fails_fast(predicates, message): + with pytest.raises(ValueError, match=re.escape(message)): + posix_tools._parse_meta_predicates(predicates) + + +def test_unsupported_operator_names_the_supported_set(): + """The refusal teaches the whole grammar instead of just rejecting.""" + with pytest.raises(ValueError, match="supported: = > >= < <= in has between"): + posix_tools._parse_meta_predicates(["status matches active"]) + + +def test_duplicate_key_refusal_points_at_between(): + with pytest.raises(ValueError, match=re.escape("use 'between' for ranges")): + posix_tools._parse_meta_predicates(["score>0.3", "score<0.8"]) + + +@pytest.mark.parametrize( + ("entity_metadata", "fields", "expected"), + [ + ({"title": "Alpha"}, ["title"], {"title": "Alpha"}), + ({"review": {"approved": True}}, ["review.approved"], {"review.approved": True}), + # A missing key renders as null — the row is never dropped. + ({"title": "Alpha"}, ["missing"], {"missing": None}), + # A non-dict intermediate ends the walk at null rather than raising. + ({"review": "yes"}, ["review.approved"], {"review.approved": None}), + (None, ["title"], {"title": None}), + ({"a": {"b": {"c": 1}}}, ["a.b.c", "a.b"], {"a.b.c": 1, "a.b": {"c": 1}}), + ], +) +def test_project_metadata_fields(entity_metadata, fields, expected): + assert posix_tools._project_metadata_fields(entity_metadata, fields) == expected + + +# --- find --meta: metadata search arm --- + + +@pytest_asyncio.fixture +async def meta_notes(client, test_project): + """Seed metadata-bearing notes across two directories (one name has a space).""" + await write_note( + title="Alpha Spec", + directory="specs", + content="# Alpha Spec\n\nalpha body", + project=test_project.name, + metadata={ + "status": "active", + "priority": "high", + "confidence": 0.9, + "tags": ["security", "oauth"], + "review": {"approved": True}, + }, + ) + await write_note( + title="Beta Spec", + directory="specs", + content="# Beta Spec\n\nbeta body", + project=test_project.name, + metadata={ + "status": "draft", + "priority": "low", + "confidence": 0.2, + "tags": ["security"], + }, + ) + await write_note( + title="Gamma Note", + directory="My Notes", + content="# Gamma Note\n\ngamma body", + project=test_project.name, + metadata={ + "status": "active", + "priority": "critical", + "confidence": 0.5, + "tags": ["oauth"], + }, + ) + + +@pytest.mark.asyncio +async def test_find_meta_returns_the_search_response_shape(client, test_project, meta_notes): + result = await find(meta=["status=active"], project=test_project.name) + + assert set(result) == { + "results", + "current_page", + "page_size", + "total", + "total_is_exact", + "has_more", + } + assert result["total"] == 2 + assert result["total_is_exact"] is True + assert {row["title"] for row in result["results"]} == {"Alpha Spec", "Gamma Note"} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("meta", "expected_titles"), + [ + (["status=active"], {"Alpha Spec", "Gamma Note"}), + (["confidence>0.5"], {"Alpha Spec"}), + (["confidence>=0.5"], {"Alpha Spec", "Gamma Note"}), + (["confidence<0.5"], {"Beta Spec"}), + (["confidence<=0.5"], {"Beta Spec", "Gamma Note"}), + (["priority in high,critical"], {"Alpha Spec", "Gamma Note"}), + # `has` is contains-ALL, so a two-element list narrows to one note. + (["tags has security"], {"Alpha Spec", "Beta Spec"}), + (["tags has security,oauth"], {"Alpha Spec"}), + (["confidence between 0.1,0.6"], {"Beta Spec", "Gamma Note"}), + (["review.approved=true"], {"Alpha Spec"}), + (["note_type=note"], {"Alpha Spec", "Beta Spec", "Gamma Note"}), + # Repeated predicates AND together. + (["status=active", "priority=high"], {"Alpha Spec"}), + (["status=active", "priority=nonexistent"], set()), + ], +) +async def test_find_meta_operators_select_the_right_notes( + client, test_project, meta_notes, meta, expected_titles +): + result = await find(meta=meta, project=test_project.name) + + assert {row["title"] for row in result["results"]} == expected_titles + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("path", "expected_titles"), + [ + ("/", {"Alpha Spec", "Gamma Note"}), + ("specs", {"Alpha Spec"}), + ("/specs", {"Alpha Spec"}), + # A spaced directory scopes by its slug, the same transform used at index time. + ("My Notes", {"Gamma Note"}), + ("nonexistent", set()), + ], +) +async def test_find_meta_scopes_by_path_subtree( + client, test_project, meta_notes, path, expected_titles +): + result = await find(path, meta=["status=active"], project=test_project.name) + + assert {row["title"] for row in result["results"]} == expected_titles + + +@pytest.mark.asyncio +async def test_find_meta_paginates_with_exact_totals(client, test_project, meta_notes): + """Scope and predicates AND inside one server-side WHERE, so the total is + the real match count and every page is reachable.""" + first = await find(meta=["status=active"], page_size=1, project=test_project.name) + second = await find(meta=["status=active"], page=2, page_size=1, project=test_project.name) + + assert first["total"] == 2 + assert first["total_is_exact"] is True + assert first["has_more"] is True + assert len(first["results"]) == 1 + assert second["has_more"] is False + assert first["results"][0]["permalink"] != second["results"][0]["permalink"] + + +@pytest.mark.asyncio +async def test_find_meta_projects_requested_fields(client, test_project, meta_notes): + """Projection reads the entity's normalized frontmatter; a field a hit does + not carry renders as null instead of dropping the row.""" + result = await find( + meta=["status=active"], + fields=["title", "priority", "review.approved", "missing_field"], + project=test_project.name, + ) + + projected = {row["title"]: row["fields"] for row in result["results"]} + assert projected["Alpha Spec"] == { + "title": "Alpha Spec", + "priority": "high", + # Frontmatter round-trips the nested boolean as its stored string form. + "review.approved": "True", + "missing_field": None, + } + assert projected["Gamma Note"] == { + "title": "Gamma Note", + "priority": "critical", + "review.approved": None, + "missing_field": None, + } + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("kwargs", "message"), + [ + # The metadata search matches slugified permalinks, so a filename glob + # has no faithful translation — refuse rather than silently ignore it. + ({"name": "*.md", "meta": ["status=active"]}, "'name' cannot combine with 'meta'"), + # Permalink-prefix scope is whole-subtree; a depth bound is inexpressible. + ({"depth": 3, "meta": ["status=active"]}, "'depth' cannot combine with 'meta'"), + ({"fields": ["title"]}, "'fields' requires 'meta' predicates"), + ({"meta": ["status=active"], "fields": []}, "must be non-empty"), + ({"meta": ["status=active"], "fields": [" "]}, "must be non-empty"), + # An empty predicate list is not "no filter": it would parse to {} and + # run the metadata search with no WHERE at all, matching every note in + # the project where the caller asked for a filtered set. + ({"meta": []}, "'meta' must carry at least one predicate"), + ], +) +async def test_find_meta_combination_rules_refuse_before_io(kwargs, message): + with pytest.raises(ValueError, match=re.escape(message)): + await find(**kwargs) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("kwargs", "message"), + [ + ({"page": 0}, "page must be >= 1"), + ({"page_size": 0}, "page_size must be >= 1"), + ({"page_size": 201}, "page_size must be <= 200"), + ], +) +async def test_find_meta_rejects_bad_pagination(kwargs, message): + """The metadata arm bounds pagination exactly as the listing arm does. + + `find_listing` states those bounds for the directory arm, and the metadata + arm never reaches it — so `find` states them again on the branch it does + take. Unstated, a `page=0` would route, open a client, and come back as a + transport error from the search API instead of the refusal the plain + listing gives for the same argument. No project fixture here is the point: + the refusal lands before any I/O. + """ + with pytest.raises(ValueError, match=re.escape(message)): + await find(meta=["status=active"], **kwargs) + + +@pytest.mark.asyncio +async def test_find_meta_quoted_list_element_keeps_its_comma(client, test_project): + """The quoting escape reaches the list operators, not just the scalar ones. + + Splitting the raw value before scalar inference would sever '"a,b"' into + '"a' and 'b"' and filter for values nothing carries — no error, no hits. + """ + await write_note( + title="Comma Note", + directory="notes", + content="# Comma Note", + project=test_project.name, + metadata={"label": "a,b"}, + ) + + result = await find(meta=['label in "a,b",c'], project=test_project.name) + + assert {row["title"] for row in result["results"]} == {"Comma Note"} + + +@pytest.mark.asyncio +async def test_find_meta_empty_list_is_not_an_unfiltered_search(client, test_project, meta_notes): + """meta=[] must never widen to the whole project. + + The MCP surface can produce it directly (`"meta": []`, or `"meta": "[]"` + through coerce_list), so the refusal is asserted against the real stack — + where an unfiltered metadata search would return all three seeded notes. + """ + with pytest.raises(ValueError, match=re.escape("'meta' must carry at least one predicate")): + await find(meta=[], project=test_project.name) + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("limit", "expected_peak"), [(8, 2), (1, 1)]) +async def test_find_meta_fields_hydrates_hits_concurrently( + client, test_project, meta_notes, monkeypatch, limit, expected_peak +): + """Projection costs one entity GET per hit (no bulk read exists), so the + reads must overlap up to the bound instead of serializing — a full page + serialized is up to MAX_DIRECTORY_PAGE_SIZE round trips inside one call.""" + # Import here to mirror the tool's own deferred client import. + from basic_memory.mcp.clients import KnowledgeClient + + monkeypatch.setattr(posix_tools, "_FIELD_PROJECTION_CONCURRENCY", limit) + original_get_entity = KnowledgeClient.get_entity + in_flight = 0 + peak = 0 + + async def counting_get_entity(self, entity_id, **kwargs): + nonlocal in_flight, peak + in_flight += 1 + peak = max(peak, in_flight) + try: + # Yield once so an overlap, if any, is observable deterministically. + await asyncio.sleep(0) + return await original_get_entity(self, entity_id, **kwargs) + finally: + in_flight -= 1 + + monkeypatch.setattr(KnowledgeClient, "get_entity", counting_get_entity) + + result = await find(meta=["status=active"], fields=["title"], project=test_project.name) + + assert peak == expected_peak + # Order still pairs each hit with its own entity, concurrency notwithstanding. + assert {row["title"]: row["fields"]["title"] for row in result["results"]} == { + "Alpha Spec": "Alpha Spec", + "Gamma Note": "Gamma Note", + } + + +@pytest.mark.asyncio +async def test_find_meta_unsupported_key_surfaces_the_api_error(client, test_project, meta_notes): + """The server owns key validation; its refusal reaches the caller intact.""" + with pytest.raises(ToolError, match="metadata filter key"): + await find(meta=["status.=active"], project=test_project.name) + + +@pytest.mark.asyncio +async def test_find_meta_fields_requires_hit_external_ids(client, test_project, monkeypatch): + """Projection hydrates each hit by external_id, so a server old enough to + omit it fails fast instead of silently returning unprojected rows.""" + # Import here to mirror the tool's own deferred client import. + from basic_memory.mcp.clients import SearchClient + from basic_memory.schemas.search import SearchItemType, SearchResponse, SearchResult + + async def legacy_search(self, query, page=1, page_size=10): + return SearchResponse( + results=[ + SearchResult( + title="Alpha Spec", + type=SearchItemType.ENTITY, + score=1.0, + permalink="test-project/specs/alpha-spec", + file_path="specs/Alpha Spec.md", + ) + ], + current_page=page, + page_size=page_size, + total=1, + ) + + monkeypatch.setattr(SearchClient, "search", legacy_search) + + with pytest.raises(ToolError, match="server too old for field projection"): + await find(meta=["status=active"], fields=["title"], project=test_project.name) + + +@pytest.mark.asyncio +async def test_find_without_meta_stays_the_directory_listing(client, test_graph, test_project): + """The no-meta arm is untouched: same payload with the new params defaulted + or passed as None, and no search-shape keys anywhere.""" + baseline = await find(name="*.md", project=test_project.name) + with_defaults = await find(name="*.md", meta=None, fields=None, project=test_project.name) + + assert with_defaults == baseline + assert set(baseline) == {"nodes", "page", "page_size", "total", "has_more"} + assert baseline["total"] == 5 + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("meta", "filters"), + [ + (["status=active"], {"status": "active"}), + (["confidence>0.5"], {"confidence": {"$gt": 0.5}}), + (["priority in high,critical"], {"priority": {"$in": ["high", "critical"]}}), + (["tags has security,oauth"], {"tags": ["security", "oauth"]}), + (["confidence between 0.1,0.6"], {"confidence": {"$between": [0.1, 0.6]}}), + (["review.approved=true"], {"review.approved": True}), + ], +) +async def test_find_meta_matches_search_notes_parity( + client, test_project, meta_notes, meta, filters +): + """PARITY: the POSIX surface and the rich surface answer the same question + identically — same filters dict, same hits — through the same real stack.""" + assert posix_tools._parse_meta_predicates(meta) == filters + + found = await find(meta=meta, project=test_project.name) + searched = await search_notes( + project=test_project.name, + metadata_filters=filters, + output_format="json", + ) + + assert isinstance(searched, dict) + assert found["results"] + assert {row["permalink"] for row in found["results"]} == { + row["permalink"] for row in searched["results"] + } + + # --- tail --- @@ -739,6 +1221,53 @@ async def test_find_qualified_path_routes_to_project( assert names == {"Second Find Note.md"} +@pytest.mark.asyncio +async def test_find_meta_qualified_path_routes_to_project( + client, test_project, second_project, no_project_constraint +): + """The meta arm shares find's route resolution, so '/dir' scopes to + that project's subtree and nothing from the other project leaks in.""" + await write_note( + title="Second Meta Note", + directory="notes", + content="# Second Meta Note", + project="second-project", + metadata={"status": "active"}, + ) + await write_note( + title="Home Meta Note", + directory="notes", + content="# Home Meta Note", + project=test_project.name, + metadata={"status": "active"}, + ) + + result = await find("second-project/notes", meta=["status=active"]) + + assert {row["title"] for row in result["results"]} == {"Second Meta Note"} + + +@pytest.mark.asyncio +async def test_find_meta_unqualified_refuses_in_multi_project_config( + client, test_project, second_project, no_project_constraint +): + """A metadata query is still project-scoped, so an unqualified call in a + multi-project config refuses with the active project list.""" + with pytest.raises(UnqualifiedPathRefusedError, match="no project specified"): + await find(meta=["status=active"]) + + +@pytest.mark.asyncio +async def test_find_meta_predicate_errors_precede_routing( + client, test_project, second_project, no_project_constraint +): + """Predicate parsing happens before any routing decision, so a bad operator + reports itself instead of hiding behind the multi-project refusal.""" + with pytest.raises(ValueError, match="unsupported predicate operator") as excinfo: + await find(meta=["status!=active"]) + assert not isinstance(excinfo.value, UnqualifiedPathRefusedError) + + @pytest.mark.asyncio async def test_cat_qualified_identifier_equals_explicit_project_read( client, test_graph, test_project, second_project, no_project_constraint From 68b5fab6d7c7937e56a30b2e9a862de6680755da Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 10:39:06 -0500 Subject: [PATCH 02/12] fix(mcp): refuse subtree metadata scopes and malformed predicate operators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on find --meta. The subtree scope was built from permalink_match, but a permalink stops reflecting file_path once a note carries an explicit frontmatter permalink or is moved with update_permalinks_on_move=False (the default). So 'find /specs --meta ...' silently dropped matching files under specs/ and admitted files elsewhere whose permalink began with specs/ — while reporting the wrong total as exact. The search stack has no file-path filter to fix this with: search_index stores the column but nothing queries it, and threading one through would be a public schema change across both backends and the vector path. Rather than ship a half-correct filter, a non-root scope now refuses and names the limitation; the predicates are the whole WHERE, so the reported total describes the query that actually ran. A bare still routes. The predicate regex also folded a second operator character into the value, so 'status==active' became {'status': '=active'} and returned empty instead of erroring. An unquoted value starting with an operator character now refuses and teaches both the grammar and the quoting escape, which is verified end to end. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/cli/commands/posix.py | 3 +- src/basic_memory/man/man1/find(1).md | 40 +++-- src/basic_memory/mcp/tools/posix_tools.py | 132 +++++++++------ tests/mcp/test_tool_posix.py | 191 ++++++++++++++++++++-- 4 files changed, 282 insertions(+), 84 deletions(-) diff --git a/src/basic_memory/cli/commands/posix.py b/src/basic_memory/cli/commands/posix.py index 2679b0295..faab39da7 100644 --- a/src/basic_memory/cli/commands/posix.py +++ b/src/basic_memory/cli/commands/posix.py @@ -745,7 +745,8 @@ def find( "--meta", help=( "Metadata predicate, repeatable: 'status=active', 'confidence>0.6', " - "'priority in high,critical', 'tags has security', 'score between 0.3,0.8'" + "'priority in high,critical', 'tags has security', 'score between 0.3,0.8'. " + "Queries the whole project — a PATH below the project root is refused" ), ), ] = None, diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index 95425afec..2256d26a0 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -20,7 +20,7 @@ bm find [PATH] [--name GLOB] [--depth N] [--page N] [--page-size N] [--json | --plain] [--project NAME | --project-id UUID] [--local | --cloud] -bm find [PATH] --meta PREDICATE [--meta PREDICATE ...] [--fields LIST] +bm find [PROJECT] --meta PREDICATE [--meta PREDICATE ...] [--fields LIST] [--page N] [--page-size N] [--json | --plain] [--project NAME | --project-id UUID] [--local | --cloud] ``` @@ -36,23 +36,28 @@ prints one path per line, find(1) style; `--json` (or piped output) emits the listing with pagination and totals. With `--meta`, find queries notes by their frontmatter instead: every -predicate must hold, and `PATH` still scopes the results — server-side, by -permalink prefix, so the totals are exact and every page is reachable. The -payload becomes the search response shape (the same one `bm grep` returns), -not the directory listing. Non-markdown files carry no frontmatter and are -never metadata hits. +predicate must hold, across the whole project. The payload becomes the search +response shape (the same one `bm grep` returns), not the directory listing. +Totals are exact and every page is reachable. Non-markdown files carry no +frontmatter and are never metadata hits. `--fields` is the SELECT to the predicates' WHERE: it adds a `fields` object to each hit carrying the named frontmatter values, so a filtered set can be tabulated without reading every note. A field a hit does not carry renders as null; the row is never dropped. -`--name` and `--depth` are refused alongside `--meta`. The metadata query -matches slugified permalinks, where a filename glob has no faithful -translation, and its subtree scope is all-or-nothing, where a depth bound is -not expressible — refusing beats silently ignoring either and misreporting -the match set. Scope with `PATH` instead. `--fields` without `--meta` is -refused for the same honesty: without predicates there is nothing to project. +`--name`, `--depth`, and a `PATH` below the project root are all refused +alongside `--meta`, because the search API expresses none of them: no +filename glob, no depth bound, and no file-path filter. Its one path-shaped +predicate is a permalink prefix, and a permalink is not a file path — a note +that pins `permalink:` in its frontmatter, or that was moved while +`update_permalinks_on_move` is off (the default), keeps a permalink that no +longer says where the file lives. Scoping by it would drop notes that really +are under the named directory, admit notes that are not, and still report the +count as exact. Refusing beats misreporting the match set. `PATH` may still +name a project (`bm find myproject --meta ...`) — that is a routing prefix, +not a subtree. `--fields` without `--meta` is refused for the same honesty: +without predicates there is nothing to project. ## PREDICATE GRAMMAR @@ -76,14 +81,18 @@ comma inside a list element: `label in "a,b",c` matches `a,b` or `c`. Keys accept dot-paths into nested frontmatter (`review.approved`), and `note_type` is accepted as a spelling of the frontmatter `type` key, matching `search-notes(3)`. Any other operator (`!=` among them — the search API has -no not-equals) fails fast, naming the supported set. +no not-equals) fails fast, naming the supported set. That includes a +mis-spelled multi-character operator: `status==active`, `status=>active` and +`count>>3` are refused rather than read as the values `=active`, `>active` +and `>3`. An unquoted value may therefore not begin with `=`, `<` or `>`; +quote one that genuinely does, as in `range=">=5"`. ## OPTIONS - **--name** — file-name glob, e.g. `"*.md"`; omitted matches everything. Cannot combine with `--meta` - **--depth** — recursion depth, 1-10 (default 10). A non-default depth - cannot combine with `--meta` + cannot combine with `--meta`. Nor can a `PATH` below the project root - **--meta** — frontmatter predicate, repeatable; see PREDICATE GRAMMAR. Switches the payload to the search response shape - **--fields** — comma-separated frontmatter fields to show per hit, e.g. @@ -97,7 +106,8 @@ bm find --name "*.md" bm find /specs --depth 3 bm find /notes --name "auth*" --plain bm find --meta "status=active" -bm find /specs --meta "status=active" --meta "confidence>0.6" +bm find --meta "status=active" --meta "confidence>0.6" +bm find myproject --meta "status=active" bm find --meta "tags has security,oauth" --fields title,priority bm find --meta "status=active" --fields title --plain ``` diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index 39abd0179..efccb7538 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -48,7 +48,6 @@ get_project_client, resolve_project_path_route, ) -from basic_memory.mcp.project_context_identifiers import canonical_memory_path_for_active_route from basic_memory.mcp.server import POSIX_TOOLS_TAG, mcp, set_posix_tools_visibility from basic_memory.schemas.directory import ( DEFAULT_DIRECTORY_PAGE_SIZE, @@ -550,13 +549,19 @@ async def find_listing( _PREDICATE_SYMBOL_RE = re.compile(r"^([A-Za-z0-9_.-]+)\s*(>=|<=|=|>|<)\s*(.*)$") _SYMBOL_OPERATORS = {">": "$gt", ">=": "$gte", "<": "$lt", "<=": "$lte"} _SUPPORTED_PREDICATE_OPS = "= > >= < <= in has between" +# The symbol regex consumes the first operator it recognizes, so an operator +# spelling outside the supported set ("==", "=>", ">>", ">=>") leaves its +# second character at the head of the value. These are the characters that can +# be left behind that way. A set, not a string: "" is a substring of any string +# but is not a member here, so an empty token never reads as operator-prefixed. +_OPERATOR_VALUE_PREFIXES = frozenset("=<>") # Mirrors search_notes' alias: "note_type" (the entity model column) means the # frontmatter "type" key, so the two surfaces accept the same spelling. _METADATA_KEY_ALIASES = {"note_type": "type"} -def _predicate_scalar(token: str) -> Any: - """JSON-scalar-infer one predicate value token. +def _predicate_scalar(token: str, predicate: str) -> Any: + """Read one predicate value token, refusing a folded-in operator character. "true"/"false"/"null"/numbers become bool/None/int/float so the produced filters dict is byte-equal to what a rich search_notes caller passes as @@ -564,6 +569,21 @@ def _predicate_scalar(token: str) -> Any: is not a JSON scalar stays the raw string. """ text = token.strip() + # Trigger: an unquoted value opens with one of the operator characters. + # Why: only the operators in _SUPPORTED_PREDICATE_OPS are real, but the + # regexes match the longest supported one and hand the rest to the + # value — 'status==active' would filter for the string "=active" and + # 'count>>3' for ">3", so a typo'd operator answered as an empty (or + # worse, a non-empty but wrong) result set instead of the refusal the + # grammar documents. + # Outcome: refuse, naming the supported set and the quoting escape hatch a + # value that genuinely starts with '=', '<' or '>' needs. + if text[:1] in _OPERATOR_VALUE_PREFIXES: + raise ValueError( + f"find: unsupported predicate operator in '{predicate}'; " + f"supported: {_SUPPORTED_PREDICATE_OPS}; quote the value as " + f'"{text}" to match text that starts with that character' + ) try: value = json.loads(text) except json.JSONDecodeError: @@ -639,7 +659,8 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: ) if op in ("in", "has", "between"): items = [ - _predicate_scalar(item) for item in _split_predicate_items(raw_value, predicate) + _predicate_scalar(item, predicate) + for item in _split_predicate_items(raw_value, predicate) ] if op == "between" and len(items) != 2: raise ValueError(f"find: 'between' needs exactly min,max in '{predicate}'") @@ -649,7 +670,7 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: else: if not raw_value.strip(): raise ValueError(f"find: predicate '{predicate}' has no value") - value = _predicate_scalar(raw_value) + value = _predicate_scalar(raw_value, predicate) filters[key] = value if op == "=" else {_SYMBOL_OPERATORS[op]: value} return filters @@ -703,21 +724,24 @@ async def find( """Recursively list files by name glob, or query notes by frontmatter metadata. Without `meta`, this is a recursive directory listing. With `meta`, find - routes through the metadata search instead: predicates AND together and - `path` still scopes the results — server-side, by slugified-permalink - prefix, so non-markdown files (which have no permalink) are never metadata - hits. `name` and `depth` are refused alongside `meta`: the search API has - no filename-glob or depth-bound facility, and silently ignoring either - would misreport the match set. + routes through the metadata search instead: predicates AND together over + the whole project, and non-markdown files (which carry no frontmatter) are + never hits. `name`, `depth`, and any `path` below the project root are all + refused alongside `meta`, because the search API expresses none of them: it + has no filename glob, no depth bound, and no file-path filter — it scopes + by permalink, and a permalink stops mirroring its file path the moment a + note pins one in frontmatter or is moved under the default + update_permalinks_on_move=False. Substituting any of the three would + misreport the match set while still calling the total exact. Args: path: Directory to start from (default: project root). '/path' - routes into that project. With `meta`, scopes the search to this - subtree. + routes into that project. With `meta`, only a project root is + addressable — a directory below it is refused (see above). name: File-name glob to match, e.g. "*.md". None matches everything. - Cannot combine with `meta` — scope with `path` instead. + Cannot combine with `meta`. depth: How many levels to recurse (1-10, default: 10). A non-default - depth cannot combine with `meta` (subtree scope is all-or-nothing). + depth cannot combine with `meta`. page: Page number (1-indexed). page_size: Nodes per page. meta: Frontmatter metadata predicates, repeatable; every predicate must @@ -732,9 +756,10 @@ async def find( become booleans/None/numbers); quote a token to force a literal string (e.g. 'status="true"'), including inside a list, where the quotes also protect a comma ('label in "a,b",c' matches "a,b" or - "c"). Keys accept dot-paths ("review.approved"); "note_type" - aliases the frontmatter "type" key. Any other operator fails fast - naming the supported set. + "c") and a value that itself starts with an operator character + ('range=">=5"'). Keys accept dot-paths ("review.approved"); + "note_type" aliases the frontmatter "type" key. Any other operator + fails fast naming the supported set. fields: Frontmatter fields to return per hit (dot-paths allowed), e.g. ["title", "priority"]. Requires `meta`. A field missing on a hit renders as null — rows are never dropped. @@ -750,11 +775,12 @@ async def find( requested. """ # Combination rules, before any I/O. The metadata search matches slugified - # permalinks, where a filename glob has no faithful translation, and its - # permalink-prefix scope is whole-subtree, so a depth bound is not - # expressible — refuse both rather than silently ignoring either. `fields` - # is the SELECT to the predicates' WHERE; without predicates the directory - # listing stays byte-identical to today. + # permalinks, where a filename glob has no faithful translation, and it + # takes no depth bound — refuse both rather than silently ignoring either. + # (The `path` scope is refused too, after routing, where the caller's + # project prefix has been stripped and "is this the project root?" can be + # answered.) `fields` is the SELECT to the predicates' WHERE; without + # predicates the directory listing stays byte-identical to today. if meta is not None: # Trigger: 'meta' present but carrying no predicates. # Why: an empty list parses to an empty filters dict, which is not None @@ -770,12 +796,12 @@ async def find( if name is not None: raise ValueError( "find: 'name' cannot combine with 'meta' — the metadata search matches " - "slugified permalinks, not filenames; scope with 'path' instead" + "slugified permalinks, not filenames" ) if depth != _MAX_FIND_DEPTH: raise ValueError( - "find: 'depth' cannot combine with 'meta' — the metadata search scopes " - "by whole subtree; scope with 'path' instead" + "find: 'depth' cannot combine with 'meta' — the metadata search takes " + "no depth bound" ) if fields is not None: if meta is None: @@ -816,11 +842,33 @@ async def find( route = await resolve_project_path_route( path, project=project, project_id=project_id, context=context ) + # Trigger: a metadata query addressed below the project root. + # Why: the search API has no file-path filter. Its only path-shaped + # predicate is permalink_match, and a permalink is not a file path: + # a note that pins `permalink:` in frontmatter, or that was moved + # while update_permalinks_on_move is off (the default), keeps a + # permalink that no longer says where the file lives. Scoping by + # permalink prefix would therefore drop notes that really are under + # the requested directory and admit notes that are not — and report + # the resulting count as total_is_exact. + # Outcome: refuse the subtree scope naming the limitation, rather than + # answer a different question with an exact-looking total. The + # project-root form stays exact, so it is the offered path. + # + # route.path is the caller's input verbatim when no project prefix was + # recognized, so one strip covers both routed and raw spellings. + if route.path.strip("/"): + raise ValueError( + f"find: 'meta' cannot be scoped to '{path}' — the search API filters by " + "permalink, not by file path, and a permalink stops mirroring its file " + "path once a note pins one in frontmatter or is moved with " + "update_permalinks_on_move disabled (the default). A subtree scope would " + "omit matching files and admit unrelated ones while still reporting the " + "total as exact. Query the project root and filter the hits by their " + "file_path, or drop 'meta' for a path-scoped directory listing" + ) return await _find_by_metadata( route_project=route.project, - # route.path is the caller's input verbatim when no project prefix - # was recognized, so one strip covers both routed and raw spellings. - scope=route.path.strip("/"), metadata_filters=metadata_filters, fields=fields, page=page, @@ -847,7 +895,6 @@ async def find( async def _find_by_metadata( *, route_project: Optional[str], - scope: str, metadata_filters: dict[str, Any], fields: Optional[list[str]], page: int, @@ -855,16 +902,15 @@ async def _find_by_metadata( project_id: Optional[str], context: Context | None, ) -> dict[str, Any]: - """find's metadata arm: one search call, plus per-hit field projection. + """find's metadata arm: one project-wide search call, plus field projection. The listing arm's counterpart to ``find_listing``; `find` has already bounded the pagination and refused `name` and `depth` against `meta`. - The path scope composes server-side as a permalink-prefix GLOB, built in - the indexed permalink namespace with the same slug transform used at index - time — so a spaced directory name scopes correctly — and ANDed with the - metadata filters in the search WHERE, keeping totals exact and pagination - honest. + The predicates are the whole WHERE — `find` has already refused any scope + below the project root, because the search API offers no file-path filter + to express one honestly. So the total the server reports is the real match + count for the query that ran, and every page of it is reachable. """ async with get_project_client(route_project, context=context, project_id=project_id) as ( client, @@ -873,21 +919,7 @@ async def _find_by_metadata( # Import here to avoid circular import from basic_memory.mcp.clients import KnowledgeClient, SearchClient - # Indexed permalinks carry the project (and, hosted, the workspace) - # route prefix, so the subtree GLOB must be built in that same - # namespace — the one memory:// paths resolve into — or a - # project-relative pattern like "specs/*" would match nothing. - permalink_match = ( - canonical_memory_path_for_active_route( - active_project, - f"{generate_permalink(scope, split_extension=False)}/*", - include_project=ConfigManager().config.permalinks_include_project, - ) - if scope - else None - ) query = SearchQuery( - permalink_match=permalink_match, metadata_filters=metadata_filters, entity_types=[SearchItemType.ENTITY], ) diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index 0e50c002f..0d0c4b00f 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -8,6 +8,7 @@ import asyncio import re from pathlib import Path +from textwrap import dedent from types import SimpleNamespace import pytest @@ -530,6 +531,13 @@ async def test_find_rejects_bad_arguments(kwargs, message): ('label in "a,b",c', {"label": {"$in": ["a,b", "c"]}}), ('tags has "red, green"', {"tags": ["red, green"]}), ('name in "quoted"', {"name": {"$in": ["quoted"]}}), + # An unquoted value may not start with an operator character (a mis-spelled + # operator is the far likelier reading), so quoting is how a value that + # genuinely does start with one is expressed — scalars and list elements alike. + ('range=">=5"', {"range": ">=5"}), + ('range>"<=5"', {"range": {"$gt": "<=5"}}), + ('bound in ">=5","<=9"', {"bound": {"$in": [">=5", "<=9"]}}), + ('marks has "","=b"', {"marks": ["", "=b"]}), # Dot-paths address nested frontmatter and pass through verbatim. ("review.approved=true", {"review.approved": True}), # The one alias search_notes carries, so both surfaces accept one spelling. @@ -620,6 +628,48 @@ def test_duplicate_key_refusal_points_at_between(): posix_tools._parse_meta_predicates(["score>0.3", "score<0.8"]) +@pytest.mark.parametrize( + "predicate", + [ + # Symbol operators: the regex matches the longest SUPPORTED spelling, + # so the second operator character used to land at the head of the value. + "status==active", + "status=>active", + "status=>3", + "count>=>3", + "count<<1", + "count<=<1", + # Word operators fold the same way — everything after them is the value. + "priority in >high", + "tags has =security", + "score between >0.3,0.8", + "score between 0.3,<0.8", + ], +) +def test_malformed_operators_refuse_instead_of_folding_into_the_value(predicate): + """REGRESSION: a mis-spelled multi-character operator is a refusal, not a value. + + 'status==active' used to parse as {"status": "=active"} and 'count>>3' as + {"count": {"$gt": ">3"}} — filters for text no note carries, so the caller + got an empty (or worse, a non-empty but wrong) result set where the grammar + documents an unsupported-operator error. + """ + with pytest.raises(ValueError, match="unsupported predicate operator"): + posix_tools._parse_meta_predicates([predicate]) + + +def test_malformed_operator_refusal_teaches_the_grammar_and_the_escape(): + """The refusal names the supported set and the quoting escape, in one message.""" + with pytest.raises(ValueError) as excinfo: + posix_tools._parse_meta_predicates(["status==active"]) + + message = str(excinfo.value) + assert "unsupported predicate operator in 'status==active'" in message + assert "supported: = > >= < <= in has between" in message + assert 'quote the value as "=active"' in message + + @pytest.mark.parametrize( ("entity_metadata", "fields", "expected"), [ @@ -682,6 +732,43 @@ async def meta_notes(client, test_project): ) +@pytest_asyncio.fixture +async def diverged_permalink_notes(client, test_project): + """Seed two notes whose permalinks do not mirror their file paths. + + An explicit frontmatter `permalink:` is honored verbatim (#93), which is the + honest construction here: the shared test config sets + update_permalinks_on_move=True, so a move would NOT reproduce the split the + product's own default (False) produces in the field. + """ + await write_note( + title="Housed In Specs", + directory="specs", + content=dedent(""" + --- + permalink: archive/housed-in-specs + status: active + --- + + # Housed In Specs + """).strip(), + project=test_project.name, + ) + await write_note( + title="Housed In Archive", + directory="archive", + content=dedent(""" + --- + permalink: specs/housed-in-archive + status: active + --- + + # Housed In Archive + """).strip(), + project=test_project.name, + ) + + @pytest.mark.asyncio async def test_find_meta_returns_the_search_response_shape(client, test_project, meta_notes): result = await find(meta=["status=active"], project=test_project.name) @@ -729,23 +816,59 @@ async def test_find_meta_operators_select_the_right_notes( @pytest.mark.asyncio -@pytest.mark.parametrize( - ("path", "expected_titles"), - [ - ("/", {"Alpha Spec", "Gamma Note"}), - ("specs", {"Alpha Spec"}), - ("/specs", {"Alpha Spec"}), - # A spaced directory scopes by its slug, the same transform used at index time. - ("My Notes", {"Gamma Note"}), - ("nonexistent", set()), - ], -) -async def test_find_meta_scopes_by_path_subtree( - client, test_project, meta_notes, path, expected_titles -): +@pytest.mark.parametrize("path", ["/", ""]) +async def test_find_meta_answers_at_the_project_root(client, test_project, meta_notes, path): + """Both root spellings reach the metadata search; the predicates are the whole WHERE.""" result = await find(path, meta=["status=active"], project=test_project.name) - assert {row["title"] for row in result["results"]} == expected_titles + assert {row["title"] for row in result["results"]} == {"Alpha Spec", "Gamma Note"} + assert result["total_is_exact"] is True + + +@pytest.mark.asyncio +@pytest.mark.parametrize("path", ["specs", "/specs", "My Notes", "nonexistent"]) +async def test_find_meta_refuses_a_subtree_scope(client, test_project, meta_notes, path): + """CONTRACT (changed): `meta` answers only at a project root. + + This used to scope by permalink prefix and report the resulting count as + exact. The search API has no file-path filter — permalink_match is its only + path-shaped predicate — and a permalink is not a file path, so that scope + both dropped and admitted the wrong notes silently. Refuse instead of + answering a different question under an exact-looking total. + """ + with pytest.raises(ValueError) as excinfo: + await find(path, meta=["status=active"], project=test_project.name) + + message = str(excinfo.value) + assert f"find: 'meta' cannot be scoped to '{path}'" in message + assert "permalink, not by file path" in message + + +@pytest.mark.asyncio +async def test_permalink_prefix_scope_would_have_answered_wrongly( + client, test_project, diverged_permalink_notes +): + """EVIDENCE for the refusal above: a permalink does not track its file path. + + An explicit frontmatter `permalink:` is honored verbatim (#93) — the same + divergence a move produces under the default update_permalinks_on_move=False. + Here one note lives under specs/ but permalinks under archive/, and the other + is its mirror image, so a `find /specs --meta` scoped by permalink prefix + would have dropped the first, admitted the second, and called that total + exact. The root query sees both, and the rows show the split directly. + """ + result = await find(meta=["status=active"], project=test_project.name) + + rows = {row["title"]: row for row in result["results"]} + assert result["total_is_exact"] is True + assert set(rows) == {"Housed In Specs", "Housed In Archive"} + assert rows["Housed In Specs"]["file_path"] == "specs/Housed In Specs.md" + assert rows["Housed In Specs"]["permalink"].endswith("archive/housed-in-specs") + assert rows["Housed In Archive"]["file_path"] == "archive/Housed In Archive.md" + assert rows["Housed In Archive"]["permalink"].endswith("specs/housed-in-archive") + + with pytest.raises(ValueError, match="permalink, not by file path"): + await find("specs", meta=["status=active"], project=test_project.name) @pytest.mark.asyncio @@ -855,6 +978,30 @@ async def test_find_meta_quoted_list_element_keeps_its_comma(client, test_projec assert {row["title"] for row in result["results"]} == {"Comma Note"} +@pytest.mark.asyncio +async def test_find_meta_quoting_reaches_operator_prefixed_values(client, test_project): + """Quoting is the escape for a value that starts with an operator character. + + Unquoted, 'range=>=5' reads as a malformed operator and is refused; quoted, + the same value is matched literally — proved against the real stack so the + documented escape hatch is not just a parser property. + """ + await write_note( + title="Range Note", + directory="notes", + content="# Range Note", + project=test_project.name, + metadata={"range": ">=5"}, + ) + + with pytest.raises(ValueError, match="unsupported predicate operator"): + await find(meta=["range=>=5"], project=test_project.name) + + result = await find(meta=['range=">=5"'], project=test_project.name) + + assert {row["title"] for row in result["results"]} == {"Range Note"} + + @pytest.mark.asyncio async def test_find_meta_empty_list_is_not_an_unfiltered_search(client, test_project, meta_notes): """meta=[] must never widen to the whole project. @@ -1225,8 +1372,13 @@ async def test_find_qualified_path_routes_to_project( async def test_find_meta_qualified_path_routes_to_project( client, test_project, second_project, no_project_constraint ): - """The meta arm shares find's route resolution, so '/dir' scopes to - that project's subtree and nothing from the other project leaks in.""" + """The meta arm shares find's route resolution, so a bare '' routes + to that project's root and nothing from the other project leaks in. + + A project prefix is a mount point, not a subtree, so it survives the + subtree-scope refusal: 'second-project' strips to the empty project-relative + path. 'second-project/notes' does not, and is refused. + """ await write_note( title="Second Meta Note", directory="notes", @@ -1242,10 +1394,13 @@ async def test_find_meta_qualified_path_routes_to_project( metadata={"status": "active"}, ) - result = await find("second-project/notes", meta=["status=active"]) + result = await find("second-project", meta=["status=active"]) assert {row["title"] for row in result["results"]} == {"Second Meta Note"} + with pytest.raises(ValueError, match="permalink, not by file path"): + await find("second-project/notes", meta=["status=active"]) + @pytest.mark.asyncio async def test_find_meta_unqualified_refuses_in_multi_project_config( From 5635c51422b0f29566231df90f3e9f240c6bf065 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 11:57:50 -0500 Subject: [PATCH 03/12] feat(core): filter search by file path, restore find --meta subtree scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit find --meta scoped by permalink_match, which stops reflecting file_path once a note carries an explicit frontmatter permalink or is moved with update_permalinks_on_move=False (the default). The previous commit refused non-root scopes rather than answer wrongly; this adds the filter that lets them be answered correctly. SearchQuery.file_path_prefix is threaded through the service, the repository contract, both dialect implementations, and the vector and hybrid retrieval paths — including the post-filter and stable-pool refetch, which is where a filter wired only into FTS would leak. One shared helper builds the predicate, so parity is structural rather than two hand-written predicates that happen to agree: SUBSTR(file_path, 1, :len) = :prefix Chosen over LIKE deliberately. There is no pattern language to escape, and LIKE would be wrong twice: _ and % are ordinary characters in directory names, so my_notes would silently admit my-notes, and LIKE case-folds differently per backend, so one filter would answer two different questions. The compared prefix carries its trailing slash, so specs never admits specs-archive. The divergence test now proves the correct behavior instead of the refusal: a note living under specs/ is returned regardless of its permalink, and one whose permalink merely starts with specs/ is not. Reintroducing permalink scoping fails it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- .../api/v2/routers/search_router.py | 6 +- src/basic_memory/cli/commands/posix.py | 4 +- src/basic_memory/man/man1/find(1).md | 44 ++- src/basic_memory/mcp/tools/posix_tools.py | 92 ++--- .../repository/postgres_search_repository.py | 15 + .../repository/search_repository.py | 2 + .../repository/search_repository_base.py | 51 ++- .../repository/sqlite_search_repository.py | 19 +- src/basic_memory/schemas/search.py | 34 ++ src/basic_memory/services/search_service.py | 9 + tests/mcp/test_tool_posix.py | 118 ++++-- tests/repository/test_hybrid_fusion.py | 2 + tests/repository/test_rerank_pipeline.py | 1 + .../test_search_file_path_prefix.py | 353 ++++++++++++++++++ tests/repository/test_search_trace.py | 1 + tests/repository/test_semantic_search_base.py | 1 + tests/repository/test_semantic_vector_sync.py | 1 + tests/repository/test_vector_pagination.py | 2 + tests/repository/test_vector_threshold.py | 2 + tests/services/test_search_service.py | 29 ++ 20 files changed, 667 insertions(+), 119 deletions(-) create mode 100644 tests/repository/test_search_file_path_prefix.py diff --git a/src/basic_memory/api/v2/routers/search_router.py b/src/basic_memory/api/v2/routers/search_router.py index 1e903d9cb..eaf618b89 100644 --- a/src/basic_memory/api/v2/routers/search_router.py +++ b/src/basic_memory/api/v2/routers/search_router.py @@ -120,7 +120,11 @@ async def search( or query.permalink_match ), has_filters=bool( - query.note_types or query.entity_types or query.categories or query.metadata_filters + query.note_types + or query.entity_types + or query.categories + or query.metadata_filters + or query.file_path_prefix ), ): cache_key = ReadCacheKey( diff --git a/src/basic_memory/cli/commands/posix.py b/src/basic_memory/cli/commands/posix.py index faab39da7..db108bce0 100644 --- a/src/basic_memory/cli/commands/posix.py +++ b/src/basic_memory/cli/commands/posix.py @@ -746,7 +746,7 @@ def find( help=( "Metadata predicate, repeatable: 'status=active', 'confidence>0.6', " "'priority in high,critical', 'tags has security', 'score between 0.3,0.8'. " - "Queries the whole project — a PATH below the project root is refused" + "PATH still scopes the query, by file path" ), ), ] = None, @@ -774,7 +774,7 @@ def find( bm find --name "*.md" bm find /specs --depth 3 bm find /notes --name "auth*" --plain - bm find --meta "status=active" --meta "confidence>0.6" --fields title,priority + bm find /specs --meta "status=active" --meta "confidence>0.6" --fields title,priority """ # Deferred: loading the MCP tool stack at module import slows CLI startup (#886). from basic_memory.mcp.tools import find as mcp_find diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index 2256d26a0..3550bd5f1 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -20,7 +20,7 @@ bm find [PATH] [--name GLOB] [--depth N] [--page N] [--page-size N] [--json | --plain] [--project NAME | --project-id UUID] [--local | --cloud] -bm find [PROJECT] --meta PREDICATE [--meta PREDICATE ...] [--fields LIST] +bm find [PATH] --meta PREDICATE [--meta PREDICATE ...] [--fields LIST] [--page N] [--page-size N] [--json | --plain] [--project NAME | --project-id UUID] [--local | --cloud] ``` @@ -36,28 +36,34 @@ prints one path per line, find(1) style; `--json` (or piped output) emits the listing with pagination and totals. With `--meta`, find queries notes by their frontmatter instead: every -predicate must hold, across the whole project. The payload becomes the search -response shape (the same one `bm grep` returns), not the directory listing. -Totals are exact and every page is reachable. Non-markdown files carry no -frontmatter and are never metadata hits. +predicate must hold, and `PATH` still scopes the results — server-side, by +file-path prefix, so the totals are exact and every page is reachable. The +payload becomes the search response shape (the same one `bm grep` returns), +not the directory listing. Non-markdown files carry no frontmatter and are +never metadata hits. + +The scope matches the *file path* a note is indexed under, not its permalink. +A permalink stops mirroring its file path the moment a note pins `permalink:` +in its frontmatter, or is moved while `update_permalinks_on_move` is off (the +default), so scoping by permalink would drop notes that really are under the +named directory and admit notes that are not. The prefix matches on a +directory boundary and case-sensitively, identically on SQLite and Postgres: +`/specs` admits `specs/api.md`, never `specs-archive/api.md` or `Specs/api.md`, +and a `_` or `%` in a directory name is an ordinary character, not a wildcard. +`PATH` may also name a project (`bm find myproject --meta ...`) — that is a +routing prefix, a mount point rather than a subtree, and scopes to that +project's root. `--fields` is the SELECT to the predicates' WHERE: it adds a `fields` object to each hit carrying the named frontmatter values, so a filtered set can be tabulated without reading every note. A field a hit does not carry renders as null; the row is never dropped. -`--name`, `--depth`, and a `PATH` below the project root are all refused -alongside `--meta`, because the search API expresses none of them: no -filename glob, no depth bound, and no file-path filter. Its one path-shaped -predicate is a permalink prefix, and a permalink is not a file path — a note -that pins `permalink:` in its frontmatter, or that was moved while -`update_permalinks_on_move` is off (the default), keeps a permalink that no -longer says where the file lives. Scoping by it would drop notes that really -are under the named directory, admit notes that are not, and still report the -count as exact. Refusing beats misreporting the match set. `PATH` may still -name a project (`bm find myproject --meta ...`) — that is a routing prefix, -not a subtree. `--fields` without `--meta` is refused for the same honesty: -without predicates there is nothing to project. +`--name` and `--depth` are refused alongside `--meta`. The search API has no +filename glob, and its path scope is whole-subtree, where a depth bound is not +expressible — refusing beats silently ignoring either and misreporting the +match set. Scope with `PATH` instead. `--fields` without `--meta` is refused +for the same honesty: without predicates there is nothing to project. ## PREDICATE GRAMMAR @@ -92,7 +98,7 @@ quote one that genuinely does, as in `range=">=5"`. - **--name** — file-name glob, e.g. `"*.md"`; omitted matches everything. Cannot combine with `--meta` - **--depth** — recursion depth, 1-10 (default 10). A non-default depth - cannot combine with `--meta`. Nor can a `PATH` below the project root + cannot combine with `--meta` - **--meta** — frontmatter predicate, repeatable; see PREDICATE GRAMMAR. Switches the payload to the search response shape - **--fields** — comma-separated frontmatter fields to show per hit, e.g. @@ -106,7 +112,7 @@ bm find --name "*.md" bm find /specs --depth 3 bm find /notes --name "auth*" --plain bm find --meta "status=active" -bm find --meta "status=active" --meta "confidence>0.6" +bm find /specs --meta "status=active" --meta "confidence>0.6" bm find myproject --meta "status=active" bm find --meta "tags has security,oauth" --fields title,priority bm find --meta "status=active" --fields title --plain diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index efccb7538..392d9f25f 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -724,24 +724,24 @@ async def find( """Recursively list files by name glob, or query notes by frontmatter metadata. Without `meta`, this is a recursive directory listing. With `meta`, find - routes through the metadata search instead: predicates AND together over - the whole project, and non-markdown files (which carry no frontmatter) are - never hits. `name`, `depth`, and any `path` below the project root are all - refused alongside `meta`, because the search API expresses none of them: it - has no filename glob, no depth bound, and no file-path filter — it scopes - by permalink, and a permalink stops mirroring its file path the moment a - note pins one in frontmatter or is moved under the default - update_permalinks_on_move=False. Substituting any of the three would - misreport the match set while still calling the total exact. + routes through the metadata search instead: predicates AND together, `path` + still scopes the results — server-side, by the indexed file path, so a note + is scoped by where it actually lives rather than by a permalink that may no + longer say — and non-markdown files (which carry no frontmatter) are never + hits. `name` and `depth` are refused alongside `meta`: the search API has no + filename-glob or depth-bound facility, and silently ignoring either would + misreport the match set. Args: path: Directory to start from (default: project root). '/path' - routes into that project. With `meta`, only a project root is - addressable — a directory below it is refused (see above). + routes into that project. With `meta`, scopes the search to this + subtree (matched on a directory boundary, so "specs" never admits + "specs-archive/"). name: File-name glob to match, e.g. "*.md". None matches everything. - Cannot combine with `meta`. + Cannot combine with `meta` — scope with `path` instead. depth: How many levels to recurse (1-10, default: 10). A non-default - depth cannot combine with `meta`. + depth cannot combine with `meta` (the subtree scope is + all-or-nothing). page: Page number (1-indexed). page_size: Nodes per page. meta: Frontmatter metadata predicates, repeatable; every predicate must @@ -774,13 +774,11 @@ async def find( totals); each result carries a `fields` object when `fields` was requested. """ - # Combination rules, before any I/O. The metadata search matches slugified - # permalinks, where a filename glob has no faithful translation, and it - # takes no depth bound — refuse both rather than silently ignoring either. - # (The `path` scope is refused too, after routing, where the caller's - # project prefix has been stripped and "is this the project root?" can be - # answered.) `fields` is the SELECT to the predicates' WHERE; without - # predicates the directory listing stays byte-identical to today. + # Combination rules, before any I/O. The metadata search takes no filename + # glob and no depth bound, so `name` and `depth` are refused rather than + # silently ignored; `path` survives, because the search API does express a + # file-path subtree. `fields` is the SELECT to the predicates' WHERE; + # without predicates the directory listing stays byte-identical to today. if meta is not None: # Trigger: 'meta' present but carrying no predicates. # Why: an empty list parses to an empty filters dict, which is not None @@ -795,13 +793,13 @@ async def find( ) if name is not None: raise ValueError( - "find: 'name' cannot combine with 'meta' — the metadata search matches " - "slugified permalinks, not filenames" + "find: 'name' cannot combine with 'meta' — the metadata search has no " + "filename glob; scope with 'path' instead" ) if depth != _MAX_FIND_DEPTH: raise ValueError( - "find: 'depth' cannot combine with 'meta' — the metadata search takes " - "no depth bound" + "find: 'depth' cannot combine with 'meta' — the metadata search scopes " + "by whole subtree; scope with 'path' instead" ) if fields is not None: if meta is None: @@ -842,33 +840,13 @@ async def find( route = await resolve_project_path_route( path, project=project, project_id=project_id, context=context ) - # Trigger: a metadata query addressed below the project root. - # Why: the search API has no file-path filter. Its only path-shaped - # predicate is permalink_match, and a permalink is not a file path: - # a note that pins `permalink:` in frontmatter, or that was moved - # while update_permalinks_on_move is off (the default), keeps a - # permalink that no longer says where the file lives. Scoping by - # permalink prefix would therefore drop notes that really are under - # the requested directory and admit notes that are not — and report - # the resulting count as total_is_exact. - # Outcome: refuse the subtree scope naming the limitation, rather than - # answer a different question with an exact-looking total. The - # project-root form stays exact, so it is the offered path. - # - # route.path is the caller's input verbatim when no project prefix was - # recognized, so one strip covers both routed and raw spellings. - if route.path.strip("/"): - raise ValueError( - f"find: 'meta' cannot be scoped to '{path}' — the search API filters by " - "permalink, not by file path, and a permalink stops mirroring its file " - "path once a note pins one in frontmatter or is moved with " - "update_permalinks_on_move disabled (the default). A subtree scope would " - "omit matching files and admit unrelated ones while still reporting the " - "total as exact. Query the project root and filter the hits by their " - "file_path, or drop 'meta' for a path-scoped directory listing" - ) return await _find_by_metadata( route_project=route.project, + # route.path is the caller's input verbatim when no project prefix + # was recognized, so one strip covers both routed and raw spellings. + # A bare '' strips to "", the project root — a mount point, + # not a subtree. + scope=route.path.strip("/"), metadata_filters=metadata_filters, fields=fields, page=page, @@ -895,6 +873,7 @@ async def find( async def _find_by_metadata( *, route_project: Optional[str], + scope: str, metadata_filters: dict[str, Any], fields: Optional[list[str]], page: int, @@ -902,15 +881,18 @@ async def _find_by_metadata( project_id: Optional[str], context: Context | None, ) -> dict[str, Any]: - """find's metadata arm: one project-wide search call, plus field projection. + """find's metadata arm: one search call, plus per-hit field projection. The listing arm's counterpart to ``find_listing``; `find` has already bounded the pagination and refused `name` and `depth` against `meta`. - The predicates are the whole WHERE — `find` has already refused any scope - below the project root, because the search API offers no file-path filter - to express one honestly. So the total the server reports is the real match - count for the query that ran, and every page of it is reachable. + The path scope composes server-side as a file-path prefix — the indexed + `file_path`, which is where the note actually lives, not its permalink, + which stops mirroring that path once a note pins one in frontmatter or is + moved with update_permalinks_on_move disabled (the default). It ANDs with + the metadata filters in the same WHERE, so the total the server reports is + the real match count for the scope that ran, and every page of it is + reachable. """ async with get_project_client(route_project, context=context, project_id=project_id) as ( client, @@ -920,6 +902,8 @@ async def _find_by_metadata( from basic_memory.mcp.clients import KnowledgeClient, SearchClient query = SearchQuery( + # "" is the project root: the predicates are then the whole WHERE. + file_path_prefix=scope or None, metadata_filters=metadata_filters, entity_types=[SearchItemType.ENTITY], ) diff --git a/src/basic_memory/repository/postgres_search_repository.py b/src/basic_memory/repository/postgres_search_repository.py index 85e6d2e70..ba51e6f2d 100644 --- a/src/basic_memory/repository/postgres_search_repository.py +++ b/src/basic_memory/repository/postgres_search_repository.py @@ -26,6 +26,7 @@ from basic_memory.repository.search_repository_base import ( SearchRepositoryBase, VectorChunkState, + file_path_prefix_condition, ) from basic_memory.repository.search_trace import ( SearchTraceCollector, @@ -968,6 +969,7 @@ async def _build_fts_query_parts( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, allow_relaxed: bool = False, ) -> tuple[str, str, dict[str, Any], str, str]: """Build Postgres FTS FROM/WHERE params shared by search and count.""" @@ -1130,6 +1132,13 @@ async def _build_fts_query_parts( else: conditions.append("search_index.permalink = :permalink") + # Handle directory subtree scope. The predicate is built by the shared + # helper so Postgres and SQLite scope by the identical rule; see + # file_path_prefix_condition for the boundary and escaping reasoning. + subtree_condition = file_path_prefix_condition(file_path_prefix, params) + if subtree_condition is not None: + conditions.append(subtree_condition) + # Handle search item type filter (parameterized for defense-in-depth) if search_item_types: type_placeholders = [] @@ -1341,6 +1350,7 @@ async def search( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, limit: int = 10, @@ -1362,6 +1372,7 @@ async def search( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, retrieval_mode=retrieval_mode, min_similarity=min_similarity, limit=limit, @@ -1388,6 +1399,7 @@ async def search( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, allow_relaxed=allow_relaxed, ) @@ -1534,6 +1546,7 @@ async def count( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, allow_relaxed: bool = False, @@ -1550,6 +1563,7 @@ async def count( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, retrieval_mode=retrieval_mode, min_similarity=min_similarity, ) @@ -1570,6 +1584,7 @@ async def count( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, allow_relaxed=allow_relaxed, ) sql = f"SELECT COUNT(*) FROM {from_clause} WHERE {where_clause}" diff --git a/src/basic_memory/repository/search_repository.py b/src/basic_memory/repository/search_repository.py index df67fd113..ace8e329e 100644 --- a/src/basic_memory/repository/search_repository.py +++ b/src/basic_memory/repository/search_repository.py @@ -79,6 +79,7 @@ async def search( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, limit: int = 10, @@ -102,6 +103,7 @@ async def count( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, allow_relaxed: bool = False, diff --git a/src/basic_memory/repository/search_repository_base.py b/src/basic_memory/repository/search_repository_base.py index 9b6d7e205..96c4aa98c 100644 --- a/src/basic_memory/repository/search_repository_base.py +++ b/src/basic_memory/repository/search_repository_base.py @@ -77,7 +77,11 @@ VectorChunkState, ) from basic_memory.runtime.vector_sync import VectorSyncBatchResult -from basic_memory.schemas.search import SearchItemType, SearchRetrievalMode +from basic_memory.schemas.search import ( + SearchItemType, + SearchRetrievalMode, + normalize_file_path_prefix, +) from basic_memory.utils import ensure_timezone_aware # --- Semantic search constants --- @@ -150,6 +154,37 @@ def __post_init__(self) -> None: object.__setattr__(self, "updated_at", ensure_timezone_aware(updated_at)) +def file_path_prefix_condition( + file_path_prefix: Optional[str], + params: Dict[str, Any], +) -> Optional[str]: + """Build the SQL scoping search rows to one directory subtree of the project. + + One implementation, shared verbatim by both backends: a subtree scope that + means different things on SQLite and Postgres would report an exact total + for a match set the other dialect never produces. + + Boundary: the compared prefix carries its trailing separator, so "specs" + admits "specs/api.md" and never "specs-archive/api.md". + + Why an explicit-length comparison rather than ``file_path LIKE 'specs/%'``: + LIKE reads "_" and "%" as wildcards and both are ordinary characters in a + directory name, so "my_notes" would silently also admit "my-notes"; and + LIKE case-folds differently per backend — SQLite's is ASCII-case-insensitive + while Postgres's is case-sensitive — so one filter would answer two + different questions. SUBSTR equality has no pattern language to escape and + compares under each backend's deterministic default text collation, which is + byte equality on both, so the dialects match exactly the same rows. + """ + normalized = normalize_file_path_prefix(file_path_prefix) + if normalized is None: + return None + prefix = f"{normalized}/" + params["file_path_prefix"] = prefix + params["file_path_prefix_length"] = len(prefix) + return "SUBSTR(search_index.file_path, 1, :file_path_prefix_length) = :file_path_prefix" + + async def purge_stale_search_index_rows( session_maker: async_sessionmaker[AsyncSession], project_id: int, @@ -308,6 +343,7 @@ async def search( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[Dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, limit: int = 10, @@ -328,6 +364,7 @@ async def search( search_item_types: Filter by SearchItemType (ENTITY, OBSERVATION, RELATION) categories: Filter observations by exact category (e.g. "requirement") metadata_filters: Structured frontmatter metadata filters + file_path_prefix: Directory subtree scope, matched against file_path limit: Maximum results to return offset: Number of results to skip @@ -351,6 +388,7 @@ async def count( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[Dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, allow_relaxed: bool = False, @@ -1903,6 +1941,7 @@ async def _dispatch_retrieval_mode( search_item_types: Optional[List[SearchItemType]], categories: Optional[List[str]], metadata_filters: Optional[dict[str, Any]], + file_path_prefix: Optional[str], retrieval_mode: SearchRetrievalMode, min_similarity: Optional[float] = None, limit: int, @@ -1938,6 +1977,7 @@ async def _dispatch_retrieval_mode( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, min_similarity=min_similarity, limit=limit, offset=offset, @@ -1959,6 +1999,7 @@ async def _dispatch_retrieval_mode( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, min_similarity=min_similarity, limit=limit, offset=offset, @@ -2137,6 +2178,7 @@ async def _search_vector_only( search_item_types: Optional[List[SearchItemType]], categories: Optional[List[str]], metadata_filters: Optional[dict[str, Any]], + file_path_prefix: Optional[str], min_similarity: Optional[float] = None, limit: int, offset: int, @@ -2324,6 +2366,7 @@ def _log_vector_summary() -> None: search_item_types, categories, metadata_filters, + file_path_prefix, ] ) @@ -2338,6 +2381,7 @@ def _log_vector_summary() -> None: search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, retrieval_mode=SearchRetrievalMode.FTS, limit=VECTOR_FILTER_SCAN_LIMIT, offset=0, @@ -2401,6 +2445,7 @@ def _log_vector_summary() -> None: search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, min_similarity=min_similarity, limit=stable_candidate_limit, offset=0, @@ -2473,6 +2518,7 @@ async def _search_hybrid( search_item_types: Optional[List[SearchItemType]], categories: Optional[List[str]], metadata_filters: Optional[dict[str, Any]], + file_path_prefix: Optional[str], min_similarity: Optional[float] = None, limit: int, offset: int, @@ -2511,6 +2557,7 @@ async def _search_hybrid( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, retrieval_mode=SearchRetrievalMode.FTS, limit=candidate_limit, offset=0, @@ -2529,6 +2576,7 @@ async def _search_hybrid( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, min_similarity=min_similarity, limit=candidate_limit, offset=0, @@ -2679,6 +2727,7 @@ def _materialize(entry: tuple[SearchIndexKey, float]) -> SearchIndexRow: search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, min_similarity=min_similarity, limit=stable_candidate_limit, offset=0, diff --git a/src/basic_memory/repository/sqlite_search_repository.py b/src/basic_memory/repository/sqlite_search_repository.py index 296a289a4..b88565515 100644 --- a/src/basic_memory/repository/sqlite_search_repository.py +++ b/src/basic_memory/repository/sqlite_search_repository.py @@ -28,7 +28,10 @@ from basic_memory.repository.rerank_provider_factory import create_rerank_provider from basic_memory.repository.search_index_row import SearchIndexRow from basic_memory.repository.search_query import relaxed_query_words -from basic_memory.repository.search_repository_base import SearchRepositoryBase +from basic_memory.repository.search_repository_base import ( + SearchRepositoryBase, + file_path_prefix_condition, +) from basic_memory.repository.script_ngrams import analyze_script_query from basic_memory.repository.search_trace import ( SearchTraceCollector, @@ -782,6 +785,7 @@ async def _build_fts_query_parts( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, ) -> tuple[str, str, dict[str, Any], str, str]: """Build SQLite FTS FROM/WHERE params shared by search and count.""" conditions = [] @@ -868,6 +872,13 @@ async def _build_fts_query_parts( params["permalink"] = permalink_text match_conditions.append("search_index.permalink MATCH :permalink") + # Handle directory subtree scope. The predicate is built by the shared + # helper so SQLite and Postgres scope by the identical rule; see + # file_path_prefix_condition for the boundary and escaping reasoning. + subtree_condition = file_path_prefix_condition(file_path_prefix, params) + if subtree_condition is not None: + conditions.append(subtree_condition) + # Handle entity type filter (parameterized for defense-in-depth) if search_item_types: type_placeholders = [] @@ -1055,6 +1066,7 @@ async def search( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, limit: int = 10, @@ -1082,6 +1094,7 @@ async def search( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, retrieval_mode=retrieval_mode, min_similarity=min_similarity, limit=limit, @@ -1108,6 +1121,7 @@ async def search( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, ) # set limit on search query @@ -1239,6 +1253,7 @@ async def count( search_item_types: Optional[List[SearchItemType]] = None, categories: Optional[List[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, allow_relaxed: bool = False, @@ -1255,6 +1270,7 @@ async def count( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, retrieval_mode=retrieval_mode, min_similarity=min_similarity, ) @@ -1275,6 +1291,7 @@ async def count( search_item_types=search_item_types, categories=categories, metadata_filters=metadata_filters, + file_path_prefix=file_path_prefix, ) sql = f"SELECT COUNT(*) FROM {from_clause} WHERE {where_clause}" logger.trace(f"Count {sql} params: {params}") diff --git a/src/basic_memory/schemas/search.py b/src/basic_memory/schemas/search.py index d751239b4..a4207e45e 100644 --- a/src/basic_memory/schemas/search.py +++ b/src/basic_memory/schemas/search.py @@ -30,6 +30,22 @@ class SearchRetrievalMode(str, Enum): HYBRID = "hybrid" +def normalize_file_path_prefix(value: Optional[str]) -> Optional[str]: + """Reduce a directory scope to its bare project-relative spelling. + + "", "/", and " " all name the project root — no subtree scope — and must + collapse to None. "/" in particular is a non-empty string, so left as-is it + would read as criteria to the service's has-criteria check while + contributing no predicate: a query that filters nothing yet reports its + total as if it had. "/specs/" and "specs" are the same scope, so they too + normalize to the one value the SQL predicate and the criteria check share. + """ + if value is None: + return None + normalized = value.strip().strip("/") + return normalized or None + + class SearchQuery(BaseModel): """Search query parameters. @@ -45,6 +61,7 @@ class SearchQuery(BaseModel): - categories: Limit observation results to exact category matches (e.g. "requirement") - after_date: Only items after date - metadata_filters: Structured frontmatter filters (field -> value) + - file_path_prefix: Limit to one directory subtree of the project - tags: Convenience frontmatter tag filter - status: Convenience frontmatter status filter @@ -67,6 +84,10 @@ class SearchQuery(BaseModel): categories: Optional[List[str]] = None # Filter observations by exact category after_date: Optional[Union[datetime, str]] = None # Time-based filter metadata_filters: Optional[dict[str, Any]] = None # Structured frontmatter filters + # Directory subtree scope, matched against the indexed file_path — not the + # permalink, which stops mirroring its file path once a note pins one in + # frontmatter or is moved with update_permalinks_on_move disabled. + file_path_prefix: Optional[str] = None tags: Optional[List[str]] = None # Convenience tag filter status: Optional[str] = None # Convenience status filter retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS @@ -88,6 +109,17 @@ def normalize_note_types(cls, values: Optional[List[str]]) -> Optional[List[str] return None return [normalize_note_type(value) for value in values] + @field_validator("file_path_prefix") + @classmethod + def normalize_scope(cls, value: Optional[str]) -> Optional[str]: + """Collapse the root spellings onto "no scope" at the boundary. + + Parsing once here means every consumer — the criteria check, the + executed-criteria description, and the SQL predicate — reads the same + value instead of each rediscovering that "/" is not a subtree. + """ + return normalize_file_path_prefix(value) + def no_criteria(self) -> bool: text_is_empty = self.text is None or (isinstance(self.text, str) and not self.text.strip()) metadata_is_empty = not self.metadata_filters @@ -106,6 +138,8 @@ def no_criteria(self) -> bool: and entity_types_is_empty and categories_is_empty and metadata_is_empty + # Normalized above, so a bare "/" never counts as a scope here. + and self.file_path_prefix is None and tags_is_empty and status_is_empty ) diff --git a/src/basic_memory/services/search_service.py b/src/basic_memory/services/search_service.py index 984ad5c8b..588090365 100644 --- a/src/basic_memory/services/search_service.py +++ b/src/basic_memory/services/search_service.py @@ -51,6 +51,7 @@ class PreparedSearchQuery: categories: list[str] | None after_date: datetime | None metadata_filters: dict[str, Any] | None + file_path_prefix: str | None retrieval_mode: SearchRetrievalMode min_similarity: float | None @@ -109,6 +110,7 @@ def quoted(value: str | None) -> str | None: "after_date": prepared.after_date, "categories": list(prepared.categories) if prepared.categories else None, "metadata_filters": dict(prepared.metadata_filters) if prepared.metadata_filters else None, + "file_path_prefix": quoted(prepared.file_path_prefix), } return " ".join(f"{name}={value}" for name, value in criteria.items() if value is not None) @@ -224,6 +226,7 @@ def prepare_query(self, query: SearchQuery) -> PreparedSearchQuery | None: categories=query.categories, after_date=after_date, metadata_filters=metadata_filters, + file_path_prefix=query.file_path_prefix, retrieval_mode=query.retrieval_mode or SearchRetrievalMode.FTS, min_similarity=query.min_similarity, ) @@ -238,6 +241,8 @@ def prepare_query(self, query: SearchQuery) -> PreparedSearchQuery | None: or prepared.categories or prepared.after_date or prepared.metadata_filters + # Normalized by SearchQuery, so only a real subtree reaches here. + or prepared.file_path_prefix ) if not has_criteria: logger.debug("no criteria passed to query") @@ -252,6 +257,7 @@ def _prepared_has_filters(prepared: PreparedSearchQuery) -> bool: or prepared.search_item_types or prepared.categories or prepared.after_date + or prepared.file_path_prefix ) async def _include_legacy_note_type_spellings( @@ -305,6 +311,7 @@ async def _search_repository( categories=prepared.categories, after_date=prepared.after_date, metadata_filters=prepared.metadata_filters, + file_path_prefix=prepared.file_path_prefix, retrieval_mode=prepared.retrieval_mode, min_similarity=prepared.min_similarity, limit=limit, @@ -322,6 +329,7 @@ async def _search_repository( categories=prepared.categories, after_date=prepared.after_date, metadata_filters=prepared.metadata_filters, + file_path_prefix=prepared.file_path_prefix, retrieval_mode=prepared.retrieval_mode, min_similarity=prepared.min_similarity, limit=limit, @@ -348,6 +356,7 @@ async def _count_repository( categories=prepared.categories, after_date=prepared.after_date, metadata_filters=prepared.metadata_filters, + file_path_prefix=prepared.file_path_prefix, retrieval_mode=prepared.retrieval_mode, min_similarity=prepared.min_similarity, allow_relaxed=allow_relaxed, diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index 0d0c4b00f..a683b0d4b 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -740,6 +740,10 @@ async def diverged_permalink_notes(client, test_project): honest construction here: the shared test config sets update_permalinks_on_move=True, so a move would NOT reproduce the split the product's own default (False) produces in the field. + + One note lives under specs/ but permalinks under archive/; the other is its + mirror image. Any scope built from permalinks answers this pair exactly + backwards, which is what the regression below pins down. """ await write_note( title="Housed In Specs", @@ -826,49 +830,73 @@ async def test_find_meta_answers_at_the_project_root(client, test_project, meta_ @pytest.mark.asyncio -@pytest.mark.parametrize("path", ["specs", "/specs", "My Notes", "nonexistent"]) -async def test_find_meta_refuses_a_subtree_scope(client, test_project, meta_notes, path): - """CONTRACT (changed): `meta` answers only at a project root. +@pytest.mark.parametrize( + ("path", "expected_titles"), + [ + ("specs", {"Alpha Spec", "Beta Spec"}), + ("/specs", {"Alpha Spec", "Beta Spec"}), + # A spaced directory name is a file path, not a slug: it scopes verbatim. + ("My Notes", {"Gamma Note"}), + ("nonexistent", set()), + ], +) +async def test_find_meta_scopes_by_path_subtree( + client, test_project, meta_notes, path, expected_titles +): + """`path` narrows the metadata query to one directory, server-side.""" + result = await find(path, meta=["note_type=note"], project=test_project.name) - This used to scope by permalink prefix and report the resulting count as - exact. The search API has no file-path filter — permalink_match is its only - path-shaped predicate — and a permalink is not a file path, so that scope - both dropped and admitted the wrong notes silently. Refuse instead of - answering a different question under an exact-looking total. - """ - with pytest.raises(ValueError) as excinfo: - await find(path, meta=["status=active"], project=test_project.name) + assert {row["title"] for row in result["results"]} == expected_titles + assert result["total"] == len(expected_titles) + assert result["total_is_exact"] is True - message = str(excinfo.value) - assert f"find: 'meta' cannot be scoped to '{path}'" in message - assert "permalink, not by file path" in message + +@pytest.mark.asyncio +async def test_find_meta_scope_stops_at_a_directory_boundary(client, test_project, meta_notes): + """A sibling directory whose name starts with the scope stays out.""" + await write_note( + title="Archived Spec", + directory="specs-archive", + content="# Archived Spec", + project=test_project.name, + metadata={"status": "active"}, + ) + + result = await find("specs", meta=["status=active"], project=test_project.name) + + assert {row["title"] for row in result["results"]} == {"Alpha Spec"} @pytest.mark.asyncio -async def test_permalink_prefix_scope_would_have_answered_wrongly( +async def test_scope_follows_the_file_path_not_the_permalink( client, test_project, diverged_permalink_notes ): - """EVIDENCE for the refusal above: a permalink does not track its file path. - - An explicit frontmatter `permalink:` is honored verbatim (#93) — the same - divergence a move produces under the default update_permalinks_on_move=False. - Here one note lives under specs/ but permalinks under archive/, and the other - is its mirror image, so a `find /specs --meta` scoped by permalink prefix - would have dropped the first, admitted the second, and called that total - exact. The root query sees both, and the rows show the split directly. + """REGRESSION: `find /specs --meta` scopes by where the file lives. + + A permalink stops mirroring its file path once a note pins one in + frontmatter (#93) or is moved with update_permalinks_on_move disabled (the + default). A scope built from permalink prefixes therefore answers this pair + exactly backwards: it would drop "Housed In Specs", which really is under + specs/, and admit "Housed In Archive", which is not — while still reporting + that count as exact. Scoping by the indexed file_path answers the question + the caller asked, so reintroducing a permalink-based scope fails here. """ - result = await find(meta=["status=active"], project=test_project.name) + result = await find("specs", meta=["status=active"], project=test_project.name) rows = {row["title"]: row for row in result["results"]} + assert set(rows) == {"Housed In Specs"} + assert result["total"] == 1 assert result["total_is_exact"] is True - assert set(rows) == {"Housed In Specs", "Housed In Archive"} + # The hit is the one whose *file* is under specs/, and its permalink is not. assert rows["Housed In Specs"]["file_path"] == "specs/Housed In Specs.md" assert rows["Housed In Specs"]["permalink"].endswith("archive/housed-in-specs") - assert rows["Housed In Archive"]["file_path"] == "archive/Housed In Archive.md" - assert rows["Housed In Archive"]["permalink"].endswith("specs/housed-in-archive") - with pytest.raises(ValueError, match="permalink, not by file path"): - await find("specs", meta=["status=active"], project=test_project.name) + # The mirror image: permalinked under specs/, but housed under archive/. + archive = await find("archive", meta=["status=active"], project=test_project.name) + archive_rows = {row["title"]: row for row in archive["results"]} + assert set(archive_rows) == {"Housed In Archive"} + assert archive_rows["Housed In Archive"]["file_path"] == "archive/Housed In Archive.md" + assert archive_rows["Housed In Archive"]["permalink"].endswith("specs/housed-in-archive") @pytest.mark.asyncio @@ -916,10 +944,10 @@ async def test_find_meta_projects_requested_fields(client, test_project, meta_no @pytest.mark.parametrize( ("kwargs", "message"), [ - # The metadata search matches slugified permalinks, so a filename glob - # has no faithful translation — refuse rather than silently ignore it. + # The search API has no filename-glob facility, so a `name` pattern has + # no faithful translation — refuse rather than silently ignore it. ({"name": "*.md", "meta": ["status=active"]}, "'name' cannot combine with 'meta'"), - # Permalink-prefix scope is whole-subtree; a depth bound is inexpressible. + # The file-path scope is whole-subtree; a depth bound is inexpressible. ({"depth": 3, "meta": ["status=active"]}, "'depth' cannot combine with 'meta'"), ({"fields": ["title"]}, "'fields' requires 'meta' predicates"), ({"meta": ["status=active"], "fields": []}, "must be non-empty"), @@ -1372,12 +1400,13 @@ async def test_find_qualified_path_routes_to_project( async def test_find_meta_qualified_path_routes_to_project( client, test_project, second_project, no_project_constraint ): - """The meta arm shares find's route resolution, so a bare '' routes - to that project's root and nothing from the other project leaks in. + """The meta arm shares find's route resolution, so '/dir' scopes to + that project's subtree and nothing from the other project leaks in. - A project prefix is a mount point, not a subtree, so it survives the - subtree-scope refusal: 'second-project' strips to the empty project-relative - path. 'second-project/notes' does not, and is refused. + A project prefix is a mount point, not a subtree: a bare 'second-project' + strips to the empty project-relative path and answers at that project's + root, while 'second-project/notes' strips to the 'notes' subtree of the + same project. """ await write_note( title="Second Meta Note", @@ -1394,12 +1423,19 @@ async def test_find_meta_qualified_path_routes_to_project( metadata={"status": "active"}, ) - result = await find("second-project", meta=["status=active"]) + await write_note( + title="Second Root Note", + directory="specs", + content="# Second Root Note", + project="second-project", + metadata={"status": "active"}, + ) - assert {row["title"] for row in result["results"]} == {"Second Meta Note"} + root = await find("second-project", meta=["status=active"]) + scoped = await find("second-project/notes", meta=["status=active"]) - with pytest.raises(ValueError, match="permalink, not by file path"): - await find("second-project/notes", meta=["status=active"]) + assert {row["title"] for row in root["results"]} == {"Second Meta Note", "Second Root Note"} + assert {row["title"] for row in scoped["results"]} == {"Second Meta Note"} @pytest.mark.asyncio diff --git a/tests/repository/test_hybrid_fusion.py b/tests/repository/test_hybrid_fusion.py index 2042814ec..4a438ae6b 100644 --- a/tests/repository/test_hybrid_fusion.py +++ b/tests/repository/test_hybrid_fusion.py @@ -81,6 +81,7 @@ async def search( search_item_types: Optional[list[SearchItemType]] = None, categories: Optional[list[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, limit: int = 10, @@ -160,6 +161,7 @@ def _fake_embedding_provider() -> EmbeddingProvider: search_item_types=None, categories=None, metadata_filters=None, + file_path_prefix=None, limit=10, offset=0, ) diff --git a/tests/repository/test_rerank_pipeline.py b/tests/repository/test_rerank_pipeline.py index dfc8a61d0..fcef636fa 100644 --- a/tests/repository/test_rerank_pipeline.py +++ b/tests/repository/test_rerank_pipeline.py @@ -793,6 +793,7 @@ async def deep_page(offset: int) -> list[SearchIndexRow]: search_item_types=None, categories=None, metadata_filters=None, + file_path_prefix=None, limit=1, offset=offset, ) diff --git a/tests/repository/test_search_file_path_prefix.py b/tests/repository/test_search_file_path_prefix.py new file mode 100644 index 000000000..611edee50 --- /dev/null +++ b/tests/repository/test_search_file_path_prefix.py @@ -0,0 +1,353 @@ +"""Dialect contract for the search_index file-path subtree filter. + +Every test here runs against whichever backend the session is configured for — +SQLite by default, PostgreSQL under BASIC_MEMORY_TEST_POSTGRES=1 — through the +shared `search_repository` fixture. The point is that the two dialects must +agree row for row: a subtree scope that means "specs/ and everything under it" +on one backend and something wider on the other would report an exact total for +a match set the other backend never produces. +""" + +from datetime import datetime, timezone +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest + +from basic_memory import db +from basic_memory.models.knowledge import Entity +from basic_memory.repository.search_index_row import SearchIndexRow +from basic_memory.repository.search_repository_base import file_path_prefix_condition +from basic_memory.schemas.search import ( + SearchItemType, + SearchRetrievalMode, + normalize_file_path_prefix, +) + +# file_path -> (title, status). Every decoy here is a real way a naive predicate +# leaks: a sibling directory sharing the scope's name as a prefix, a "_" or "%" +# read as a LIKE wildcard, and a case variant that only one dialect's LIKE folds. +SEEDED_NOTES: dict[str, tuple[str, str]] = { + "specs/alpha.md": ("Alpha", "active"), + "specs/nested/beta.md": ("Beta", "draft"), + "specs-archive/gamma.md": ("Gamma", "active"), + "Specs/delta.md": ("Delta", "active"), + "my_notes/epsilon.md": ("Epsilon", "active"), + "my-notes/zeta.md": ("Zeta", "active"), + "100%/eta.md": ("Eta", "active"), + "100pct/theta.md": ("Theta", "active"), +} + + +async def _index_note( + search_repository, + session_maker, + file_path: str, + *, + title: str | None = None, + status: str | None = None, + content: str = "subtree scope fixture", +) -> Entity: + """Index one entity and its search row at an exact file_path.""" + seeded_title, seeded_status = SEEDED_NOTES.get(file_path, ("", "active")) + title = title if title is not None else seeded_title + status = status if status is not None else seeded_status + now = datetime.now(timezone.utc) + permalink = file_path.removesuffix(".md") + + async with db.scoped_session(session_maker) as session: + entity = Entity( + project_id=search_repository.project_id, + title=title, + note_type="note", + permalink=permalink, + file_path=file_path, + content_type="text/markdown", + entity_metadata={"status": status}, + created_at=now, + updated_at=now, + ) + session.add(entity) + await session.flush() + + await search_repository.index_item( + SearchIndexRow( + project_id=search_repository.project_id, + id=entity.id, + type=SearchItemType.ENTITY.value, + title=entity.title, + content_stems=content, + content_snippet=content, + permalink=entity.permalink, + file_path=entity.file_path, + entity_id=entity.id, + metadata={"note_type": entity.note_type}, + created_at=entity.created_at, + updated_at=entity.updated_at, + ) + ) + return entity + + +@pytest.fixture +async def seeded_paths(search_repository, session_maker) -> dict[str, int]: + """Index every seeded note; yields file_path -> search_index row id.""" + return { + file_path: (await _index_note(search_repository, session_maker, file_path)).id + for file_path in SEEDED_NOTES + } + + +async def _titles(search_repository, **kwargs) -> set[str]: + rows = await search_repository.search(limit=100, **kwargs) + return {row.title for row in rows} + + +@pytest.mark.asyncio +async def test_scopes_to_the_named_subtree(search_repository, seeded_paths): + """A prefix admits the directory's own files and everything beneath it.""" + assert await _titles(search_repository, file_path_prefix="specs") == {"Alpha", "Beta"} + + +@pytest.mark.asyncio +async def test_scopes_to_a_nested_subtree(search_repository, seeded_paths): + """Multi-segment prefixes address a directory further down the tree.""" + assert await _titles(search_repository, file_path_prefix="specs/nested") == {"Beta"} + + +@pytest.mark.asyncio +async def test_prefix_matches_only_on_a_directory_boundary(search_repository, seeded_paths): + """REGRESSION: "specs" must not admit the sibling "specs-archive/". + + The compared prefix carries its trailing separator precisely so a directory + whose name merely starts with the scope stays out. + """ + assert "Gamma" not in await _titles(search_repository, file_path_prefix="specs") + assert await _titles(search_repository, file_path_prefix="specs-archive") == {"Gamma"} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("scope", "expected", "excluded"), + [ + # "_" is LIKE's single-character wildcard; here it is a directory name. + ("my_notes", {"Epsilon"}, "Zeta"), + # "%" is LIKE's any-length wildcard; unescaped, "100%/" would also admit + # "100pct/" (and anything else starting with "100"). + ("100%", {"Eta"}, "Theta"), + ], +) +async def test_wildcard_characters_in_a_directory_name_are_literal( + search_repository, seeded_paths, scope, expected, excluded +): + """REGRESSION: a directory named with "_" or "%" is not a pattern.""" + titles = await _titles(search_repository, file_path_prefix=scope) + + assert titles == expected + assert excluded not in titles + + +@pytest.mark.asyncio +async def test_prefix_is_case_sensitive_on_both_backends(search_repository, seeded_paths): + """CONTRACT: casing decides membership identically on SQLite and Postgres. + + SQLite's LIKE is ASCII-case-insensitive and Postgres's is case-sensitive, so + a LIKE-based scope would put "Specs/delta.md" inside "specs" on one backend + and outside it on the other. The shared predicate compares the stored bytes. + """ + assert await _titles(search_repository, file_path_prefix="specs") == {"Alpha", "Beta"} + assert await _titles(search_repository, file_path_prefix="Specs") == {"Delta"} + + +@pytest.mark.asyncio +@pytest.mark.parametrize("scope", ["specs", "/specs", "specs/", "/specs/", " specs "]) +async def test_scope_spellings_normalize_to_one_query(search_repository, seeded_paths, scope): + """Leading, trailing, and surrounding whitespace/slashes name one subtree.""" + assert await _titles(search_repository, file_path_prefix=scope) == {"Alpha", "Beta"} + + +@pytest.mark.asyncio +@pytest.mark.parametrize("scope", [None, "", "/", " "]) +async def test_root_spellings_apply_no_scope(search_repository, seeded_paths, scope): + """The root is the absence of a subtree predicate, not a prefix of "/".""" + titles = await _titles( + search_repository, + file_path_prefix=scope, + search_item_types=[SearchItemType.ENTITY], + ) + + assert titles == {title for title, _status in SEEDED_NOTES.values()} + + +@pytest.mark.asyncio +async def test_scope_composes_with_metadata_filters_and_counts_agree( + search_repository, seeded_paths +): + """The scope ANDs into the same WHERE, so count() describes the page's query. + + A total drawn from a different WHERE than the page is the exact failure the + file-path filter exists to prevent: it would advertise pages that hold + nothing and hide pages that hold matches. + """ + kwargs = {"file_path_prefix": "specs", "metadata_filters": {"status": "active"}} + + assert await _titles(search_repository, **kwargs) == {"Alpha"} + assert await search_repository.count(**kwargs) == 1 + # Unscoped, the same predicate also reaches every decoy directory. + active_notes = sum(1 for _title, status in SEEDED_NOTES.values() if status == "active") + assert await search_repository.count(metadata_filters={"status": "active"}) == active_notes + + +@pytest.mark.asyncio +async def test_scope_composes_with_text_search(search_repository, seeded_paths): + """The scope narrows a full-text query rather than replacing it.""" + scoped = await _titles(search_repository, search_text="subtree", file_path_prefix="specs") + + assert scoped == {"Alpha", "Beta"} + + +@pytest.mark.asyncio +async def test_scope_composes_with_an_unsegmented_script_query(search_repository, session_maker): + """The scope survives the script-ngram query shape both backends build. + + A CJK query plus metadata filters takes each backend's most rearranged FROM + clause — SQLite ranks the MATCH inside a derived table aliased back to + `search_index`, Postgres joins a script-ngram candidate set — so a predicate + that referenced the base table by any other name would fail here rather than + at some later runtime. + """ + await _index_note( + search_repository, + session_maker, + "specs/cjk.md", + title="Scoped Script", + content="適者生存", + ) + await _index_note( + search_repository, + session_maker, + "notes/cjk.md", + title="Unscoped Script", + content="適者生存", + ) + + titles = await _titles( + search_repository, + search_text="適者生存", + file_path_prefix="specs", + metadata_filters={"status": "active"}, + ) + + assert titles == {"Scoped Script"} + + +@pytest.mark.asyncio +async def test_count_matches_the_paged_rows(search_repository, seeded_paths): + """Pagination over a scoped query stays reachable end to end.""" + total = await search_repository.count(file_path_prefix="specs") + first = await search_repository.search(file_path_prefix="specs", limit=1, offset=0) + second = await search_repository.search(file_path_prefix="specs", limit=1, offset=1) + + assert total == 2 + assert {first[0].title, second[0].title} == {"Alpha", "Beta"} + + +@pytest.mark.asyncio +async def test_nonexistent_scope_is_empty_not_unfiltered(search_repository, seeded_paths): + """A directory with no notes answers zero rows, never the whole project.""" + assert await _titles(search_repository, file_path_prefix="nowhere") == set() + assert await search_repository.count(file_path_prefix="nowhere") == 0 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("retrieval_mode", [SearchRetrievalMode.VECTOR, SearchRetrievalMode.HYBRID]) +async def test_semantic_retrieval_honors_the_scope( + search_repository, seeded_paths, monkeypatch, retrieval_mode +): + """The vector and hybrid paths apply the scope, not just the FTS path. + + Semantic retrieval has no SQL WHERE of its own: it post-filters its nearest + neighbours through a scoped FTS query. A scope threaded only into the FTS + entry point would leave `search(retrieval_mode="vector")` answering + project-wide, which is the half-wired failure this test rules out. The + nearest-neighbour stage is stubbed to return every seeded note so anything + that survives did so through the filter. + """ + monkeypatch.setattr(search_repository, "_semantic_enabled", True) + monkeypatch.setattr(search_repository, "_semantic_min_similarity", 0.0) + monkeypatch.setattr( + search_repository, + "_embedding_provider", + SimpleNamespace( + dimensions=4, + model_name="stub", + embed_query=AsyncMock(return_value=[0.0, 0.0, 0.0, 1.0]), + ), + ) + monkeypatch.setattr(search_repository, "_ensure_vector_tables", AsyncMock()) + monkeypatch.setattr(search_repository, "_prepare_vector_session", AsyncMock()) + monkeypatch.setattr( + search_repository, + "_run_vector_query", + AsyncMock( + return_value=[ + { + "entity_id": row_id, + "chunk_key": f"entity:{row_id}:0", + "chunk_text": "subtree scope fixture", + "best_similarity": 0.9, + } + for row_id in seeded_paths.values() + ] + ), + ) + + rows = await search_repository.search( + search_text="subtree", + file_path_prefix="specs", + retrieval_mode=retrieval_mode, + limit=100, + ) + + assert {row.title for row in rows} == {"Alpha", "Beta"} + + +def test_condition_is_one_shared_predicate_for_both_dialects(): + """The SQL text and its parameters are backend-independent by construction. + + Both `_build_fts_query_parts` implementations call this one helper, so the + identical-behavior claim above is structural rather than a coincidence two + hand-written predicates happen to share. + """ + params: dict[str, object] = {} + condition = file_path_prefix_condition("/specs/", params) + + assert condition == ( + "SUBSTR(search_index.file_path, 1, :file_path_prefix_length) = :file_path_prefix" + ) + assert params == {"file_path_prefix": "specs/", "file_path_prefix_length": len("specs/")} + + +@pytest.mark.parametrize("scope", [None, "", "/", " / "]) +def test_condition_declines_a_root_scope(scope): + """No predicate at all, so the root query is not silently narrowed.""" + params: dict[str, object] = {} + + assert file_path_prefix_condition(scope, params) is None + assert params == {} + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + (None, None), + ("", None), + ("/", None), + (" ", None), + ("specs", "specs"), + ("/specs/", "specs"), + (" /specs/nested/ ", "specs/nested"), + ], +) +def test_normalize_collapses_root_spellings(value, expected): + assert normalize_file_path_prefix(value) == expected diff --git a/tests/repository/test_search_trace.py b/tests/repository/test_search_trace.py index c5693a948..88cd3772a 100644 --- a/tests/repository/test_search_trace.py +++ b/tests/repository/test_search_trace.py @@ -1391,6 +1391,7 @@ def test_non_text_criteria_and_null_owner_rows_stay_inspectable(): categories=None, after_date=None, metadata_filters=None, + file_path_prefix=None, retrieval_mode=SearchRetrievalMode.FTS, min_similarity=None, ) diff --git a/tests/repository/test_semantic_search_base.py b/tests/repository/test_semantic_search_base.py index 57a1a8d9b..d6bc9aef0 100644 --- a/tests/repository/test_semantic_search_base.py +++ b/tests/repository/test_semantic_search_base.py @@ -87,6 +87,7 @@ async def search( search_item_types: list[SearchItemType] | None = None, categories: list[str] | None = None, metadata_filters: dict[str, Any] | None = None, + file_path_prefix: str | None = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: float | None = None, limit: int = 10, diff --git a/tests/repository/test_semantic_vector_sync.py b/tests/repository/test_semantic_vector_sync.py index 0e6a8c12e..c14e4cbd1 100644 --- a/tests/repository/test_semantic_vector_sync.py +++ b/tests/repository/test_semantic_vector_sync.py @@ -56,6 +56,7 @@ async def search( search_item_types: list[SearchItemType] | None = None, categories: list[str] | None = None, metadata_filters: dict[str, Any] | None = None, + file_path_prefix: str | None = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: float | None = None, limit: int = 10, diff --git a/tests/repository/test_vector_pagination.py b/tests/repository/test_vector_pagination.py index 004969de1..9e16a154a 100644 --- a/tests/repository/test_vector_pagination.py +++ b/tests/repository/test_vector_pagination.py @@ -66,6 +66,7 @@ async def search( search_item_types: list[SearchItemType] | None = None, categories: list[str] | None = None, metadata_filters: dict[str, Any] | None = None, + file_path_prefix: str | None = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: float | None = None, limit: int = 10, @@ -192,6 +193,7 @@ async def run_page(offset, limit): search_item_types=None, categories=None, metadata_filters=None, + file_path_prefix=None, limit=limit, offset=offset, ) diff --git a/tests/repository/test_vector_threshold.py b/tests/repository/test_vector_threshold.py index cbe55ad54..96b39549b 100644 --- a/tests/repository/test_vector_threshold.py +++ b/tests/repository/test_vector_threshold.py @@ -70,6 +70,7 @@ async def search( search_item_types: Optional[list[SearchItemType]] = None, categories: Optional[list[str]] = None, metadata_filters: Optional[dict[str, Any]] = None, + file_path_prefix: Optional[str] = None, retrieval_mode: SearchRetrievalMode = SearchRetrievalMode.FTS, min_similarity: Optional[float] = None, limit: int = 10, @@ -164,6 +165,7 @@ async def fake_scoped_session(session_maker): search_item_types=None, categories=None, metadata_filters=None, + file_path_prefix=None, limit=10, offset=0, ) diff --git a/tests/services/test_search_service.py b/tests/services/test_search_service.py index 230bfc733..2fbb04610 100644 --- a/tests/services/test_search_service.py +++ b/tests/services/test_search_service.py @@ -327,6 +327,35 @@ async def test_search_categories_only_is_not_no_criteria(): assert SearchQuery().no_criteria() is True +@pytest.mark.asyncio +async def test_file_path_prefix_only_is_not_no_criteria(): + """A subtree scope is criteria; a root spelling of it is not. + + The scope normalizes at the schema boundary, so "/" cannot look like a + filter that the query then fails to apply. + """ + assert SearchQuery(file_path_prefix="specs").no_criteria() is False + assert SearchQuery(file_path_prefix="/").no_criteria() is True + assert SearchQuery(file_path_prefix="/specs/").file_path_prefix == "specs" + + +@pytest.mark.asyncio +async def test_search_by_file_path_prefix_alone_runs_a_scoped_query(search_service, test_graph): + """A scope-only query executes instead of short-circuiting as criteria-free. + + test_graph seeds every note under test/, so the matching scope must return + rows and a non-matching one must return none — the second half proving the + predicate really ran rather than the scope being dropped. + """ + scoped = await search_service.search(SearchQuery(file_path_prefix="test"), limit=100) + elsewhere = await search_service.search(SearchQuery(file_path_prefix="nowhere"), limit=100) + + assert len(scoped) > 0 + assert all(row.file_path.startswith("test/") for row in scoped) + assert elsewhere == [] + assert await search_service.count(SearchQuery(file_path_prefix="test")) == len(scoped) + + @pytest.mark.asyncio async def test_extract_entity_tags_exception_handling(search_service): """Test the _extract_entity_tags method exception handling (lines 147-151).""" From 2d897e9686277f1f59c2954f65dc583283ade794 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 13:21:56 -0500 Subject: [PATCH 04/12] feat(core): match null metadata, refuse predicates that cannot match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three predicate findings, all the same shape: a malformed or unsupported predicate produced a silent empty result or a generic transport error instead of naming the problem. null now matches for real. 'owner=null' finds notes carrying no owner, on both dialects, because SQLite's json_extract and Postgres's jsonb_extract_path_text already collapse a missing key and an explicit JSON null to SQL NULL — so one IS NULL clause answers the same set on either. The parser emits a distinct is_null op rather than eq with a None value, so neither consumer can fall through to the equality branch, which is the bug this is fixing. null outside equality is refused: '>', 'in', 'between' and 'has' all compile to comparisons that are never true against NULL, so they were the same silent zero in four more operators. A non-finite number (score=NaN, 1e999) reached HTTPX's JSON encoder and surfaced as a transport failure; it is now a predicate error naming the value and the quoting escape. An unterminated scalar quote (status="active) was treated as literal text, so the search ran and reported no matches with the malformed predicate hidden. Rather than mirror the list parser's check, that check was removed: a split only happens outside quotes, so a dangling quote always lands inside one element and a single check covers scalars and list elements alike. One refusal site, one message, five fewer lines. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- docs/metadata-search.md | 19 +++ skills/memory-metadata-search/SKILL.md | 13 ++ src/basic_memory/cli/commands/posix.py | 3 +- src/basic_memory/man/man1/find(1).md | 19 ++- src/basic_memory/man/man3/search-notes(3).md | 5 +- src/basic_memory/mcp/tools/posix_tools.py | 91 +++++++++-- src/basic_memory/mcp/tools/search.py | 5 +- .../repository/metadata_filters.py | 33 ++++ .../repository/postgres_search_repository.py | 9 ++ .../repository/sqlite_search_repository.py | 9 ++ tests/mcp/test_tool_posix.py | 151 +++++++++++++++++- tests/repository/test_metadata_filters.py | 29 ++++ .../test_metadata_filters_edge_cases.py | 114 +++++++++++++ 13 files changed, 484 insertions(+), 16 deletions(-) diff --git a/docs/metadata-search.md b/docs/metadata-search.md index 6275fbb33..05df41f73 100644 --- a/docs/metadata-search.md +++ b/docs/metadata-search.md @@ -55,6 +55,23 @@ Range filter (inclusive). Takes a `[min, max]` pair. {"score": {"$between": [0.3, 0.8]}} ``` +### Null (missing or explicitly null) + +A `None` value asks whether a note carries any value for the field at all. + +```json +{"owner": null} +``` + +Matches notes whose frontmatter has no `owner` key, and notes whose `owner` is +explicitly null. Both backends extract those two cases to SQL `NULL`, so the +filter answers the same note set on SQLite and Postgres. + +Null works only through equality. Inside `$in`, `$between`, an array-contains +list, or a comparison it is rejected: those compile to a SQL comparison against +the value, and a comparison with `NULL` is never true, so the filter would +report a confident zero rather than the query it cannot express. + ### Nested Access (dot notation) Access nested frontmatter values using dots. @@ -70,6 +87,7 @@ This queries the `version` key inside a `schema` object in frontmatter. | Operator | Syntax | Example | |----------|--------|---------| | Equality | `{"field": "value"}` | `{"status": "active"}` | +| Is null | `{"field": null}` | `{"owner": null}` | | Array contains (all) | `{"field": ["a", "b"]}` | `{"tags": ["security", "oauth"]}` | | `$in` (any of) | `{"field": {"$in": [...]}}` | `{"priority": {"$in": ["high", "critical"]}}` | | `$gt` / `$gte` | `{"field": {"$gt": N}}` | `{"confidence": {"$gt": 0.7}}` | @@ -82,6 +100,7 @@ This queries the `version` key inside a `schema` object in frontmatter. - Each operator dict must contain exactly one operator. - `$in` and array-contains require non-empty lists. - `$between` requires exactly two values `[min, max]`. +- `null` is an is-null match and only valid as a plain equality value. ## MCP Tool — `search_notes` diff --git a/skills/memory-metadata-search/SKILL.md b/skills/memory-metadata-search/SKILL.md index f045600bc..f623f6bbd 100644 --- a/skills/memory-metadata-search/SKILL.md +++ b/skills/memory-metadata-search/SKILL.md @@ -56,6 +56,17 @@ Numeric values use numeric comparison; strings use lexicographic comparison. {"score": {"$between": [0.3, 0.8]}} ``` +### Null (field missing or explicitly null) + +```json +{"owner": null} +``` + +Matches notes with no `owner` key and notes whose `owner` is explicitly null. +Null works only as a plain equality value — inside `$in`, `$between`, an +array-contains list, or a comparison it is rejected, because those compare +against the value and a comparison with null is never true. + ### Nested Access (dot notation) ```json @@ -67,6 +78,7 @@ Numeric values use numeric comparison; strings use lexicographic comparison. | Operator | Syntax | Example | |----------|--------|---------| | Equality | `{"field": "value"}` | `{"status": "active"}` | +| Is null | `{"field": null}` | `{"owner": null}` | | Array contains | `{"field": ["a", "b"]}` | `{"tags": ["security", "oauth"]}` | | `$in` | `{"field": {"$in": [...]}}` | `{"priority": {"$in": ["high", "critical"]}}` | | `$gt` / `$gte` | `{"field": {"$gt": N}}` | `{"confidence": {"$gt": 0.7}}` | @@ -79,6 +91,7 @@ Numeric values use numeric comparison; strings use lexicographic comparison. - Operator dicts must contain exactly one operator - `$in` and array-contains require non-empty lists - `$between` requires exactly `[min, max]` +- `null` is an is-null match and only valid as a plain equality value > **Warning:** Operators MUST include the `$` prefix — write `$gte`, not `gte`. Without the prefix the filter is treated as an exact-match key and will silently return no results. Correct: `{"confidence": {"$gte": 0.7}}`. Wrong: `{"confidence": {"gte": 0.7}}`. diff --git a/src/basic_memory/cli/commands/posix.py b/src/basic_memory/cli/commands/posix.py index db108bce0..098d2ecfa 100644 --- a/src/basic_memory/cli/commands/posix.py +++ b/src/basic_memory/cli/commands/posix.py @@ -745,7 +745,8 @@ def find( "--meta", help=( "Metadata predicate, repeatable: 'status=active', 'confidence>0.6', " - "'priority in high,critical', 'tags has security', 'score between 0.3,0.8'. " + "'priority in high,critical', 'tags has security', 'score between 0.3,0.8', " + "'owner=null' (key missing or null). " "PATH still scopes the query, by file path" ), ), diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index 3550bd5f1..ee015b419 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -77,12 +77,28 @@ confidence>0.6 comparison: > >= < <= priority in high,critical any of the listed values tags has security,oauth array contains ALL listed values score between 0.3,0.8 inclusive range +owner=null key missing or explicitly null ``` Values are JSON-scalar inferred: `true`/`false`/`null` and numbers become booleans, null, and numbers. Quote a token to force the literal string — `status="true"` matches the four-character string. Quoting also protects a -comma inside a list element: `label in "a,b",c` matches `a,b` or `c`. +comma inside a list element: `label in "a,b",c` matches `a,b` or `c`. An +unterminated quote is a typo, not a value: `status="active` is refused rather +than searched for as the text `"active`. + +`null` matches only through `=`, and it means "this note carries no value +here" — the key is absent from the frontmatter, or present and explicitly +null. Both backends extract those two cases identically, so `owner=null` +answers the same note set on SQLite and Postgres. The other operators compare +against their value and a SQL comparison with null is never true, so +`score>null` and `priority in null,high` are refused instead of answering a +confident zero. + +Numbers must be finite. `score=NaN`, `score=Infinity` and an overflowing +exponent like `score=1e999` are refused by the grammar rather than failing +later as an encoding error; quote one (`score="NaN"`) to match the literal +text. Keys accept dot-paths into nested frontmatter (`review.approved`), and `note_type` is accepted as a spelling of the frontmatter `type` key, matching @@ -114,6 +130,7 @@ bm find /notes --name "auth*" --plain bm find --meta "status=active" bm find /specs --meta "status=active" --meta "confidence>0.6" bm find myproject --meta "status=active" +bm find --meta "owner=null" --fields title bm find --meta "tags has security,oauth" --fields title,priority bm find --meta "status=active" --fields title --plain ``` diff --git a/src/basic_memory/man/man3/search-notes(3).md b/src/basic_memory/man/man3/search-notes(3).md index 25b1d668c..d982154ee 100644 --- a/src/basic_memory/man/man3/search-notes(3).md +++ b/src/basic_memory/man/man3/search-notes(3).md @@ -58,7 +58,10 @@ which is how the manual implements apropos (see [[Manpage]]). - **search_type** — see modes above; default is dynamic (`hybrid` if semantic search is enabled, else `text`) - **metadata_filters** — dict of frontmatter field → value; integer values - match integer YAML fields (`{"section": 3}` works) + match integer YAML fields (`{"section": 3}` works). A `None` value is an + is-null match — notes where the key is absent or explicitly null. `None` + inside `$in`, `$between`, a contains list, or a comparison is refused: those + compare against the value, and a comparison with null is never true - **tags** — list or comma string, same convention as [[write-note(3)]] - **min_similarity** — float override for vector/hybrid threshold; `0.0` shows everything, `0.8` is high precision diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index 392d9f25f..c6a4b793c 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -30,6 +30,7 @@ import asyncio import json +import math import os import re from typing import Annotated, Any, Optional @@ -560,8 +561,26 @@ async def find_listing( _METADATA_KEY_ALIASES = {"note_type": "type"} +def _opens_an_unterminated_quote(text: str) -> bool: + """True when a double quote opens in `text` and nothing closes it. + + One scanner decides this for every value token, scalar or list element, so + the two paths cannot disagree about what "quoted" means. + """ + in_quotes = False + escaped = False + for char in text: + if escaped: + escaped = False + elif in_quotes and char == "\\": + escaped = True + elif char == '"': + in_quotes = not in_quotes + return in_quotes + + def _predicate_scalar(token: str, predicate: str) -> Any: - """Read one predicate value token, refusing a folded-in operator character. + """Read one predicate value token, refusing everything a search cannot answer. "true"/"false"/"null"/numbers become bool/None/int/float so the produced filters dict is byte-equal to what a rich search_notes caller passes as @@ -584,10 +603,37 @@ def _predicate_scalar(token: str, predicate: str) -> Any: f"supported: {_SUPPORTED_PREDICATE_OPS}; quote the value as " f'"{text}" to match text that starts with that character' ) + # Trigger: a quote opens in the token and never closes. + # Why: json.loads rejects it, and the raw-text fallback would then keep the + # dangling quote as part of a literal value — 'status="active' would + # search for the seven-character text '"active', report no matches, + # and hide the typo behind an ordinary empty result. + # Outcome: refuse for every value token, so the scalar operators and the + # list operators (where a severed quote would also mis-split the + # list) answer a dangling quote the same way. + if _opens_an_unterminated_quote(text): + raise ValueError( + f"find: predicate '{predicate}' has an unterminated quoted value; " + "close the quote — 'status=\"active\"' forces a literal string, and " + "'label in \"a,b\",c' protects a comma inside a list element" + ) try: value = json.loads(text) except json.JSONDecodeError: return text + # Trigger: the token parsed to a non-finite float. Python's JSON reader + # accepts NaN/Infinity/-Infinity as an extension, and overflows a + # large exponent ("1e999") to infinity. + # Why: none of those are JSON the request encoder will emit, so the filter + # died at transport with "Out of range float values are not JSON + # compliant" — a network-shaped error for what is a predicate typo. + # Outcome: refuse here, in the same shape as the grammar's other refusals. + if isinstance(value, float) and not math.isfinite(value): + raise ValueError( + f"find: predicate '{predicate}' has a non-finite number '{text}'; " + f'predicate values must be finite numbers; quote the value as "{text}" ' + "to match that literal text" + ) if value is None or isinstance(value, (bool, int, float, str)): return value return text @@ -601,8 +647,11 @@ def _split_predicate_items(raw_value: str, predicate: str) -> list[str]: and `between` too: 'label in "a,b",c' yields ['"a,b"', 'c'], which _predicate_scalar then reads as the literal strings "a,b" and "c". Splitting the raw string first would sever the quoted token into '"a' and 'b"' and - filter for values nothing carries — wrong, and silent. An unterminated quote - is a typo rather than a value, so it fails fast for the same reason. + filter for values nothing carries — wrong, and silent. + + A split only happens outside quotes, so an unterminated quote here always + ends up inside one element and _predicate_scalar refuses it; this function + does not repeat that check. """ items: list[str] = [] current: list[str] = [] @@ -620,11 +669,6 @@ def _split_predicate_items(raw_value: str, predicate: str) -> list[str]: current.pop() items.append("".join(current)) current = [] - if in_quotes: - raise ValueError( - f"find: predicate '{predicate}' has an unterminated quoted value; " - 'quote a list element as "text with, commas"' - ) items.append("".join(current)) stripped = [item.strip() for item in items] if any(not item for item in stripped): @@ -632,6 +676,25 @@ def _split_predicate_items(raw_value: str, predicate: str) -> list[str]: return stripped +def _refuse_null_outside_equality(values: list[Any], op: str, predicate: str) -> None: + """Refuse a null bound, list element, or comparison value. + + Trigger: `null` reached an operator other than '='. + Why: '=' compiles to IS NULL server-side, which is the question null asks — + does this note carry a value here at all. Every other operator compares + against the value, and a SQL comparison with NULL is never true, so + 'score>null' or 'priority in null,high' would answer zero rows for + every note in the project rather than name the query it cannot run. + Outcome: refuse, pointing at the equality spelling that does work. + """ + if any(value is None for value in values): + raise ValueError( + f"find: predicate '{predicate}' uses null with '{op}'; null matches only " + "as equality ('owner=null' finds notes carrying no owner); quote the " + 'value as "null" to match that literal text' + ) + + def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: """Translate POSIX-style predicate strings into the search API metadata_filters dict. @@ -662,6 +725,7 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: _predicate_scalar(item, predicate) for item in _split_predicate_items(raw_value, predicate) ] + _refuse_null_outside_equality(items, op, predicate) if op == "between" and len(items) != 2: raise ValueError(f"find: 'between' needs exactly min,max in '{predicate}'") filters[key] = ( @@ -671,6 +735,8 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: if not raw_value.strip(): raise ValueError(f"find: predicate '{predicate}' has no value") value = _predicate_scalar(raw_value, predicate) + if op != "=": + _refuse_null_outside_equality([value], op, predicate) filters[key] = value if op == "=" else {_SYMBOL_OPERATORS[op]: value} return filters @@ -752,14 +818,17 @@ async def find( "priority in high,critical" any of the listed values "tags has security,oauth" array contains ALL listed values "score between 0.3,0.8" inclusive range + "owner=null" key missing or explicitly null Values are JSON-scalar inferred ("true"/"false"/"null"/numbers become booleans/None/numbers); quote a token to force a literal string (e.g. 'status="true"'), including inside a list, where the quotes also protect a comma ('label in "a,b",c' matches "a,b" or "c") and a value that itself starts with an operator character - ('range=">=5"'). Keys accept dot-paths ("review.approved"); - "note_type" aliases the frontmatter "type" key. Any other operator - fails fast naming the supported set. + ('range=">=5"'). Numbers must be finite, and null is only meaningful + with "=" — the other operators compare against the value, and a + comparison with null is never true. Keys accept dot-paths + ("review.approved"); "note_type" aliases the frontmatter "type" key. + Any other operator fails fast naming the supported set. fields: Frontmatter fields to return per hit (dot-paths allowed), e.g. ["title", "priority"]. Requires `meta`. A field missing on a hit renders as null — rows are never dropped. diff --git a/src/basic_memory/mcp/tools/search.py b/src/basic_memory/mcp/tools/search.py index 1795f1eb8..a3db00834 100644 --- a/src/basic_memory/mcp/tools/search.py +++ b/src/basic_memory/mcp/tools/search.py @@ -895,7 +895,10 @@ async def search_notes( ["requirement"]). Pair with entity_types=["observation"] to return only observations whose category matches exactly. after_date: Optional date filter for recent content (e.g., "1 week", "2d", "2024-01-01") - metadata_filters: Optional structured frontmatter filters (e.g., {"status": "in-progress"}) + metadata_filters: Optional structured frontmatter filters (e.g., {"status": "in-progress"}). + A None value is an is-null match: notes where the key is absent or explicitly + null. None inside $in/$between/a contains list/a comparison is refused — + those compare against the value, and a comparison with null is never true. tags: Optional tag filter (frontmatter tags); shorthand for metadata_filters["tags"]. Accepts a list (["a", "b"]) or a comma-separated string ("a,b"), matching the write_note tags convention and the tag: query shorthand. diff --git a/src/basic_memory/repository/metadata_filters.py b/src/basic_memory/repository/metadata_filters.py index c5a233104..faba459d1 100644 --- a/src/basic_memory/repository/metadata_filters.py +++ b/src/basic_memory/repository/metadata_filters.py @@ -59,11 +59,31 @@ def _normalize_numeric(value: object) -> float: return float(cast(str | int | float, value)) +def _refuse_null(values: Iterable[Any], raw_key: str, op: str) -> None: + """Refuse None anywhere but equality. + + Trigger: a null bound, list element, or comparison value. + Why: every operator but equality compiles to a SQL predicate that compares + against the value, and a comparison with NULL is never true — so the + filter would answer zero rows for every note in the project, reporting + a silent wrong answer instead of naming the query it cannot express. + Equality is the one place null has a meaning both backends express + (IS NULL: the key is absent or explicitly null). + Outcome: refuse, naming the equality form that does work. + """ + if any(value is None for value in values): + raise ValueError( + f"null is not supported by '{op}' in metadata filter for '{raw_key}'; " + f"use {{'{raw_key}': None}} to match a missing or null value" + ) + + def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter]: """Parse metadata filters into normalized clauses. Supported forms: - {"status": "in-progress"} + - {"owner": None} # is null: the key is absent or explicitly null - {"tags": ["security", "oauth"]} # array contains all - {"priority": {"$in": ["high", "critical"]}} - {"schema.confidence": {"$gt": 0.7}} @@ -94,12 +114,14 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter if op == "$in": if not isinstance(value, list) or not value: raise ValueError(f"$in requires a non-empty list for '{raw_key}'") + _refuse_null(value, raw_key, op) parsed.append( ParsedMetadataFilter(path_parts, "in", [_normalize_scalar(v) for v in value]) ) continue if op in _COMPARISON_OPERATORS: + _refuse_null([value], raw_key, op) if _is_numeric_value(value): normalized = _normalize_numeric(value) comparison = "numeric" @@ -119,6 +141,7 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter if op == "$between": if not isinstance(value, list) or len(value) != 2: raise ValueError(f"$between requires [min, max] for '{raw_key}'") + _refuse_null(value, raw_key, op) if _is_numeric_collection(value): normalized = [_normalize_numeric(v) for v in value] comparison = "numeric" @@ -134,6 +157,7 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter if isinstance(raw_value, list): if not raw_value: raise ValueError(f"Empty list not allowed for metadata filter '{raw_key}'") + _refuse_null(raw_value, raw_key, "array contains") parsed.append( ParsedMetadataFilter( path_parts, "contains", [_normalize_scalar(v) for v in raw_value] @@ -141,6 +165,15 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter ) continue + # Null equality: the only operator NULL has a meaning for. Both backends + # extract a missing key and an explicit JSON null as SQL NULL, so one + # IS NULL clause answers "which notes have no owner?" identically on + # SQLite and Postgres. Emitted as its own op because `= NULL` is never + # true in SQL — an ordinary equality clause would report a confident zero. + if raw_value is None: + parsed.append(ParsedMetadataFilter(path_parts, "is_null", None)) + continue + # Simple equality parsed.append(ParsedMetadataFilter(path_parts, "eq", _normalize_scalar(raw_value))) diff --git a/src/basic_memory/repository/postgres_search_repository.py b/src/basic_memory/repository/postgres_search_repository.py index ba51e6f2d..bf07ec020 100644 --- a/src/basic_memory/repository/postgres_search_repository.py +++ b/src/basic_memory/repository/postgres_search_repository.py @@ -1206,6 +1206,15 @@ async def _build_fts_query_parts( text_expr = f"jsonb_extract_path_text({metadata_expr}, {path_args})" json_expr = f"jsonb_extract_path({metadata_expr}, {path_args})" + # jsonb_extract_path_text returns SQL NULL both for a missing key + # and for an explicit JSON null — the same two cases SQLite's + # json_extract collapses — so the dialects answer + # `{"owner": None}` row for row. `= NULL` is never true, so + # equality here would report a confident zero. + if filt.op == "is_null": + conditions.append(f"{text_expr} IS NULL") + continue + if filt.op == "eq": value_param = f"meta_val_{idx}" params[value_param] = filt.value diff --git a/src/basic_memory/repository/sqlite_search_repository.py b/src/basic_memory/repository/sqlite_search_repository.py index b88565515..9021d4bfc 100644 --- a/src/basic_memory/repository/sqlite_search_repository.py +++ b/src/basic_memory/repository/sqlite_search_repository.py @@ -952,6 +952,15 @@ async def _build_fts_query_parts( params[path_param] = build_sqlite_json_path(filt.path_parts) extract_expr = f"json_extract(entity.entity_metadata, :{path_param})" + # json_extract returns SQL NULL both for a missing key and for an + # explicit JSON null, and the generated frontmatter_* columns are + # that same json_extract — so IS NULL means "the note carries no + # value here", the question `{"owner": None}` asks. `= NULL` is + # never true, so equality here would report a confident zero. + if filt.op == "is_null": + conditions.append(f"{extract_expr} IS NULL") + continue + if filt.op == "eq": value_param = f"meta_val_{idx}" params[value_param] = filt.value diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index a683b0d4b..310993ac1 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -22,7 +22,7 @@ UnqualifiedPathRefusedError, ) from basic_memory.mcp.tools import cat, find, grep, ls, man, search_notes, tail, write_note -from basic_memory.repository.metadata_filters import parse_metadata_filters +from basic_memory.repository.metadata_filters import ParsedMetadataFilter, parse_metadata_filters from basic_memory.schemas.search import SearchRetrievalMode @@ -529,6 +529,10 @@ async def test_find_rejects_bad_arguments(kwargs, message): # Quoting is the documented escape for literal values, and it holds inside # a list: the comma it protects belongs to the value, not to the list. ('label in "a,b",c', {"label": {"$in": ["a,b", "c"]}}), + # A backslash-escaped quote stays inside the value; it neither closes the + # token nor leaves it looking unterminated. + ('label in "a\\"b",c', {"label": {"$in": ['a"b', "c"]}}), + ('note="say \\"hi\\""', {"note": 'say "hi"'}), ('tags has "red, green"', {"tags": ["red, green"]}), ('name in "quoted"', {"name": {"$in": ["quoted"]}}), # An unquoted value may not start with an operator character (a mis-spelled @@ -538,6 +542,11 @@ async def test_find_rejects_bad_arguments(kwargs, message): ('range>"<=5"', {"range": {"$gt": "<=5"}}), ('bound in ">=5","<=9"', {"bound": {"$in": [">=5", "<=9"]}}), ('marks has "","=b"', {"marks": ["", "=b"]}), + # Quoting is also the escape for the tokens the grammar refuses unquoted: + # the non-finite number spellings and null outside equality. + ('score="NaN"', {"score": "NaN"}), + ('score>"Infinity"', {"score": {"$gt": "Infinity"}}), + ('owner in "null","alice"', {"owner": {"$in": ["null", "alice"]}}), # Dot-paths address nested frontmatter and pass through verbatim. ("review.approved=true", {"review.approved": True}), # The one alias search_notes carries, so both surfaces accept one spelling. @@ -607,6 +616,10 @@ def test_parse_meta_predicates_and_together(): (['priority in "high,low'], "unterminated quoted value"), (['tags has red,"green'], "unterminated quoted value"), (["status="], "has no value"), + # Non-finite numbers and null-outside-equality: both used to reach the + # server (or the request encoder) as a query nothing could answer. + (["score=NaN"], "non-finite number"), + (["score>null"], "uses null with '>'"), (["status=active", "status=draft"], "duplicate predicate key 'status'"), # The alias collapses onto the same key, so the collision is still caught. (["note_type=note", "type=spec"], "duplicate predicate key 'type'"), @@ -670,6 +683,121 @@ def test_malformed_operator_refusal_teaches_the_grammar_and_the_escape(): assert 'quote the value as "=active"' in message +@pytest.mark.parametrize( + "predicate", + [ + # Python's JSON reader accepts these three spellings as an extension... + "score=NaN", + "score=Infinity", + "score=-Infinity", + # ...and silently overflows an oversized exponent to infinity. + "score=1e999", + "score=-1e999", + # Every operator reads its values through the same scalar reader. + "score>NaN", + "score<=Infinity", + "score in 0.5,NaN", + "marks has NaN", + "score between NaN,0.8", + "score between 0.3,1e999", + ], +) +def test_non_finite_numbers_refuse_instead_of_failing_at_transport(predicate): + """REGRESSION: a non-finite number died in the request encoder, not the grammar. + + json.loads builds a real float for NaN/Infinity/-Infinity and overflows + 1e999 to inf, so the parser accepted them into the filters dict. Nothing + rejected them until httpx serialized the request body and raised "Out of + range float values are not JSON compliant" — a transport failure standing in + for a predicate typo, naming neither find nor the offending predicate. + """ + with pytest.raises(ValueError, match="non-finite number"): + posix_tools._parse_meta_predicates([predicate]) + + +def test_non_finite_refusal_names_the_predicate_and_the_escape(): + """The refusal is shaped like the grammar's others: what, where, and the way out.""" + with pytest.raises(ValueError) as excinfo: + posix_tools._parse_meta_predicates(["score=NaN"]) + + message = str(excinfo.value) + assert "predicate 'score=NaN' has a non-finite number 'NaN'" in message + assert "predicate values must be finite numbers" in message + assert 'quote the value as "NaN"' in message + + +@pytest.mark.parametrize( + "predicate", + [ + 'status="active', + 'status = "active', + 'confidence>"0.6', + # A quote that opens, closes, and opens again is still unterminated. + 'name="a"b"c', + # The list operators reach the same check through their split elements. + 'priority in "high,low', + 'tags has red,"green', + 'score between "0.3,0.8', + ], +) +def test_a_dangling_quote_refuses_for_every_operator(predicate): + """REGRESSION: the scalar path used to keep the dangling quote as the value. + + 'status="active' fails json.loads, and the raw-text fallback then filtered + for the literal seven characters '"active' — a search that runs, matches + nothing, and reports an ordinary empty result with the typo buried in it. + The list operators already refused a severed quote; one shared check now + gives both paths the same answer. + """ + with pytest.raises(ValueError, match="unterminated quoted value"): + posix_tools._parse_meta_predicates([predicate]) + + +def test_unterminated_quote_refusal_teaches_both_uses_of_quoting(): + with pytest.raises(ValueError) as excinfo: + posix_tools._parse_meta_predicates(['status="active']) + + message = str(excinfo.value) + assert "predicate 'status=\"active' has an unterminated quoted value" in message + assert 'status="active"' in message + assert 'label in "a,b",c' in message + + +@pytest.mark.parametrize( + "predicate", + [ + "score>null", + "score>=null", + "score Date: Tue, 1 Sep 2026 15:53:32 -0500 Subject: [PATCH 05/12] fix(core): scope metadata find to notes and to the path as written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings on the unshipped metadata-predicate surface, each the same shape: a wrong answer wearing the same confident total as a right one. A metadata filter now matches Markdown notes only. Every indexed file gets an ENTITY row, and a PDF or an image carries no frontmatter keys at all — which is exactly what IS NULL asks for, so 'owner=null' returned the whole non-note half of a project and counted it into an exact total. Positive predicates hid the hole, because nothing a regular file carries could satisfy one; the content-type clause therefore applies to any metadata filter, so the frontmatter-only contract belongs to the clause rather than to whichever operator happened to be used. It is one shared predicate for both dialects, sitting next to the subtree scope for the same reason: a filter admitting different rows per backend would report an exact total for a match set the other never produces. The path scope now means what it says. Exactly two things are notation — a leading './' and the surrounding separators — and DirectoryService already strips those for the plain listing, so one PATH names one subtree with or without --meta. './specs' used to reach the SQL as the prefix './specs/' and match nothing at all, and './' scoped to a directory named '.' rather than to the root. Everything else survives byte for byte, whitespace included: a directory really can be named ' specs ', and stripping answered for 'specs/' instead — a different subtree, not an empty result. find stopped pre-stripping as well, so SearchQuery's field validator is the single boundary parser instead of two half-normalizations in sequence. Field projection no longer leaves reads running past the client that issued them. gather raises the first failure and leaves its siblings alive, and find then unwound out of get_project_client, closing the client underneath as many as ~199 reads still parked on the semaphore — each firing against a closed client and raising into a task nobody awaits. Cancel-and-drain in a finally: the cancels are no-ops on success, and the original failure is still what reaches the caller. Deliberately not TaskGroup, whose ExceptionGroup would replace a precise ToolError with "unhandled errors in a TaskGroup". Signed-off-by: phernandez --- docs/metadata-search.md | 2 + skills/memory-metadata-search/SKILL.md | 2 + src/basic_memory/man/man1/find(1).md | 4 + src/basic_memory/mcp/tools/posix_tools.py | 34 ++++++-- .../repository/postgres_search_repository.py | 5 ++ .../repository/search_repository_base.py | 21 +++++ .../repository/sqlite_search_repository.py | 5 ++ src/basic_memory/schemas/search.py | 33 ++++--- tests/mcp/test_tool_posix.py | 86 +++++++++++++++++++ .../test_metadata_filters_edge_cases.py | 47 +++++++++- .../test_search_file_path_prefix.py | 44 ++++++++-- 11 files changed, 256 insertions(+), 27 deletions(-) diff --git a/docs/metadata-search.md b/docs/metadata-search.md index 05df41f73..58d699b80 100644 --- a/docs/metadata-search.md +++ b/docs/metadata-search.md @@ -10,6 +10,8 @@ Basic Memory automatically indexes custom frontmatter fields so you can query th Filters are a JSON dictionary where each key targets a frontmatter field and the value specifies the match condition. Multiple keys combine with **AND** logic — every filter must match. +Metadata filters only ever match Markdown notes. A project can also index PDFs, images and other regular files, but those carry no frontmatter, so they are never metadata hits and never counted in the result total — including for the null filter below, which asks about a *missing* value. + ### Equality Match a single value exactly. diff --git a/skills/memory-metadata-search/SKILL.md b/skills/memory-metadata-search/SKILL.md index f623f6bbd..21c8dad83 100644 --- a/skills/memory-metadata-search/SKILL.md +++ b/skills/memory-metadata-search/SKILL.md @@ -92,6 +92,8 @@ against the value and a comparison with null is never true. - `$in` and array-contains require non-empty lists - `$between` requires exactly `[min, max]` - `null` is an is-null match and only valid as a plain equality value +- Metadata filters match Markdown notes only — indexed PDFs, images and other + regular files carry no frontmatter and are never hits, not even for `null` > **Warning:** Operators MUST include the `$` prefix — write `$gte`, not `gte`. Without the prefix the filter is treated as an exact-match key and will silently return no results. Correct: `{"confidence": {"$gte": 0.7}}`. Wrong: `{"confidence": {"gte": 0.7}}`. diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index ee015b419..be4dd47a3 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -50,6 +50,10 @@ named directory and admit notes that are not. The prefix matches on a directory boundary and case-sensitively, identically on SQLite and Postgres: `/specs` admits `specs/api.md`, never `specs-archive/api.md` or `Specs/api.md`, and a `_` or `%` in a directory name is an ordinary character, not a wildcard. +Only the surrounding separators and a leading `./` are notation — the plain +listing reads them the same way, so one `PATH` names one subtree with or +without `--meta`. Everything else belongs to the path, including whitespace: a +directory named `" specs "` is addressed by that exact spelling. `PATH` may also name a project (`bm find myproject --meta ...`) — that is a routing prefix, a mount point rather than a subtree, and scopes to that project's root. diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index c6a4b793c..bf31da21b 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -912,10 +912,12 @@ async def find( return await _find_by_metadata( route_project=route.project, # route.path is the caller's input verbatim when no project prefix - # was recognized, so one strip covers both routed and raw spellings. - # A bare '' strips to "", the project root — a mount point, - # not a subtree. - scope=route.path.strip("/"), + # was recognized, so one value covers both routed and raw spellings. + # SearchQuery.file_path_prefix is the boundary parser for it: it + # reads "./specs" the way the directory listing does, and collapses + # every root spelling — including the "" a bare '' routes + # to, a mount point rather than a subtree — onto "no scope". + scope=route.path, metadata_filters=metadata_filters, fields=fields, page=page, @@ -971,8 +973,9 @@ async def _find_by_metadata( from basic_memory.mcp.clients import KnowledgeClient, SearchClient query = SearchQuery( - # "" is the project root: the predicates are then the whole WHERE. - file_path_prefix=scope or None, + # Normalized by the field validator, which maps every root spelling + # onto None: the predicates are then the whole WHERE. + file_path_prefix=scope, metadata_filters=metadata_filters, entity_types=[SearchItemType.ENTITY], ) @@ -1007,7 +1010,24 @@ async def entity_metadata(entity_external_id: str) -> dict[str, Any] | None: entity = await knowledge_client.get_entity(entity_external_id) return entity.entity_metadata - hydrated = await asyncio.gather(*(entity_metadata(hit_id) for hit_id in hit_ids)) + # Trigger: any one projection read fails — a hit deleted between the + # search and its hydration, or a cloud-routed GET erroring. + # Why: gather raises the first failure but leaves its siblings running, + # and this function then unwinds out of get_project_client, which + # closes the client underneath them. Every read still queued behind + # the semaphore would fire against a closed client and raise into a + # task nobody awaits — background work outliving the resource that + # owns it, and a log full of secondary errors hiding the real one. + # Outcome: the siblings are cancelled and drained inside the client's + # lifetime; the first failure is still what reaches the caller. + # On success the cancels are no-ops on already-finished tasks. + reads = [asyncio.create_task(entity_metadata(hit_id)) for hit_id in hit_ids] + try: + hydrated = await asyncio.gather(*reads) + finally: + for read in reads: + read.cancel() + await asyncio.gather(*reads, return_exceptions=True) # Injected post-dump so null field values survive exclude_none. for row, metadata in zip(payload["results"], hydrated, strict=True): row["fields"] = _project_metadata_fields(metadata, fields) diff --git a/src/basic_memory/repository/postgres_search_repository.py b/src/basic_memory/repository/postgres_search_repository.py index bf07ec020..01c4efc54 100644 --- a/src/basic_memory/repository/postgres_search_repository.py +++ b/src/basic_memory/repository/postgres_search_repository.py @@ -27,6 +27,7 @@ SearchRepositoryBase, VectorChunkState, file_path_prefix_condition, + metadata_filter_content_type_condition, ) from basic_memory.repository.search_trace import ( SearchTraceCollector, @@ -1193,6 +1194,10 @@ async def _build_fts_query_parts( if metadata_filters: parsed_filters = parse_metadata_filters(metadata_filters) from_clause = f"{from_clause} JOIN entity ON search_index.entity_id = entity.id" + # Frontmatter filters answer for notes only; see + # metadata_filter_content_type_condition for why every regular file + # would otherwise satisfy a null predicate. + conditions.append(metadata_filter_content_type_condition(params)) metadata_expr = "entity.entity_metadata::jsonb" for idx, filt in enumerate(parsed_filters): diff --git a/src/basic_memory/repository/search_repository_base.py b/src/basic_memory/repository/search_repository_base.py index 96c4aa98c..b76f92c0f 100644 --- a/src/basic_memory/repository/search_repository_base.py +++ b/src/basic_memory/repository/search_repository_base.py @@ -76,6 +76,7 @@ StagedVectorDeletion as _StagedVectorDeletion, VectorChunkState, ) +from basic_memory.runtime.storage import RUNTIME_MARKDOWN_CONTENT_TYPE from basic_memory.runtime.vector_sync import VectorSyncBatchResult from basic_memory.schemas.search import ( SearchItemType, @@ -185,6 +186,26 @@ def file_path_prefix_condition( return "SUBSTR(search_index.file_path, 1, :file_path_prefix_length) = :file_path_prefix" +def metadata_filter_content_type_condition(params: Dict[str, Any]) -> str: + """Build the SQL restricting a metadata-filtered query to Markdown notes. + + Frontmatter is a Markdown-only construct, but every indexed file — PDF, + image, binary — gets its own ENTITY row whose ``entity_metadata`` carries no + keys at all. A positive predicate can never match one, so this constraint + was invisible until ``{"key": None}`` arrived: ``IS NULL`` is satisfied by + the *absence* of a key, which is exactly the state every regular file is in, + and the whole non-note half of a project counted into an exact total. + + Applied to any metadata filter, not just the null one, so the + frontmatter-only contract is a property of the clause rather than of which + operator happened to be used. Shared by both backends for the same reason + the subtree scope is: a filter that admits different rows per dialect would + report an exact total for a match set the other never produces. + """ + params["metadata_filter_content_type"] = RUNTIME_MARKDOWN_CONTENT_TYPE + return "entity.content_type = :metadata_filter_content_type" + + async def purge_stale_search_index_rows( session_maker: async_sessionmaker[AsyncSession], project_id: int, diff --git a/src/basic_memory/repository/sqlite_search_repository.py b/src/basic_memory/repository/sqlite_search_repository.py index 9021d4bfc..03f287a0c 100644 --- a/src/basic_memory/repository/sqlite_search_repository.py +++ b/src/basic_memory/repository/sqlite_search_repository.py @@ -31,6 +31,7 @@ from basic_memory.repository.search_repository_base import ( SearchRepositoryBase, file_path_prefix_condition, + metadata_filter_content_type_condition, ) from basic_memory.repository.script_ngrams import analyze_script_query from basic_memory.repository.search_trace import ( @@ -933,6 +934,10 @@ async def _build_fts_query_parts( if metadata_filters: parsed_filters = parse_metadata_filters(metadata_filters) from_clause = "search_index JOIN entity ON search_index.entity_id = entity.id" + # Frontmatter filters answer for notes only; see + # metadata_filter_content_type_condition for why every regular file + # would otherwise satisfy a null predicate. + conditions.append(metadata_filter_content_type_condition(params)) entity_columns = await self._get_entity_columns() for idx, filt in enumerate(parsed_filters): diff --git a/src/basic_memory/schemas/search.py b/src/basic_memory/schemas/search.py index a4207e45e..749d93243 100644 --- a/src/basic_memory/schemas/search.py +++ b/src/basic_memory/schemas/search.py @@ -31,19 +31,32 @@ class SearchRetrievalMode(str, Enum): def normalize_file_path_prefix(value: Optional[str]) -> Optional[str]: - """Reduce a directory scope to its bare project-relative spelling. - - "", "/", and " " all name the project root — no subtree scope — and must - collapse to None. "/" in particular is a non-empty string, so left as-is it - would read as criteria to the service's has-criteria check while - contributing no predicate: a query that filters nothing yet reports its - total as if it had. "/specs/" and "specs" are the same scope, so they too - normalize to the one value the SQL predicate and the criteria check share. + """Reduce a directory scope to the bare project-relative spelling the index stores. + + Exactly two things here are notation rather than path: a leading "./" and + the surrounding separators. ``DirectoryService`` removes those two before it + lists a directory, and ``find``'s two arms read one ``path`` argument — a + scope that means "specs/" without ``meta`` and "./specs/" with it is the + same argument asking two different questions, and the SQL prefix built from + the second matches nothing, silently. + + Everything else survives byte for byte, whitespace included. A directory + really can be named " specs ", and stripping would answer for "specs/" + instead: a different subtree, reported under the same exact total as the + right one. The preserved spelling gives the honest empty result. + + A spelling carrying no path at all — "", "/", "./", " ", " / " — names + the project root, i.e. no subtree scope, and must collapse to None. "/" in + particular is a non-empty string, so left as-is it would read as criteria to + the service's has-criteria check while contributing no predicate: a query + that filters nothing yet reports its total as if it had. """ if value is None: return None - normalized = value.strip().strip("/") - return normalized or None + scope = value.removeprefix("./") + if not scope.strip().strip("/"): + return None + return scope.strip("/") class SearchQuery(BaseModel): diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index 310993ac1..6b57745c7 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -7,6 +7,7 @@ import asyncio import re +from datetime import datetime, timezone from pathlib import Path from textwrap import dedent from types import SimpleNamespace @@ -17,11 +18,13 @@ from fastmcp.exceptions import ToolError import basic_memory.mcp.tools.posix_tools as posix_tools +from basic_memory import db from basic_memory.mcp.project_context import ( ProjectPrefixConflictError, UnqualifiedPathRefusedError, ) from basic_memory.mcp.tools import cat, find, grep, ls, man, search_notes, tail, write_note +from basic_memory.models import Entity from basic_memory.repository.metadata_filters import ParsedMetadataFilter, parse_metadata_filters from basic_memory.schemas.search import SearchRetrievalMode @@ -968,6 +971,45 @@ async def test_find_meta_null_finds_the_notes_carrying_no_value(client, test_pro assert {row["title"] for row in present["results"]} == {"Alpha Spec"} +@pytest.mark.asyncio +async def test_find_meta_never_returns_a_non_markdown_file( + client, test_project, meta_notes, entity_repository, search_service, session_maker +): + """REGRESSION: `find --meta` is frontmatter-only, so a PDF is never a hit. + + An indexed regular file gets an ENTITY row like any note, and it carries no + frontmatter keys at all — which is precisely what `key=null` asks for. So + the null predicate returned every PDF, image and binary in the project and + counted them into the exact total, while positive predicates hid the hole + because nothing a regular file carries could satisfy one. Both shapes are + asserted here so the constraint cannot regress to a null-only special case. + """ + now = datetime.now(timezone.utc) + async with db.scoped_session(session_maker) as session: + scan = await entity_repository.add( + session, + Entity( + project_id=test_project.id, + title="Scanned Contract", + note_type="file", + content_type="application/pdf", + file_path="specs/Scanned Contract.pdf", + permalink="specs/scanned-contract", + created_at=now, + updated_at=now, + ), + ) + await search_service.index_entity_data(scan) + + absent = await find(meta=["review.approved=null"], project=test_project.name) + present = await find(meta=["status=active"], project=test_project.name) + + assert {row["title"] for row in absent["results"]} == {"Beta Spec", "Gamma Note"} + assert absent["total"] == 2 + assert absent["total_is_exact"] is True + assert {row["title"] for row in present["results"]} == {"Alpha Spec", "Gamma Note"} + + @pytest.mark.asyncio @pytest.mark.parametrize("path", ["/", ""]) async def test_find_meta_answers_at_the_project_root(client, test_project, meta_notes, path): @@ -984,6 +1026,10 @@ async def test_find_meta_answers_at_the_project_root(client, test_project, meta_ [ ("specs", {"Alpha Spec", "Beta Spec"}), ("/specs", {"Alpha Spec", "Beta Spec"}), + # "./" is relative notation, the same way the directory listing reads + # it; without that, the SQL prefix "./specs/" matched nothing at all. + ("./specs", {"Alpha Spec", "Beta Spec"}), + ("./specs/", {"Alpha Spec", "Beta Spec"}), # A spaced directory name is a file path, not a slug: it scopes verbatim. ("My Notes", {"Gamma Note"}), ("nonexistent", set()), @@ -1230,6 +1276,46 @@ async def counting_get_entity(self, entity_id, **kwargs): } +@pytest.mark.asyncio +async def test_find_meta_fields_cancels_sibling_reads_when_one_fails( + client, test_project, meta_notes, monkeypatch +): + """REGRESSION: no projection read outlives the client it was issued on. + + gather raises the first failure and leaves the rest running, so find used to + unwind out of get_project_client — closing the shared client — while sibling + GETs were still on the wire or still queued behind the semaphore. Those then + raised against a closed client into tasks nobody awaits: background work + past the resource's lifetime, and secondary errors burying the real one. + """ + # Import here to mirror the tool's own deferred client import. + from basic_memory.mcp.clients import KnowledgeClient + + reads: list[asyncio.Task[dict[str, object] | None]] = [] + + async def racing_get_entity(self, entity_id, **kwargs): + current = asyncio.current_task() + assert current is not None + reads.append(current) + if len(reads) == 1: + # Yield first so the sibling is genuinely in flight when this fails. + await asyncio.sleep(0) + raise ToolError("hit deleted between search and hydration") + # Stands in for a request still awaiting its response. + await asyncio.sleep(60) + raise AssertionError("sibling read outlived the client that issued it") + + monkeypatch.setattr(KnowledgeClient, "get_entity", racing_get_entity) + + with pytest.raises(ToolError, match="hit deleted between search and hydration"): + await find(meta=["status=active"], fields=["title"], project=test_project.name) + + assert len(reads) == 2 + sibling = reads[1] + assert sibling.done() + assert sibling.cancelled() + + @pytest.mark.asyncio async def test_find_meta_unsupported_key_surfaces_the_api_error(client, test_project, meta_notes): """The server owns key validation; its refusal reaches the caller intact.""" diff --git a/tests/repository/test_metadata_filters_edge_cases.py b/tests/repository/test_metadata_filters_edge_cases.py index 13b09f41a..65d981e0f 100644 --- a/tests/repository/test_metadata_filters_edge_cases.py +++ b/tests/repository/test_metadata_filters_edge_cases.py @@ -15,10 +15,18 @@ from basic_memory.schemas.search import SearchItemType -async def _index_entity_with_metadata(search_repository, session_maker, title, entity_metadata): +async def _index_entity_with_metadata( + search_repository, + session_maker, + title, + entity_metadata, + *, + content_type="text/markdown", + extension="md", +): """Helper: create an entity with given metadata and index it for search.""" slug = "-".join(title.lower().split()) - file_path = f"test/{slug}.md" + file_path = f"test/{slug}.{extension}" permalink = f"test/{slug}" now = datetime.now(timezone.utc) @@ -29,7 +37,7 @@ async def _index_entity_with_metadata(search_repository, session_maker, title, e note_type="note", permalink=permalink, file_path=file_path, - content_type="text/markdown", + content_type=content_type, entity_metadata=entity_metadata, created_at=now, updated_at=now, @@ -101,6 +109,39 @@ async def test_null_filter_matches_a_missing_key_and_an_explicit_null( assert await search_repository.count(metadata_filters={"owner": None}) == 2 +@pytest.mark.asyncio +async def test_metadata_filters_never_match_a_non_markdown_file(search_repository, session_maker): + """REGRESSION: a PDF or an image is not an unowned note. + + Frontmatter is a Markdown-only construct, but every indexed file gets an + ENTITY row, and a regular file's entity_metadata carries no keys at all — + so `IS NULL` matched every PDF, image and binary in the project and counted + them into an exact total. Positive predicates hid the hole because nothing + a regular file carries could satisfy one; the content-type constraint makes + the frontmatter-only contract hold for either shape of predicate. + """ + unowned_note = await _index_entity_with_metadata( + search_repository, + session_maker, + "Unowned Note", + {"status": "active"}, + ) + await _index_entity_with_metadata( + search_repository, + session_maker, + "Scanned Contract", + None, + content_type="application/pdf", + extension="pdf", + ) + + results = await search_repository.search(metadata_filters={"owner": None}) + + assert {r.id for r in results} == {unowned_note.id} + # The total is what paginates, so it has to exclude the PDF too. + assert await search_repository.count(metadata_filters={"owner": None}) == 1 + + @pytest.mark.asyncio async def test_null_filter_excludes_a_note_carrying_a_value(search_repository, session_maker): """A note with a real value is never a null match. diff --git a/tests/repository/test_search_file_path_prefix.py b/tests/repository/test_search_file_path_prefix.py index 611edee50..885cd96ba 100644 --- a/tests/repository/test_search_file_path_prefix.py +++ b/tests/repository/test_search_file_path_prefix.py @@ -26,7 +26,8 @@ # file_path -> (title, status). Every decoy here is a real way a naive predicate # leaks: a sibling directory sharing the scope's name as a prefix, a "_" or "%" -# read as a LIKE wildcard, and a case variant that only one dialect's LIKE folds. +# read as a LIKE wildcard, a case variant that only one dialect's LIKE folds, +# and a name whose own leading/trailing spaces a normalizer could eat. SEEDED_NOTES: dict[str, tuple[str, str]] = { "specs/alpha.md": ("Alpha", "active"), "specs/nested/beta.md": ("Beta", "draft"), @@ -36,6 +37,7 @@ "my-notes/zeta.md": ("Zeta", "active"), "100%/eta.md": ("Eta", "active"), "100pct/theta.md": ("Theta", "active"), + " specs /iota.md": ("Iota", "active"), } @@ -160,14 +162,32 @@ async def test_prefix_is_case_sensitive_on_both_backends(search_repository, seed @pytest.mark.asyncio -@pytest.mark.parametrize("scope", ["specs", "/specs", "specs/", "/specs/", " specs "]) +@pytest.mark.parametrize("scope", ["specs", "/specs", "specs/", "/specs/", "./specs", "./specs/"]) async def test_scope_spellings_normalize_to_one_query(search_repository, seeded_paths, scope): - """Leading, trailing, and surrounding whitespace/slashes name one subtree.""" + """Separators and the "./" relative prefix are notation, so all name one subtree. + + "./specs" is the spelling the plain directory listing already accepts — + `DirectoryService` strips that prefix — so the two `find` arms must not + disagree about which subtree one `path` argument names. + """ assert await _titles(search_repository, file_path_prefix=scope) == {"Alpha", "Beta"} @pytest.mark.asyncio -@pytest.mark.parametrize("scope", [None, "", "/", " "]) +async def test_whitespace_in_a_directory_name_belongs_to_the_path(search_repository, seeded_paths): + """REGRESSION: " specs " is its own directory, not a padded "specs". + + Stripping the surrounding whitespace silently repointed the scope at a + different subtree and reported that subtree's rows under the same exact + total the named one would have carried. Both directions are asserted so a + reintroduced strip fails here rather than merging the two scopes unnoticed. + """ + assert await _titles(search_repository, file_path_prefix=" specs ") == {"Iota"} + assert await _titles(search_repository, file_path_prefix="specs") == {"Alpha", "Beta"} + + +@pytest.mark.asyncio +@pytest.mark.parametrize("scope", [None, "", "/", " ", "./"]) async def test_root_spellings_apply_no_scope(search_repository, seeded_paths, scope): """The root is the absence of a subtree predicate, not a prefix of "/".""" titles = await _titles( @@ -328,7 +348,7 @@ def test_condition_is_one_shared_predicate_for_both_dialects(): assert params == {"file_path_prefix": "specs/", "file_path_prefix_length": len("specs/")} -@pytest.mark.parametrize("scope", [None, "", "/", " / "]) +@pytest.mark.parametrize("scope", [None, "", "/", " / ", "./"]) def test_condition_declines_a_root_scope(scope): """No predicate at all, so the root query is not silently narrowed.""" params: dict[str, object] = {} @@ -344,10 +364,20 @@ def test_condition_declines_a_root_scope(scope): ("", None), ("/", None), (" ", None), + (" / ", None), + # "./" is the relative spelling of the root, as it is for `ls`. + ("./", None), ("specs", "specs"), ("/specs/", "specs"), - (" /specs/nested/ ", "specs/nested"), + ("/specs/nested/", "specs/nested"), + ("./specs/", "specs"), + # Only one "./" is notation; a second is a directory literally named ".". + ("././specs", "./specs"), + # Whitespace inside a scope that carries a path is part of the path. + (" specs ", " specs "), + ("/ specs /", " specs "), + ("My Notes/drafts", "My Notes/drafts"), ], ) -def test_normalize_collapses_root_spellings(value, expected): +def test_normalize_keeps_the_path_and_drops_only_the_notation(value, expected): assert normalize_file_path_prefix(value) == expected From 8fbac45eca91096c57f7924c98ce55571196bca0 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 16:22:24 -0500 Subject: [PATCH 06/12] fix(core): escape LIKE wildcards in array-contains metadata filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An array-contains metadata filter — `tags has 100%` from find, or `{"tags": ["100%"]}` on the search API — is answered by two clauses ORed together: an exact JSON-membership test (json_each on SQLite, `@>` on Postgres), plus a substring LIKE fallback that reaches frontmatter holding the array's text rather than a real array. The fallback interpolated the searched-for value straight into its LIKE pattern, so "%" and "_" in a value were read as wildcards. `tags has 100%` matched "100-percent", and because the same query builder serves search() and count(), the wrong rows counted into the exact total too. Reproduced identically on both backends before this change. Escape "%", "_" and the escape character itself, and name the escape character in an explicit ESCAPE clause — required rather than decorative, since SQLite's LIKE has no default escape character while Postgres's is already the backslash, so spelling it out is what makes one pattern mean one thing on both dialects. The escaping lives in a shared metadata_contains_like_condition() beside the existing file_path_prefix_condition and metadata_filter_content_type_condition, for the reason those are shared: a filter that admitted different rows per dialect would report an exact total for a match set the other never produces. Bind-parameter names are unchanged, and the exact JSON-membership half — the primary path — is untouched. Regression tests cover "%", "_" and the escape character on both backends, on the tags column and on a nested json_extract path, with a positive control proving the fallback still finds an element inside an array stored as text. Signed-off-by: phernandez --- .../repository/postgres_search_repository.py | 17 +-- .../repository/search_repository_base.py | 51 ++++++++ .../repository/sqlite_search_repository.py | 16 ++- .../test_metadata_filters_edge_cases.py | 112 ++++++++++++++++++ 4 files changed, 183 insertions(+), 13 deletions(-) diff --git a/src/basic_memory/repository/postgres_search_repository.py b/src/basic_memory/repository/postgres_search_repository.py index 01c4efc54..eb8a3f4e3 100644 --- a/src/basic_memory/repository/postgres_search_repository.py +++ b/src/basic_memory/repository/postgres_search_repository.py @@ -27,6 +27,7 @@ SearchRepositoryBase, VectorChunkState, file_path_prefix_condition, + metadata_contains_like_condition, metadata_filter_content_type_condition, ) from basic_memory.repository.search_trace import ( @@ -1242,14 +1243,16 @@ async def _build_fts_query_parts( for j, val in enumerate(filt.value): tag_param = f"{base_param}_{j}" params[tag_param] = json.dumps([val]) - like_param = f"{base_param}_{j}_like" - params[like_param] = f'%"{val}"%' - like_param_single = f"{base_param}_{j}_like_single" - params[like_param_single] = f"%'{val}'%" + # The exact JSONB containment test is the primary path; the + # substring patterns only reach values stored as array text. + like_condition = metadata_contains_like_condition( + text_expr, + val, + param_prefix=tag_param, + params=params, + ) tag_conditions.append( - f"({json_expr} @> CAST(:{tag_param} AS jsonb) " - f"OR {text_expr} LIKE :{like_param} " - f"OR {text_expr} LIKE :{like_param_single})" + f"({json_expr} @> CAST(:{tag_param} AS jsonb) OR {like_condition})" ) conditions.append(" AND ".join(tag_conditions)) continue diff --git a/src/basic_memory/repository/search_repository_base.py b/src/basic_memory/repository/search_repository_base.py index b76f92c0f..daa561778 100644 --- a/src/basic_memory/repository/search_repository_base.py +++ b/src/basic_memory/repository/search_repository_base.py @@ -206,6 +206,57 @@ def metadata_filter_content_type_condition(params: Dict[str, Any]) -> str: return "entity.content_type = :metadata_filter_content_type" +# SQLite's LIKE has no default escape character, and Postgres's is already the +# backslash, so naming this one explicitly in every pattern is what lets a single +# escaped pattern mean the same thing on both backends. +_LIKE_ESCAPE_CHARACTER = "\\" + + +def metadata_contains_like_condition( + extract_expr: str, + value: Any, + *, + param_prefix: str, + params: Dict[str, Any], +) -> str: + """Build the compatibility half of an array-contains metadata filter. + + The primary half of a ``{"tags": ["security"]}`` filter asks JSON whether the + array holds the element — ``json_each`` on SQLite, ``@>`` on Postgres — and + answers only when the stored value really is a JSON array. Frontmatter + written before tags were normalized can hold the array's *text* instead, + either JSON-quoted ('["security", "auth"]') or as a Python repr + ("['security', 'auth']"), and only a substring match finds an element inside + those. Hence a pattern per quote style, and hence the pattern-language + problem this function exists to solve. + + LIKE reads "%" and "_" in the searched-for value as wildcards, so + interpolating the value raw turned `tags has 100%` into a pattern that also + matched "100-percent" — a wrong hit and an inflated exact total, produced by + the branch the caller only meant as a fallback. Escaping both wildcards and + the escape character itself makes the value literal again. + + Shared by both backends for the same reason the subtree scope is: a filter + that admits different rows per dialect would report an exact total for a + match set the other never produces. + """ + escaped = ( + str(value) + .replace(_LIKE_ESCAPE_CHARACTER, _LIKE_ESCAPE_CHARACTER * 2) + .replace("%", f"{_LIKE_ESCAPE_CHARACTER}%") + .replace("_", f"{_LIKE_ESCAPE_CHARACTER}_") + ) + double_quoted_param = f"{param_prefix}_like" + single_quoted_param = f"{param_prefix}_like_single" + params[double_quoted_param] = f'%"{escaped}"%' + params[single_quoted_param] = f"%'{escaped}'%" + escape_clause = f" ESCAPE '{_LIKE_ESCAPE_CHARACTER}'" + return ( + f"{extract_expr} LIKE :{double_quoted_param}{escape_clause} " + f"OR {extract_expr} LIKE :{single_quoted_param}{escape_clause}" + ) + + async def purge_stale_search_index_rows( session_maker: async_sessionmaker[AsyncSession], project_id: int, diff --git a/src/basic_memory/repository/sqlite_search_repository.py b/src/basic_memory/repository/sqlite_search_repository.py index 03f287a0c..8f5983066 100644 --- a/src/basic_memory/repository/sqlite_search_repository.py +++ b/src/basic_memory/repository/sqlite_search_repository.py @@ -31,6 +31,7 @@ from basic_memory.repository.search_repository_base import ( SearchRepositoryBase, file_path_prefix_condition, + metadata_contains_like_condition, metadata_filter_content_type_condition, ) from basic_memory.repository.script_ngrams import analyze_script_query @@ -986,10 +987,14 @@ async def _build_fts_query_parts( for j, val in enumerate(filt.value): value_param = f"meta_val_{idx}_{j}" params[value_param] = val - like_param = f"{value_param}_like" - params[like_param] = f'%"{val}"%' - like_param_single = f"{value_param}_like_single" - params[like_param_single] = f"%'{val}'%" + # The exact JSON-membership test is the primary path; the + # substring patterns only reach values stored as array text. + like_condition = metadata_contains_like_condition( + extract_expr, + val, + param_prefix=value_param, + params=params, + ) json_each_expr = ( "json_each(entity.tags_json)" if use_tags_column @@ -998,8 +1003,7 @@ async def _build_fts_query_parts( tag_conditions.append( "(" f"EXISTS (SELECT 1 FROM {json_each_expr} WHERE value = :{value_param}) " - f"OR {extract_expr} LIKE :{like_param} " - f"OR {extract_expr} LIKE :{like_param_single}" + f"OR {like_condition}" ")" ) conditions.append(" AND ".join(tag_conditions)) diff --git a/tests/repository/test_metadata_filters_edge_cases.py b/tests/repository/test_metadata_filters_edge_cases.py index 65d981e0f..59560400a 100644 --- a/tests/repository/test_metadata_filters_edge_cases.py +++ b/tests/repository/test_metadata_filters_edge_cases.py @@ -273,6 +273,118 @@ async def test_filter_contains_single_element_array(search_repository, session_m assert {r.id for r in results} == {entity_match.id} +@pytest.mark.asyncio +async def test_contains_filter_matches_an_array_stored_as_text(search_repository, session_maker): + """CONTRACT: the substring fallback is what reaches un-normalized frontmatter. + + A `tags` filter is answered first by an exact JSON-membership test, which + needs the stored value to really be an array. Frontmatter written before + tags were normalized can hold the array's *text* instead, in either quote + style, and only a substring match finds an element inside those. This is the + positive control for the escaping tests below: a fix that made the wildcard + leak impossible by disabling the fallback would pass those and fail here. + """ + json_text = await _index_entity_with_metadata( + search_repository, + session_maker, + "Tags As Json Text", + {"tags": '["security", "auth"]'}, + ) + python_repr = await _index_entity_with_metadata( + search_repository, + session_maker, + "Tags As Python Repr", + {"tags": "['security', 'auth']"}, + ) + await _index_entity_with_metadata( + search_repository, + session_maker, + "Tags As Text Without The Element", + {"tags": '["database", "migration"]'}, + ) + + results = await search_repository.search(metadata_filters={"tags": ["security"]}) + + assert {r.id for r in results} == {json_text.id, python_repr.id} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("literal_tag", "lookalike_tag"), + [ + # "%" is LIKE's any-run wildcard: the unescaped pattern for "100%" read + # as '"100' + anything + '"', which "100-percent" satisfies. + ("100%", "100-percent"), + # "_" is LIKE's any-single-character wildcard. + ("a_c", "abc"), + # The escape character itself. Escaping only the two wildcards would + # leave this value's "%" preceded by an escaped backslash and therefore + # still live as a wildcard — so the pattern would match any run there. + ("a\\%c", "a\\zzzc"), + ], +) +async def test_contains_filter_treats_like_metacharacters_literally( + search_repository, session_maker, literal_tag, lookalike_tag +): + """REGRESSION: a `has` value is text to match, not a pattern to interpret. + + The exact JSON-membership half of a contains filter always read the value + literally; the substring fallback interpolated it straight into a LIKE + pattern, so a tag carrying "%" or "_" silently matched tags that merely + looked like it. The wrong rows counted into the exact total too, which is + what makes this a wrong answer rather than a cosmetic one. + + The literal tag still has to match — via the exact half — or the escaping + would just be a way to match nothing. + """ + literal = await _index_entity_with_metadata( + search_repository, + session_maker, + "Tag Literal", + {"tags": [literal_tag]}, + ) + await _index_entity_with_metadata( + search_repository, + session_maker, + "Tag Lookalike", + {"tags": [lookalike_tag]}, + ) + + results = await search_repository.search(metadata_filters={"tags": [literal_tag]}) + + assert {r.id for r in results} == {literal.id} + # The total is what paginates, so the lookalike must be out of it too. + assert await search_repository.count(metadata_filters={"tags": [literal_tag]}) == 1 + + +@pytest.mark.asyncio +async def test_contains_filter_escapes_metacharacters_on_a_nested_path( + search_repository, session_maker +): + """The nested path takes the generic json_extract branch, not the tags column. + + SQLite answers a bare `tags` filter from its generated `tags_json` column and + anything else from a json_extract call, so the escaping has to hold on both + expressions rather than on the one the tags tests happen to exercise. + """ + literal = await _index_entity_with_metadata( + search_repository, + session_maker, + "Nested Labels Literal", + {"review": {"labels": ["100%"]}}, + ) + await _index_entity_with_metadata( + search_repository, + session_maker, + "Nested Labels Lookalike", + {"review": {"labels": ["100-percent"]}}, + ) + + results = await search_repository.search(metadata_filters={"review.labels": ["100%"]}) + + assert {r.id for r in results} == {literal.id} + + @pytest.mark.asyncio async def test_filter_nested_path_missing_intermediate(search_repository, session_maker): """Filtering on a nested path where intermediate keys are missing returns no match.""" From 6bf33b40159a83c4a78ebe0924f722488baa61d4 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 16:59:26 -0500 Subject: [PATCH 07/12] fix(mcp): refuse malformed metadata predicate keys before transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit find's predicate key capture class admits '.' anywhere, so a malformed dot path — 'review..approved', '.owner', 'owner.' — parsed cleanly and travelled to the search API, which refuses it. Every other predicate mistake in this grammar is refused locally, naming the offending value and teaching the grammar; this one spent a request to come back with "Unsupported metadata filter key", wording that names neither find nor the shape a key must have. Validate the key against METADATA_KEY_RE — the search API parser's own key grammar, lifted from a private name so there is one definition rather than a second copy free to drift from what the repository actually accepts. The capture class stays deliberately loose: tightening it would make '.owner=null' match no regex at all and be reported as a missing operator rather than as the bad key it is. No router change. The reviewer's report that this reaches the caller as a 500 does not reproduce: the v2 search router has mapped ValueError to a 400 since d1d6f273b, and all three keys already returned 400 with a readable detail. Added router tests pinning that 400 so it stays a client error. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/man/man1/find(1).md | 7 +- src/basic_memory/mcp/tools/posix_tools.py | 24 +++++- .../repository/metadata_filters.py | 10 ++- tests/api/v2/test_search_router.py | 35 ++++++++ tests/mcp/test_tool_posix.py | 83 ++++++++++++++++++- 5 files changed, 150 insertions(+), 9 deletions(-) diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index be4dd47a3..ac8433d3d 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -106,8 +106,11 @@ text. Keys accept dot-paths into nested frontmatter (`review.approved`), and `note_type` is accepted as a spelling of the frontmatter `type` key, matching -`search-notes(3)`. Any other operator (`!=` among them — the search API has -no not-equals) fails fast, naming the supported set. That includes a +`search-notes(3)`. A key is dot-separated names of letters, digits, `_` or +`-`, so a doubled, leading or trailing dot (`review..approved`, `.owner`, +`owner.`) is refused by the grammar rather than spent as a request the +search API will reject. Any other operator (`!=` among them — the search +API has no not-equals) fails fast, naming the supported set. That includes a mis-spelled multi-character operator: `status==active`, `status=>active` and `count>>3` are refused rather than read as the values `=active`, `>active` and `>3`. An unquoted value may therefore not begin with `=`, `<` or `>`; diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index bf31da21b..8f4d3cf58 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -50,6 +50,7 @@ resolve_project_path_route, ) from basic_memory.mcp.server import POSIX_TOOLS_TAG, mcp, set_posix_tools_visibility +from basic_memory.repository.metadata_filters import METADATA_KEY_RE from basic_memory.schemas.directory import ( DEFAULT_DIRECTORY_PAGE_SIZE, MAX_DIRECTORY_PAGE_SIZE, @@ -546,6 +547,11 @@ async def find_listing( # them and symbol ops exclude the key character class, so exactly one regex can # match any given predicate. Two-char symbols sit first in the alternation so # ">=" never parses as ">" plus a value starting with "=". +# The key capture admits '.' anywhere on purpose — it is looser than a dot path. +# METADATA_KEY_RE, the search API's own key grammar, is what decides a +# well-formed one, checked in _parse_meta_predicates once the predicate has +# split. Tightening the capture instead would make '.owner=null' match no regex +# at all and be reported as a missing operator rather than as the bad key it is. _PREDICATE_WORD_RE = re.compile(r"^([A-Za-z0-9_.-]+)\s+(in|has|between)\s+(.+)$") _PREDICATE_SYMBOL_RE = re.compile(r"^([A-Za-z0-9_.-]+)\s*(>=|<=|=|>|<)\s*(.*)$") _SYMBOL_OPERATORS = {">": "$gt", ">=": "$gte", "<": "$lt", "<=": "$lte"} @@ -701,7 +707,8 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: One predicate per string; predicates AND together. Exactly one predicate per key — the API admits one operator per key, so a repeated key fails fast instead of last-wins. Raises ValueError (surfaced to MCP callers as - ToolError) on any operator outside the supported set. + ToolError) on any operator outside the supported set, and on any key outside + the search API's dot-path grammar. """ filters: dict[str, Any] = {} for predicate in predicates: @@ -715,6 +722,21 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: ) raw_key, op, raw_value = match.groups() key = _METADATA_KEY_ALIASES.get(raw_key, raw_key) + # Trigger: the key capture accepted something that is not a dot path — + # a doubled, leading or trailing dot ('review..approved', + # '.owner', 'owner.'). + # Why: the search API refuses these keys, so the query was never going + # to run. Letting it travel spends a request to come back with + # "Unsupported metadata filter key", which names neither find nor + # the shape a key must have — every other predicate mistake is + # refused here, before transport, in find's own words. + # Outcome: refuse locally, naming the offending key and the grammar. + if not METADATA_KEY_RE.match(key): + raise ValueError( + f"find: malformed predicate key '{key}' in '{predicate}'; keys are " + "dot-separated names of letters, digits, '_' or '-' " + "(e.g. 'status' or 'review.approved')" + ) if key in filters: raise ValueError( f"find: duplicate predicate key '{key}' in '{predicate}'; " diff --git a/src/basic_memory/repository/metadata_filters.py b/src/basic_memory/repository/metadata_filters.py index faba459d1..e88cea419 100644 --- a/src/basic_memory/repository/metadata_filters.py +++ b/src/basic_memory/repository/metadata_filters.py @@ -8,7 +8,13 @@ from typing import Any, Iterable, List, cast -_KEY_RE = re.compile(r"^[A-Za-z0-9_-]+(\.[A-Za-z0-9_-]+)*$") +# The one definition of a well-formed metadata key: dot-separated name segments +# of letters, digits, '_' or '-', so a doubled, leading or trailing dot is not a +# path. Public because callers that build filters — the POSIX find predicate +# grammar — refuse a malformed key against this same pattern before spending a +# request; a private copy there would be free to drift from what this parser +# actually accepts. +METADATA_KEY_RE = re.compile(r"^[A-Za-z0-9_-]+(\.[A-Za-z0-9_-]+)*$") _NUMERIC_RE = re.compile(r"^-?\d+(\.\d+)?$") _COMPARISON_OPERATORS = { "$gt": "gt", @@ -95,7 +101,7 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter if not isinstance(raw_key, str) or not raw_key.strip(): raise ValueError("metadata filter keys must be non-empty strings") key = raw_key.strip() - if not _KEY_RE.match(key): + if not METADATA_KEY_RE.match(key): raise ValueError(f"Unsupported metadata filter key: {raw_key}") path_parts = key.split(".") diff --git a/tests/api/v2/test_search_router.py b/tests/api/v2/test_search_router.py index 95ea7342c..2e729520c 100644 --- a/tests/api/v2/test_search_router.py +++ b/tests/api/v2/test_search_router.py @@ -405,6 +405,41 @@ async def test_v2_search_endpoints_use_project_id_not_name( assert response.status_code in [404, 422] +@pytest.mark.asyncio +@pytest.mark.parametrize("key", ["review..approved", ".owner", "owner."]) +async def test_search_router_returns_400_for_malformed_metadata_key( + client: AsyncClient, v2_project_url, key: str +): + """A key outside the dot-path grammar is a client error, not a server fault. + + parse_metadata_filters refuses these deep in the repository, and the + router's ValueError arm is what keeps the refusal a 400 carrying the + offending key — an unhandled ValueError here would read as a 500, blaming + the server for a caller's typo. Runs against the real search service so + the whole request path is what is being pinned. + """ + response = await client.post( + f"{v2_project_url}/search/", + json={"text": "test", "metadata_filters": {key: "x"}}, + ) + + assert response.status_code == 400 + assert response.json()["detail"] == f"Unsupported metadata filter key: {key}" + + +@pytest.mark.asyncio +async def test_search_router_accepts_a_well_formed_nested_metadata_key( + client: AsyncClient, v2_project_url +): + """The nested dot path the malformed spellings are typos of still works.""" + response = await client.post( + f"{v2_project_url}/search/", + json={"text": "test", "metadata_filters": {"review.approved": "True"}}, + ) + + assert response.status_code == 200 + + @pytest.mark.asyncio async def test_search_router_returns_400_for_semantic_disabled( client: AsyncClient, app, v2_project_url diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index 6b57745c7..76a05f431 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -25,7 +25,11 @@ ) from basic_memory.mcp.tools import cat, find, grep, ls, man, search_notes, tail, write_note from basic_memory.models import Entity -from basic_memory.repository.metadata_filters import ParsedMetadataFilter, parse_metadata_filters +from basic_memory.repository.metadata_filters import ( + METADATA_KEY_RE, + ParsedMetadataFilter, + parse_metadata_filters, +) from basic_memory.schemas.search import SearchRetrievalMode @@ -686,6 +690,69 @@ def test_malformed_operator_refusal_teaches_the_grammar_and_the_escape(): assert 'quote the value as "=active"' in message +@pytest.mark.parametrize( + "predicate", + [ + # Symbol operators: a doubled, leading or trailing dot is not a dot path. + "review..approved=true", + ".owner=null", + "owner.=x", + "a..b>1", + ".score>=0.5", + "trailing.<9", + # Word operators capture the key the same way, so they fold identically. + "review..approved in a,b", + ".tags has security", + "owner. between 1,2", + ], +) +def test_malformed_predicate_keys_refuse_before_transport(predicate): + """REGRESSION: a key that is not a dot path is refused here, not by the API. + + The key capture class admits '.' anywhere, so 'review..approved', '.owner' + and 'owner.' each parsed cleanly and travelled to the search API — which + refuses them, spending a request to answer "Unsupported metadata filter + key" in wording that names neither find nor the shape a key must have. + """ + with pytest.raises(ValueError, match="malformed predicate key"): + posix_tools._parse_meta_predicates([predicate]) + + +def test_malformed_key_refusal_names_the_key_and_the_grammar(): + """The refusal names the offending key and the shape a valid one has.""" + with pytest.raises(ValueError) as excinfo: + posix_tools._parse_meta_predicates(["review..approved=true"]) + + message = str(excinfo.value) + assert "malformed predicate key 'review..approved' in 'review..approved=true'" in message + assert "dot-separated names of letters, digits, '_' or '-'" in message + assert "review.approved" in message + + +@pytest.mark.parametrize( + "key", + ["review..approved", ".owner", "owner.", "review.approved", "status", "note-1_a.b"], +) +def test_predicate_key_grammar_is_the_api_key_grammar(key): + """find accepts exactly the keys the search API accepts, from one pattern. + + METADATA_KEY_RE is the API parser's own key grammar, and find validates + against it rather than a second copy — so the two surfaces cannot drift + into a state where find builds a filter the repository will then reject at + request time. + """ + api_accepts = METADATA_KEY_RE.match(key) is not None + + if api_accepts: + assert posix_tools._parse_meta_predicates([f"{key}=x"]) == {key: "x"} + assert parse_metadata_filters({key: "x"}) + else: + with pytest.raises(ValueError, match="malformed predicate key"): + posix_tools._parse_meta_predicates([f"{key}=x"]) + with pytest.raises(ValueError, match="Unsupported metadata filter key"): + parse_metadata_filters({key: "x"}) + + @pytest.mark.parametrize( "predicate", [ @@ -1317,9 +1384,17 @@ async def racing_get_entity(self, entity_id, **kwargs): @pytest.mark.asyncio -async def test_find_meta_unsupported_key_surfaces_the_api_error(client, test_project, meta_notes): - """The server owns key validation; its refusal reaches the caller intact.""" - with pytest.raises(ToolError, match="metadata filter key"): +async def test_find_meta_malformed_key_refuses_before_transport(client, test_project, meta_notes): + """A malformed key is refused in find's words, before a request is built. + + The API parser stays the authority on what a key may look like — find + validates against METADATA_KEY_RE, its grammar, so the two cannot + disagree — but spending a search request to be told "Unsupported metadata + filter key" told the caller neither which predicate was wrong nor what a + key must look like. Every other predicate mistake refuses here; this one + now does too. + """ + with pytest.raises(ValueError, match=re.escape("malformed predicate key 'status.'")): await find(meta=["status.=active"], project=test_project.name) From 7b213a4a8b32e0b1bcd9379f30de24e6c45982c7 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 17:49:14 -0500 Subject: [PATCH 08/12] fix(mcp): parse every frontmatter path through one grammar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit find's --fields took the same dotted frontmatter paths as --meta predicates but validated none of them. `_project_metadata_fields` split the string itself, so an empty segment walked to null: `.owner` reported null for every hit on notes that all carry `owner`, byte-identical to the null a genuinely absent field produces. Unlike a bad predicate key — which at least reached a server that refused it — this had nothing to fail against, so a typo came back as a uniform, plausible, wrong answer, after paying the search and one entity GET per hit. Fixed as coverage of one rule rather than a second check. metadata_filters now owns a MetadataPath value and parse_metadata_path, and the key regex goes back to private: the parse is the only way to apply the grammar, and `parts` exists only on a parsed path. All three surfaces that accept a caller-written path — the search API's metadata_filters, --meta predicates, --fields projection — go through it, and there is no longer a second `.split(".")` in the codebase to forget. That is what makes it closed rather than patched. A fourth consumer cannot repeat this: `_project_metadata_fields` takes list[MetadataPath], so handing it raw strings is a type error, and parse_metadata_path returns MetadataPath | None, so reaching `.parts` without handling the invalid case is a type error. Both are rejected by `ty` today, not by review vigilance. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez --- src/basic_memory/man/man1/find(1).md | 4 +- src/basic_memory/mcp/tools/posix_tools.py | 46 ++++++-- .../repository/metadata_filters.py | 56 +++++++-- tests/mcp/test_tool_posix.py | 108 ++++++++++++++++-- 4 files changed, 182 insertions(+), 32 deletions(-) diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index ac8433d3d..f920a4502 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -125,7 +125,9 @@ quote one that genuinely does, as in `range=">=5"`. - **--meta** — frontmatter predicate, repeatable; see PREDICATE GRAMMAR. Switches the payload to the search response shape - **--fields** — comma-separated frontmatter fields to show per hit, e.g. - `"title,priority"`; dot-paths allowed. Requires `--meta` + `"title,priority"`; dot-paths allowed, in the same shape predicate keys + take, and a malformed one is refused rather than shown as null for every + hit. A field a note does not carry shows as null. Requires `--meta` - **--page, --page-size** — pagination (defaults 1 and 10) ## EXAMPLES diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index 8f4d3cf58..bec7aa811 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -50,7 +50,7 @@ resolve_project_path_route, ) from basic_memory.mcp.server import POSIX_TOOLS_TAG, mcp, set_posix_tools_visibility -from basic_memory.repository.metadata_filters import METADATA_KEY_RE +from basic_memory.repository.metadata_filters import MetadataPath, parse_metadata_path from basic_memory.schemas.directory import ( DEFAULT_DIRECTORY_PAGE_SIZE, MAX_DIRECTORY_PAGE_SIZE, @@ -548,8 +548,8 @@ async def find_listing( # match any given predicate. Two-char symbols sit first in the alternation so # ">=" never parses as ">" plus a value starting with "=". # The key capture admits '.' anywhere on purpose — it is looser than a dot path. -# METADATA_KEY_RE, the search API's own key grammar, is what decides a -# well-formed one, checked in _parse_meta_predicates once the predicate has +# parse_metadata_path, which owns the frontmatter path grammar, is what decides +# a well-formed one, applied in _parse_meta_predicates once the predicate has # split. Tightening the capture instead would make '.owner=null' match no regex # at all and be reported as a missing operator rather than as the bad key it is. _PREDICATE_WORD_RE = re.compile(r"^([A-Za-z0-9_.-]+)\s+(in|has|between)\s+(.+)$") @@ -731,7 +731,7 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: # the shape a key must have — every other predicate mistake is # refused here, before transport, in find's own words. # Outcome: refuse locally, naming the offending key and the grammar. - if not METADATA_KEY_RE.match(key): + if parse_metadata_path(key) is None: raise ValueError( f"find: malformed predicate key '{key}' in '{predicate}'; keys are " "dot-separated names of letters, digits, '_' or '-' " @@ -764,22 +764,27 @@ def _parse_meta_predicates(predicates: list[str]) -> dict[str, Any]: def _project_metadata_fields( - entity_metadata: dict[str, Any] | None, fields: list[str] + entity_metadata: dict[str, Any] | None, fields: list[MetadataPath] ) -> dict[str, Any]: """Project requested frontmatter fields out of an entity's metadata. Field names are echoed verbatim as keys (dot-paths walk nested dicts). A missing key or non-dict intermediate yields None — never a dropped row. + + Takes parsed paths rather than strings because null here is a real answer + ("this note has no such field"), so a malformed path that walked to null + would be indistinguishable from data. Requiring MetadataPath moves that + refusal to the one parse that can tell the two apart. """ projected: dict[str, Any] = {} - for field_name in fields: + for field in fields: value: Any = entity_metadata - for part in field_name.split("."): + for part in field.parts: if not isinstance(value, dict): value = None break value = value.get(part) - projected[field_name] = value + projected[field.key] = value return projected @@ -892,6 +897,7 @@ async def find( "find: 'depth' cannot combine with 'meta' — the metadata search scopes " "by whole subtree; scope with 'path' instead" ) + projected_fields: list[MetadataPath] | None = None if fields is not None: if meta is None: raise ValueError( @@ -901,6 +907,26 @@ async def find( fields = [field_name.strip() for field_name in fields] if not fields or any(not field_name for field_name in fields): raise ValueError("find: 'fields' entries must be non-empty frontmatter field names") + # Trigger: a field path that is not a dot path ('review..approved', + # '.owner', 'owner.'). + # Why: projection answers a missing field with null, so an empty segment + # walked to null for every hit and read exactly like a field the + # notes genuinely do not carry — a typo returning a uniform, + # plausible, wrong answer, after paying the search and one entity + # GET per hit. Predicates at least reached a server that refused + # them; this one had nothing to fail against. + # Outcome: refuse before routing, through the same parse the predicate + # keys use, so the two cannot diverge. + projected_fields = [] + for field_name in fields: + field_path = parse_metadata_path(field_name) + if field_path is None: + raise ValueError( + f"find: malformed field path '{field_name}'; field paths are " + "dot-separated names of letters, digits, '_' or '-' " + "(e.g. 'title' or 'review.approved')" + ) + projected_fields.append(field_path) metadata_filters = _parse_meta_predicates(meta) if meta is not None else None # Trigger: predicates are present, so this call queries metadata rather than @@ -941,7 +967,7 @@ async def find( # to, a mount point rather than a subtree — onto "no scope". scope=route.path, metadata_filters=metadata_filters, - fields=fields, + fields=projected_fields, page=page, page_size=page_size, # The route's id, not the caller's raw param: it is what keeps a @@ -968,7 +994,7 @@ async def _find_by_metadata( route_project: Optional[str], scope: str, metadata_filters: dict[str, Any], - fields: Optional[list[str]], + fields: Optional[list[MetadataPath]], page: int, page_size: int, project_id: Optional[str], diff --git a/src/basic_memory/repository/metadata_filters.py b/src/basic_memory/repository/metadata_filters.py index e88cea419..fd72ce371 100644 --- a/src/basic_memory/repository/metadata_filters.py +++ b/src/basic_memory/repository/metadata_filters.py @@ -1,4 +1,9 @@ -"""Helpers for parsing structured metadata filters for search.""" +"""The frontmatter metadata path grammar, and the filters built on top of it. + +Every surface that accepts a caller-written dot path into frontmatter — the +search API's ``metadata_filters``, find's ``--meta`` predicates, find's +``--fields`` projection — parses it here, through `parse_metadata_path`. +""" from __future__ import annotations @@ -8,13 +13,11 @@ from typing import Any, Iterable, List, cast -# The one definition of a well-formed metadata key: dot-separated name segments -# of letters, digits, '_' or '-', so a doubled, leading or trailing dot is not a -# path. Public because callers that build filters — the POSIX find predicate -# grammar — refuse a malformed key against this same pattern before spending a -# request; a private copy there would be free to drift from what this parser -# actually accepts. -METADATA_KEY_RE = re.compile(r"^[A-Za-z0-9_-]+(\.[A-Za-z0-9_-]+)*$") +# Dot-separated name segments of letters, digits, '_' or '-', so a doubled, +# leading or trailing dot is not a path. Private on purpose: `parse_metadata_path` +# is the only way to apply it, which is what keeps the check and the split that +# depends on it from drifting apart in a caller. +_METADATA_KEY_RE = re.compile(r"^[A-Za-z0-9_-]+(\.[A-Za-z0-9_-]+)*$") _NUMERIC_RE = re.compile(r"^-?\d+(\.\d+)?$") _COMPARISON_OPERATORS = { "$gt": "gt", @@ -24,6 +27,37 @@ } +@dataclass(frozen=True) +class MetadataPath: + """A dot path into a note's frontmatter, already checked against the grammar. + + `parts` lives here and nowhere else, and only `parse_metadata_path` builds + one — so walking a caller-written path requires having validated it first. + That is the entire point of the type. Splitting the string at the call site + is what let `--fields` accept `review..approved` and quietly walk an empty + segment to null for every hit, indistinguishable from a field that is + genuinely absent; there is no longer a second `.split(".")` to forget. + """ + + key: str + parts: tuple[str, ...] + + +def parse_metadata_path(raw_key: str) -> MetadataPath | None: + """Parse one frontmatter dot path, or None when the text is not one. + + Returns None instead of raising so each surface refuses in its own words — + the search API, find's predicates and find's field projection all word it + differently. The optional return is also the enforcement: a caller that + skips the check has a `MetadataPath | None` and cannot reach `.parts` + without the type checker objecting. + """ + key = raw_key.strip() + if not _METADATA_KEY_RE.match(key): + return None + return MetadataPath(key, tuple(key.split("."))) + + @dataclass(frozen=True) class ParsedMetadataFilter: """Normalized metadata filter for SQL generation.""" @@ -100,11 +134,11 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter for raw_key, raw_value in (filters or {}).items(): if not isinstance(raw_key, str) or not raw_key.strip(): raise ValueError("metadata filter keys must be non-empty strings") - key = raw_key.strip() - if not METADATA_KEY_RE.match(key): + path = parse_metadata_path(raw_key) + if path is None: raise ValueError(f"Unsupported metadata filter key: {raw_key}") - path_parts = key.split(".") + path_parts = list(path.parts) # Operator form if isinstance(raw_value, dict): diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index 76a05f431..0095dd87d 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -26,9 +26,10 @@ from basic_memory.mcp.tools import cat, find, grep, ls, man, search_notes, tail, write_note from basic_memory.models import Entity from basic_memory.repository.metadata_filters import ( - METADATA_KEY_RE, + MetadataPath, ParsedMetadataFilter, parse_metadata_filters, + parse_metadata_path, ) from basic_memory.schemas.search import SearchRetrievalMode @@ -733,17 +734,16 @@ def test_malformed_key_refusal_names_the_key_and_the_grammar(): "key", ["review..approved", ".owner", "owner.", "review.approved", "status", "note-1_a.b"], ) -def test_predicate_key_grammar_is_the_api_key_grammar(key): - """find accepts exactly the keys the search API accepts, from one pattern. +def test_one_path_grammar_governs_predicates_and_filters(key): + """find's predicates and the search API accept exactly the same paths. - METADATA_KEY_RE is the API parser's own key grammar, and find validates - against it rather than a second copy — so the two surfaces cannot drift - into a state where find builds a filter the repository will then reject at - request time. + parse_metadata_path owns the grammar, and both surfaces call it rather than + keeping a copy — so they cannot drift into a state where find builds a + filter the repository will then reject at request time. """ - api_accepts = METADATA_KEY_RE.match(key) is not None + well_formed = parse_metadata_path(key) is not None - if api_accepts: + if well_formed: assert posix_tools._parse_meta_predicates([f"{key}=x"]) == {key: "x"} assert parse_metadata_filters({key: "x"}) else: @@ -753,6 +753,33 @@ def test_predicate_key_grammar_is_the_api_key_grammar(key): parse_metadata_filters({key: "x"}) +@pytest.mark.parametrize( + ("key", "expected_parts"), + [ + ("status", ("status",)), + ("review.approved", ("review", "approved")), + ("a.b.c", ("a", "b", "c")), + ("note-1_a.b", ("note-1_a", "b")), + (" padded.key ", ("padded", "key")), + ], +) +def test_parse_metadata_path_yields_the_segments(key, expected_parts): + """The parse is what produces the segments a path walk consumes.""" + path = parse_metadata_path(key) + + assert path is not None + assert path.parts == expected_parts + assert path.key == key.strip() + + +@pytest.mark.parametrize( + "key", ["review..approved", ".owner", "owner.", "", " ", "..", "a..b.c", "bad key"] +) +def test_parse_metadata_path_refuses_everything_that_is_not_a_path(key): + """No segments come back for a non-path, so nothing can walk one.""" + assert parse_metadata_path(key) is None + + @pytest.mark.parametrize( "predicate", [ @@ -882,7 +909,17 @@ def test_null_equality_reaches_the_api_as_an_is_null_clause(): ], ) def test_project_metadata_fields(entity_metadata, fields, expected): - assert posix_tools._project_metadata_fields(entity_metadata, fields) == expected + # Projection takes parsed paths, not strings: the fixtures go through the + # same parse find does, which is what makes an unwalkable path unreachable + # here rather than a null that looks like data. + assert posix_tools._project_metadata_fields(entity_metadata, _paths(fields)) == expected + + +def _paths(keys: list[str]) -> list[MetadataPath]: + """Parse well-formed test paths, asserting the fixtures really are paths.""" + parsed = [parse_metadata_path(key) for key in keys] + assert all(path is not None for path in parsed), keys + return [path for path in parsed if path is not None] # --- find --meta: metadata search arm --- @@ -1202,6 +1239,49 @@ async def test_find_meta_projects_requested_fields(client, test_project, meta_no } +@pytest.mark.asyncio +@pytest.mark.parametrize("field", ["review..approved", ".owner", "owner."]) +async def test_find_meta_malformed_field_path_refuses_without_reading_anything( + client, test_project, meta_notes, monkeypatch, field +): + """REGRESSION: a malformed field path returned null data at full cost. + + `.owner` walked an empty first segment and reported null for every hit even + though the notes carry `owner`, byte-identical to the null a genuinely + absent field produces — a typo the caller could not see, paid for with the + search plus one entity GET per hit. Nothing is read now. + """ + # Import here to mirror the tool's own deferred client import. + from basic_memory.mcp.clients import KnowledgeClient + + reads = 0 + original = KnowledgeClient.get_entity + + async def counting_get(self, external_id, *args, **kwargs): + nonlocal reads + reads += 1 + return await original(self, external_id, *args, **kwargs) + + monkeypatch.setattr(KnowledgeClient, "get_entity", counting_get) + + with pytest.raises(ValueError, match=re.escape(f"malformed field path '{field}'")): + await find(meta=["status=active"], fields=[field], project=test_project.name) + + assert reads == 0 + + +@pytest.mark.asyncio +async def test_find_meta_nested_field_path_still_projects(client, test_project, meta_notes): + """The well-formed nested path the malformed spellings are typos of works.""" + result = await find( + meta=["status=active"], fields=["review.approved"], project=test_project.name + ) + + projected = {row["title"]: row["fields"] for row in result["results"]} + assert projected["Alpha Spec"] == {"review.approved": "True"} + assert projected["Gamma Note"] == {"review.approved": None} + + @pytest.mark.asyncio @pytest.mark.parametrize( ("kwargs", "message"), @@ -1214,6 +1294,14 @@ async def test_find_meta_projects_requested_fields(client, test_project, meta_no ({"fields": ["title"]}, "'fields' requires 'meta' predicates"), ({"meta": ["status=active"], "fields": []}, "must be non-empty"), ({"meta": ["status=active"], "fields": [" "]}, "must be non-empty"), + # A malformed field path walked an empty segment to null for every hit, + # which reads exactly like a field the notes do not carry — a typo + # answered with uniform, plausible, wrong data. Refuse it like a + # predicate key, through the same parse. + ({"meta": ["status=active"], "fields": ["review..approved"]}, "malformed field path"), + ({"meta": ["status=active"], "fields": [".owner"]}, "malformed field path"), + ({"meta": ["status=active"], "fields": ["owner."]}, "malformed field path"), + ({"meta": ["status=active"], "fields": ["title", "a..b"]}, "malformed field path"), # An empty predicate list is not "no filter": it would parse to {} and # run the metadata search with no WHERE at all, matching every note in # the project where the caller asked for a filtered set. From 4f73c1686836ae4cf5438b51d38d67e09c339150 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 1 Sep 2026 18:44:21 -0500 Subject: [PATCH 09/12] docs(skills): use the POSIX read verbs in literary analysis, drop canvas (#1424) Signed-off-by: phernandez Co-authored-by: Claude Fable 5 --- skills/memory-literary-analysis/SKILL.md | 238 +++++++++++++++++++++-- 1 file changed, 221 insertions(+), 17 deletions(-) diff --git a/skills/memory-literary-analysis/SKILL.md b/skills/memory-literary-analysis/SKILL.md index 2627cfd3a..d36627715 100644 --- a/skills/memory-literary-analysis/SKILL.md +++ b/skills/memory-literary-analysis/SKILL.md @@ -1,11 +1,11 @@ --- name: memory-literary-analysis -description: "Analyze a complete literary work into a structured Basic Memory knowledge graph. Covers schema design, entity seeding, chapter-by-chapter processing, cross-referencing, validation, and visualization." +description: "Analyze a complete literary work into a structured Basic Memory knowledge graph. Covers schema design, entity seeding, chapter-by-chapter processing, cross-referencing, validation, and graph exploration." --- # Memory Literary Analysis -Transform a complete literary work into a structured knowledge graph. Characters, themes, chapters, locations, symbols, and literary devices become interconnected notes — searchable, validatable, and visualizable. +Transform a complete literary work into a structured knowledge graph. Characters, themes, chapters, locations, symbols, and literary devices become interconnected notes — searchable, validatable, and traversable. ## When to Use @@ -23,9 +23,54 @@ Phase 1: Seed → stub notes for known major entities Phase 2: Process → chapter-by-chapter notes in batches Phase 3: Cross-ref → enrich arcs, add parallels, write analysis Phase 4: Validate → schema checks, drift detection, consistency -Phase 5: Visualize → Obsidian canvas files for character webs, timelines +Phase 5: Explore → traverse the graph, write synthesis notes ``` +## Tools + +Writing always goes through `write_note` and `edit_note`. For *reading* — which is most of +the work in a long analysis — prefer the POSIX read verbs where they are available +(`enable_posix_tools` for the MCP tools; the `bm` CLI verbs are always available): + +| Need | Use | Instead of | +|------|-----|-----------| +| A section of a long note | `cat --section Observations` | reading the whole note | +| A line range of the source text | `cat .txt --lines 4200-4890` | pulling the whole book into context | +| Notes matching frontmatter | `find --meta status=active` | reading notes to check fields | +| Fields across many notes | `find --meta ... --fields pov,setting` | one read per note | +| Where something lives | `ls`, `tree`, `find --name '*.md'` | listing everything | + +The two rules that matter across a 100+ chapter run: + +- **Never read a note to check a field.** That is what `--meta` predicates and `--fields` + projection are for — one call answers what a read-per-note loop would cost. +- **Never pull a whole file into context to reach one part of it.** Sections and line ranges + slice the *output*: the full note is still fetched, then cut down before it is returned. What + they save is context, not I/O — a long chapter or a full source text costs you the tokens of + the relevant part, not of the whole file. + +These compound. In measured runs, predicate queries replaced 28-call scans with a single +call; across 138 chapters that difference is the run. + +Three sharp edges to know before you write a query. The first two fail *quietly* — a wrong +answer, exit 0, no warning — so learn them here rather than from a graph you thought you had +audited: + +- **`--meta` matches case-sensitively, and the stored value is always snake_case.** `note_type` + is an alias for the frontmatter `type:` key, compared with SQL `=`. `write_note` normalizes + `note_type` through `to_snake_case` before writing, so a note authored as `note_type="Chapter"` + is stored as `type: chapter` — the casing you author with is *not* the casing on disk. Query + the snake_case form: `--meta 'note_type=chapter'`. The capitalized spelling returns zero rows + and exit 0. The value a result row displays is the value to query with. +- **`find` pages, and the default page is 10.** Any query whose answer is "all N chapters" + needs `--page-size 200` (the maximum) — see [Coverage Checks](#coverage-checks). +- **`--name` cannot combine with `--meta`.** The metadata search matches slugified permalinks, + not filenames. Scope a `--meta` query with the positional path instead: `find /characters + --meta 'note_type=character'`. + +If the POSIX verbs are unavailable, every step below still works with `search_notes`, +`read_note`, and `list_directory` — it just costs more. + ## Phase 0: Setup ### Create the Project @@ -212,6 +257,7 @@ Schema for literary technique and device notes. ``` / + .txt # the source text, verbatim (see Phase 2) schema/ # 6 schema definitions chapters/ # one note per chapter/section + prologue/epilogue characters/ @@ -271,6 +317,46 @@ Stubs don't need to be complete — they give `[[wiki-link]]` targets and will b Obtain the full text and identify chapter/section boundaries. For public domain works, Project Gutenberg is a good source. For copyrighted works, work from a physical or licensed digital copy. +**Put the source text inside the project directory, as `.txt`, and index it once.** `bm cat` +resolves a *note identifier*, not a filesystem path — it can only reach a file the project +index has observed. A one-time index pass gives the raw text an entity row, after which the +line-range slice works against it: + +```bash +cp ~/Downloads/moby-dick.txt ~/basic-memory/moby-dick/moby-dick.txt +bm reindex --search -p moby-dick # one pass; the .txt becomes readable +``` + +Two constraints that make this the right shape, both worth respecting: + +- **Keep it `.txt`, do not convert it to `.md`.** Basic Memory injects frontmatter into + markdown notes, which shifts every line number by the height of that block — an offset map + built from the original file would then be silently wrong. A `.txt` is stored verbatim, so + its line numbers stay 1:1 with the file on disk. +- **Keep it inside the project.** A source text elsewhere on disk is not an entity, and + `bm cat` answers `Error: Entity not found`. If you must leave it outside, drop the BM verbs + for the source and use plain shell (`sed -n '4200,4890p' `) — the notes still get the + BM verbs, only the raw source falls back to the shell. + +**Then build a chapter offset map once, before processing.** Scan the text for chapter +headings and record the line range of each chapter, then read chapters by range rather than +re-reading the whole book into context: + +```bash +grep -n '^CHAPTER ' ~/basic-memory/moby-dick/moby-dick.txt # heading -> line number +bm cat moby-dick.txt --lines 4200-4890 --plain # returns one chapter, not the whole text +``` + +`grep -n` here is the shell's grep on a filesystem path (this is the map-building step, and +it needs the file). `bm cat` then takes the *note identifier* — `moby-dick.txt`, the file's +path within the project — and returns exactly that slice plus a `lines 4200-4890 of N` +footer. `bm head moby-dick.txt -n 40` is the cheap way to eyeball the heading format before +writing the grep pattern. + +Store the map in the project (a note or a small JSON file) so later batches — and a resumed +run after context compaction — do not have to rediscover it. On a long work this is the +single largest context saving in the pipeline. + ### Batching Strategy Process ~10 chapters per batch to balance depth with progress. Group by narrative arc or thematic focus: @@ -289,7 +375,9 @@ Adjust batch size based on chapter length and density. Short, action-heavy chapt For each chapter: -**1. Read the chapter carefully.** If working from a source text file, read the relevant section. +**1. Read the chapter carefully.** Read the chapter's line range from the offset map +(`bm cat .txt --lines -`), not the whole file. Read the actual text — +never work from memory or a summary; textual evidence is the entire point. **2. Create the chapter note:** @@ -377,6 +465,26 @@ The prose adds the interpretive texture that structured observations alone canno After all chapters are processed: +### Find What Needs Enriching + +Do not re-read every note to decide what is thin. Query for it: + +```bash +bm find --meta 'note_type=chapter' --fields chapter_number,pov,setting --page-size 200 +bm find --meta 'note_type=character' --fields role,status --page-size 200 # who is still a stub +bm find --meta 'chapter_number>100' --fields pov --page-size 200 # late-book POV drift +``` + +A field a note never set comes back as a blank cell (`null` under `--json`), so rows with +blanks are the work queue. This turns "audit the graph" from a read of every note into one +call per question. + +Note the lowercase `chapter`/`character` — `write_note` snake-cases `note_type` before the +note is written, so that is the value on disk no matter how your Phase 0 schemas spelled it. +Match it exactly; the capitalized spelling returns zero rows and exit 0. And `--page-size 200` +is not decoration: without it these return the first 10 rows and the work queue looks ten +items long. + ### Character Arcs For each major character, write a full `[arc]` summary observation covering their trajectory across the work. @@ -446,26 +554,120 @@ Fix issues found — common fixes: - Enum values outside allowed set → correct metadata - Fields in notes but not schema → add as optional to schema if legitimate +### Coverage Checks + +Schema validation proves notes match their shape. These prove the graph is *complete*: + +```bash +bm find --meta 'note_type=chapter' --fields chapter_number --page-size 200 # every chapter present? +bm find --meta 'note_type=chapter' --fields pov,setting --page-size 200 # missing context? +bm find /characters --meta 'note_type=character' --fields role --page-size 200 # inventory vs. seed list +``` + +**A coverage check that pages is not a coverage check.** `bm find` defaults to +`--page-size 10`, so the un-sized form of the first query "proves" a 138-chapter work has 10 +chapters. 200 is the maximum page size; past that, iterate with `--page 2`, `--page 3`, … . +Scope a `--meta` query with the positional path (`/characters`), never `--name` — the two +options are mutually exclusive, because metadata search matches slugified permalinks rather +than filenames. + +Read the count off the footer, not off the rows you can see. Every `find` result reports +`page 1 • total 138`, and appends `• more available (--page)` when the page truncated the +answer — that suffix appearing is the check *failing*, whatever the visible rows say. + +For the sequence gap — the failure that a count alone cannot catch — take the numbers from +`--json`, which carries `total`, `total_is_exact`, and `has_more`: + +```bash +expected=138; page=1; rows='[]' +while :; do + resp=$(bm find --meta 'note_type=chapter' --fields chapter_number \ + --page-size 200 --page $page --project --json) + rows=$(jq -n --argjson acc "$rows" --argjson r "$resp" \ + '$acc + [$r.results[] | {title, n: .fields.chapter_number}]') + [ "$(jq -r '.has_more' <<<"$resp")" = "true" ] || break + page=$((page + 1)) +done +jq -n --argjson rows "$rows" --argjson expected "$expected" ' + ([$rows[] | select(.n != null and (.n | tostring | test("^[0-9]+$"))) | .n | tonumber]) as $n + | { total: ($rows | length), + unnumbered: [$rows[] | select(.n == null or (.n | tostring | test("^[0-9]+$") | not)) + | .title], + missing: ([range(1; $expected + 1)] - $n), + duplicates: ($n | group_by(.) | map(select(length > 1) | .[0])), + out_of_range: ($n | map(select(. < 1 or . > $expected)) | unique) }' +``` + +The loop is not ceremony. `--page-size` caps at 200, so a single call cannot inventory a work +with more than 200 chapters — and rerunning it with `--page 2` *replaces* the numbers rather +than accumulating them, which reports chapters 1-200 as missing on a corpus that is complete. +Walk until `has_more` is false and check the union. + +Pass the work's **actual** chapter count as `$expected` — deriving the range from the highest +number found lets an incomplete graph pass. With 138 rows numbered 1..137 plus one duplicate, +a max-derived check reports `missing: []` while a chapter is genuinely absent: the duplicate +keeps the count right and the missing tail moves the goalpost. The check passes on +`unnumbered: []`, `missing: []`, `duplicates: []`, **and** `out_of_range: []` together, over +the combined pages. + +`unnumbered` is not decoration either. A `chapter` note that never got a `chapter_number` comes +back as `null`, and feeding that straight to `tonumber` aborts the whole pipeline with `null +cannot be parsed as a number` — so the check *crashes on exactly the malformed inventory it +exists to find*. Partitioning first turns that into a named row. + +`out_of_range` is not hypothetical: a prologue or epilogue typed as `chapter` lands at 0 or at +`$expected + 1`, and without that key the report reads clean — every expected number present, +none repeated — while the inventory holds a note the numbering does not account for. Type +front and back matter as its own note type, or widen `$expected` deliberately. + +A gap in the middle of a batch is the most common processing failure and the easiest to miss +by eye; a duplicated chapter number is the second, and it hides the first. + ### Relation Consistency Spot-check bidirectional relations: if Chapter X `features [[Character]]`, does Character have observations referencing Chapter X? Fix gaps. -## Phase 5: Visualization +Orphans are the other half of this check — a note with no inbound or outbound relations is +either genuinely isolated or was never linked back into the graph: + +```bash +bm orphans # entities with no relations in the graph +``` + +Graph quality is relation *density*, not note count. A pass that adds notes while leaving +orphans behind has made the graph worse. + +## Phase 5: Explore the Graph -Write [JSON Canvas](https://jsoncanvas.org/) files (`.canvas`) into the project directory for visual exploration in Obsidian. Query the graph first (`search_notes`, `build_context`), then lay out the results as canvas nodes and edges: +With the graph complete, traverse it to find what the chapter-by-chapter pass could not see: -```json -{ - "nodes": [ - {"id": "ahab", "type": "file", "file": "characters/captain-ahab.md", "x": 0, "y": 0, "width": 400, "height": 300}, - {"id": "ishmael", "type": "file", "file": "characters/ishmael.md", "x": 500, "y": 0, "width": 400, "height": 300} - ], - "edges": [ - {"id": "e1", "fromNode": "ishmael", "toNode": "ahab", "label": "narrates"} - ] -} +```bash +bm tool build-context 'memory://characters/major/*' --depth 2 # the character web +bm find --meta 'note_type=theme' --fields prevalence --page-size 200 # thematic weight +bm grep -F "doubloon" --page-size 100 --project # every mention of a symbol ``` -Useful canvases: character relationship web (protagonist/antagonist/supporting), theme connections, chapter timeline with key events. +`build-context` takes its URL as a positional argument — there is no `--url` option. + +`grep` defaults to semantic ranking and a page of 10, which answers "what is this about?" but +quietly truncates "where does this appear?" — a symbol in 40 chapters comes back as 10. For +symbol tracing, pass `-F` for literal matching and raise `--page-size`; the meaning shifts you +are hunting are usually in the later occurrences, which the default would have dropped. + +`--page-size` raises the ceiling, it does not remove it. A symbol in a long work can exceed +even 100, so check whether the last page was full and walk `--page 2`, `--page 3` until it is +not. A truncated symbol search fails the same silent way as an unpaginated `find`: a plausible +answer, exit 0, and no sign that the tail is missing. + +And `grep` searches **your notes, not the source**. The `.txt` is indexed as an entity, +but its body is not in the searchable text, so an occurrence you never carried into a note is +unreachable — verified: a word present only in the source returns `total: 0` while a word in +both returns just the note. So this answers "where have I written about the doubloon", not +"where does the doubloon appear in the book". For the latter, search the file itself and use +the [chapter offset map](#source-text-preparation) to turn a hit into a chapter. + +Traversal is where second-order questions get answered — which characters share the most +chapters, which themes converge in the final act, where a symbol's meaning shifts. Capture +what you find as `analysis/` notes; those syntheses are the payoff of having built the graph. ## Adapting to Other Genres @@ -504,6 +706,8 @@ This pipeline works for any literary text. Adjust schemas for genre: - **Seed before processing.** Create entity stubs first so wiki-links resolve immediately during chapter processing. - **Batch for sanity.** Processing ~10 chapters at a time balances depth with momentum. Track progress with a Task note. - **Read the source text.** Don't rely on memory or summaries. Read (or re-read) the actual text for each batch before creating notes. Textual evidence is everything. +- **Read narrowly.** Keep the source text in the project as `.txt`, index it once, build the chapter offset map once, then read chapters by line range and notes by section. On a long work, whole files landing in context are the largest avoidable cost in the pipeline. +- **Query, don't scan.** When you need to know which notes have a field, ask with `--meta` predicates and `--fields` projection. Reading notes to check frontmatter is the mistake this pipeline makes at scale. Two ways these queries lie quietly: `--meta` is case-sensitive against the frontmatter `type:` your schemas authored, and `find` returns 10 rows unless you pass `--page-size`. - **Observations are your index.** The knowledge graph's value comes from categorized observations. Be generous with categories and specific with content. - **Relations are your web.** Every chapter should link to characters, themes, locations, and devices. Every entity should link back to chapters where it appears. - **Enrich iteratively.** Entity notes grow richer with each chapter. Don't try to write the perfect character note upfront — append as you go. From 703f8b8c7ed9e210dc2200b1a3195ec071e9e43c Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 2 Sep 2026 00:52:20 -0500 Subject: [PATCH 10/12] fix(mcp): return only identity and projection from find --fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--fields` is the SELECT to `--meta`'s WHERE, and the whole reason to call find instead of reading every note. It was decorating the full SearchResult instead of replacing it, so every projected row still carried the note's `content` — up to SearchIndexRow.CONTENT_DISPLAY_LIMIT (4000) characters. The 200-row inventory calls the literary-analysis skill documents therefore answered a request for two frontmatter values with most of a megabyte of prose, spending exactly the tokens the projection exists to save. Projection mode now builds the row rather than adding to it: the note's identity — title, permalink, file_path, external_id, updated_at — plus the `fields` object. A whitelist, not a content blocklist, so a SearchResult that later grows another bulky column cannot leak into a projected response. Left out: bodies (content, matched_chunk), a ranking no text query produced (score is -0.0 on every metadata hit), second spellings of an identity already in the row (entity, entity_id), and index-row metadata a caller can ask for by name instead (`--fields type`). Kept because callers read them: file_path and fields drive the CLI's projected renderers, title drives the skill's jq coverage loop, external_id is the hosted deep-link id (#1423). Only projection narrows. Without `--fields` the metadata arm still answers with the full search response grep's renderers read, because there the hit is the answer; a test pins that so the narrowing cannot spread. The CLI test's mock payload was a grep row with a `fields` key bolted on — a shape the tool no longer produces — so it is spelled out as a real projected row. Signed-off-by: phernandez --- src/basic_memory/man/man1/find(1).md | 15 ++++-- src/basic_memory/mcp/tools/posix_tools.py | 53 +++++++++++++++++--- tests/cli/test_cli_posix_verbs.py | 19 +++++-- tests/mcp/test_tool_posix.py | 60 +++++++++++++++++++++++ 4 files changed, 132 insertions(+), 15 deletions(-) diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index f920a4502..ffb23751d 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -58,10 +58,14 @@ directory named `" specs "` is addressed by that exact spelling. routing prefix, a mount point rather than a subtree, and scopes to that project's root. -`--fields` is the SELECT to the predicates' WHERE: it adds a `fields` object -to each hit carrying the named frontmatter values, so a filtered set can be -tabulated without reading every note. A field a hit does not carry renders as -null; the row is never dropped. +`--fields` is the SELECT to the predicates' WHERE: each hit comes back as the +note's identity — title, permalink, file path, external id, last-updated — plus +a `fields` object carrying the named frontmatter values, so a filtered set can +be tabulated without reading every note. A field a hit does not carry renders as +null; the row is never dropped. The projection *replaces* the note body rather +than riding alongside it, so a 200-row inventory answers with the values asked +for and not with 200 note bodies. Without `--fields`, hits keep the full search +shape, content included. `--name` and `--depth` are refused alongside `--meta`. The search API has no filename glob, and its path scope is whole-subtree, where a depth bound is not @@ -127,7 +131,8 @@ quote one that genuinely does, as in `range=">=5"`. - **--fields** — comma-separated frontmatter fields to show per hit, e.g. `"title,priority"`; dot-paths allowed, in the same shape predicate keys take, and a malformed one is refused rather than shown as null for every - hit. A field a note does not carry shows as null. Requires `--meta` + hit. A field a note does not carry shows as null. Projects each hit down to + its identity plus those fields — no note content. Requires `--meta` - **--page, --page-size** — pagination (defaults 1 and 10) ## EXAMPLES diff --git a/src/basic_memory/mcp/tools/posix_tools.py b/src/basic_memory/mcp/tools/posix_tools.py index bec7aa811..aa03ea254 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -788,6 +788,43 @@ def _project_metadata_fields( return projected +# --- What a projected row carries --- +# `fields` is the SELECT to the predicates' WHERE, and a SELECT answers with the +# columns asked for. A whole SearchResult carries the note body too — up to +# SearchIndexRow.CONTENT_DISPLAY_LIMIT (4000) characters of it — so the 200-row +# inventory call the literary-analysis skill documents answered a request for two +# frontmatter values with most of a megabyte of prose, which is the exact cost +# `fields` exists to remove. +# +# A whitelist rather than a content blocklist: the row is the note's identity — +# how to name it (title), read it (permalink, file_path) and deep-link it +# (external_id, #1423) — plus when it last changed and the projection itself. A +# SearchResult that later grows another bulky column therefore cannot leak into a +# projected response. What is left out is a body (content, matched_chunk), a +# ranking no text query produced (score, -0.0 on every metadata hit), a second +# spelling of an identity already here (entity, entity_id), or index-row metadata +# a caller can name as a field instead ("type"). +# +# Only projection mode narrows. Without `fields`, `meta` still answers with the +# full search response grep's renderers read, because there the hit *is* the +# answer. +_PROJECTED_ROW_KEYS = ("title", "permalink", "file_path", "external_id", "updated_at") + + +def _projected_row( + row: dict[str, Any], entity_metadata: dict[str, Any] | None, fields: list[MetadataPath] +) -> dict[str, Any]: + """One projected hit: the note's identity, plus the fields the caller asked for. + + Takes the already-dumped row so the identity values keep the response's own + JSON serialization (`updated_at` as an ISO string), and `fields` is injected + post-dump so a null field value survives the response's exclude_none. + """ + projected = {key: row[key] for key in _PROJECTED_ROW_KEYS if key in row} + projected["fields"] = _project_metadata_fields(entity_metadata, fields) + return projected + + @mcp.tool( title="Find", description=( @@ -858,7 +895,9 @@ async def find( Any other operator fails fast naming the supported set. fields: Frontmatter fields to return per hit (dot-paths allowed), e.g. ["title", "priority"]. Requires `meta`. A field missing on a hit - renders as null — rows are never dropped. + renders as null — rows are never dropped. Requesting fields also + narrows each row to the note's identity plus those values: the + projection replaces the note body rather than riding alongside it. project: Project name. Optional - qualified paths route themselves; unqualified paths refuse when several projects are addressable. project_id: Project external_id (UUID); takes precedence over `project`. @@ -867,8 +906,9 @@ async def find( Returns: Without `meta`: the directory listing as JSON (nodes, pagination, totals). With `meta`: the search response as JSON (results, pagination, - totals); each result carries a `fields` object when `fields` was - requested. + totals). Adding `fields` projects each result down to the note's + identity — title, permalink, file_path, external_id, updated_at — plus + the requested `fields` object; no note content comes back. """ # Combination rules, before any I/O. The metadata search takes no filename # glob and no depth bound, so `name` and `depth` are refused rather than @@ -1076,9 +1116,10 @@ async def entity_metadata(entity_external_id: str) -> dict[str, Any] | None: for read in reads: read.cancel() await asyncio.gather(*reads, return_exceptions=True) - # Injected post-dump so null field values survive exclude_none. - for row, metadata in zip(payload["results"], hydrated, strict=True): - row["fields"] = _project_metadata_fields(metadata, fields) + payload["results"] = [ + _projected_row(row, metadata, fields) + for row, metadata in zip(payload["results"], hydrated, strict=True) + ] return payload diff --git a/tests/cli/test_cli_posix_verbs.py b/tests/cli/test_cli_posix_verbs.py index ab15277d6..e9f425e19 100644 --- a/tests/cli/test_cli_posix_verbs.py +++ b/tests/cli/test_cli_posix_verbs.py @@ -181,15 +181,22 @@ def _dir_node(**overrides): FIND_RESULT_EMPTY = {"nodes": [], "page": 1, "page_size": 10, "total": 0, "has_more": False} # --meta flips find's payload to the search response shape (the same contract -# grep returns); --fields adds a projected `fields` object per hit, with null -# for a field the hit does not carry. +# grep returns). --fields then *projects* each hit: the row is the note's +# identity plus a `fields` object, with null for a field the hit does not carry, +# and no note body. A projected row is therefore not a grep row with an extra +# key — spelled out here rather than spread from GREP_RESULT so this mock cannot +# drift back into promising the CLI content the tool no longer sends. FIND_META_RESULT = GREP_RESULT FIND_META_FIELDS_RESULT = { **GREP_RESULT, "results": [ { - **GREP_RESULT["results"][0], + "title": "Spec [draft] v2", + "permalink": "specs/spec-draft-v2", + "file_path": "specs/Spec [draft] v2.md", + "external_id": "0b3f0d1e-5f9a-4d2b-8c31-1f0b7a9c4d55", + "updated_at": "2025-01-01T00:00:00", "fields": { "title": "Spec [draft] v2", "priority": "high", @@ -198,7 +205,11 @@ def _dir_node(**overrides): }, }, { - **GREP_RESULT["results"][1], + "title": "Another Note", + "permalink": "notes/another-note", + "file_path": "notes/Another Note.md", + "external_id": "7c2a91b4-3d68-4e0f-9a15-2b6c8e4f0a37", + "updated_at": "2025-01-02T00:00:00", "fields": { "title": "Another Note", "priority": None, diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index 0095dd87d..c0641adcb 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -6,6 +6,7 @@ """ import asyncio +import json import re from datetime import datetime, timezone from pathlib import Path @@ -1239,6 +1240,65 @@ async def test_find_meta_projects_requested_fields(client, test_project, meta_no } +@pytest.mark.asyncio +async def test_find_meta_fields_returns_identity_and_projection_only( + client, test_project, meta_notes +): + """REGRESSION: a projected row carried the whole note beside the projection. + + `fields` is the entire reason to call find instead of reading every note, + and the row it produced still carried the note's `content` — up to + SearchIndexRow.CONTENT_DISPLAY_LIMIT (4000) characters of it. The 200-row + inventory the literary-analysis skill documents therefore spent, on note + bodies nobody asked for, most of what the projection exists to save. + + The assertion is the row's key set, not its size: a projected row is the + note's identity plus the fields requested, and nothing else. + """ + projected = await find(meta=["status=active"], fields=["priority"], project=test_project.name) + + assert projected["results"], "fixture should produce hits to project" + for row in projected["results"]: + assert set(row) == { + "title", + "permalink", + "file_path", + "external_id", + "updated_at", + "fields", + } + # The identity still names the note well enough to read it next. + assert row["file_path"] and row["permalink"] and row["external_id"] + # No note body reaches the caller by any key. + serialized = json.dumps(row) + assert "alpha body" not in serialized + assert "gamma body" not in serialized + + assert {row["title"]: row["fields"] for row in projected["results"]} == { + "Alpha Spec": {"priority": "high"}, + "Gamma Note": {"priority": "critical"}, + } + + +@pytest.mark.asyncio +async def test_find_meta_without_fields_still_returns_the_full_search_shape( + client, test_project, meta_notes +): + """The narrowing belongs to projection mode only. + + Without `fields` there is no projection to stand in for the hit, so `meta` + keeps answering with the search response `bm grep` renders — content + included. Pinned here so the projection change cannot quietly strip the + unprojected arm too. + """ + unprojected = await find(meta=["status=active"], project=test_project.name) + + assert unprojected["results"] + for row in unprojected["results"]: + assert "content" in row + assert "fields" not in row + + @pytest.mark.asyncio @pytest.mark.parametrize("field", ["review..approved", ".owner", "owner."]) async def test_find_meta_malformed_field_path_refuses_without_reading_anything( From e1ff3799bc4131ef760f46201beb2365fe074ce0 Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 2 Sep 2026 00:52:36 -0500 Subject: [PATCH 11/12] fix(core): refuse metadata comparison bounds no float can hold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A comparison such as `score>` followed by 400 digits reached the server as an ordinary finite Python int — json.loads keeps an oversized integer literal as an int, so find's non-finite check, which only sees floats, passed it through. `_normalize_numeric` then called float() on it, which raises OverflowError. OverflowError is not a ValueError, and ValueError is the only thing the search router translates, so a predicate typo surfaced as a 500 instead of a predicate error. Fixed where the rule already lives rather than in find's grammar. `_normalize_numeric` owns "this value is numeric, normalize it to a float bound"; the defect is that it accepted as numeric a value it could not normalize. Refusing there covers direct `search_notes(metadata_filters={"score": {"$gt": 10**400}})` callers too, and the router's existing ValueError translation answers 400 with no second implementation of which numbers this system can compare. The pre-transport non-finite check in `_predicate_scalar` stays: `1e999` dies in the request encoder before any server sees it, which is a different failure. Stating the rule once as "the bound must be a finite float" also closes a silent sibling the finding did not name: the same magnitude spelled as a numeric string does not raise at all — float() answers it with inf — so the bound went infinite and the comparison matched every note, or none. Signed-off-by: phernandez --- docs/metadata-search.md | 1 + skills/memory-metadata-search/SKILL.md | 3 ++ src/basic_memory/man/man1/find(1).md | 4 +- .../repository/metadata_filters.py | 38 ++++++++++++++++--- tests/mcp/test_tool_posix.py | 26 +++++++++++++ tests/repository/test_metadata_filters.py | 38 +++++++++++++++++++ 6 files changed, 104 insertions(+), 6 deletions(-) diff --git a/docs/metadata-search.md b/docs/metadata-search.md index 58d699b80..9e02800f0 100644 --- a/docs/metadata-search.md +++ b/docs/metadata-search.md @@ -103,6 +103,7 @@ This queries the `version` key inside a `schema` object in frontmatter. - `$in` and array-contains require non-empty lists. - `$between` requires exactly two values `[min, max]`. - `null` is an is-null match and only valid as a plain equality value. +- Comparison and `$between` bounds must be finite numbers. A magnitude no float can hold — a 400-digit integer, which JSON keeps as an ordinary `int` — is refused as a filter error rather than compared against an infinite bound. ## MCP Tool — `search_notes` diff --git a/skills/memory-metadata-search/SKILL.md b/skills/memory-metadata-search/SKILL.md index 21c8dad83..4f9bc2e03 100644 --- a/skills/memory-metadata-search/SKILL.md +++ b/skills/memory-metadata-search/SKILL.md @@ -92,6 +92,9 @@ against the value and a comparison with null is never true. - `$in` and array-contains require non-empty lists - `$between` requires exactly `[min, max]` - `null` is an is-null match and only valid as a plain equality value +- Comparison and `$between` bounds must be finite numbers — a magnitude no float + can hold (a 400-digit integer, which JSON keeps as an ordinary `int`) is + refused rather than compared against an infinite bound - Metadata filters match Markdown notes only — indexed PDFs, images and other regular files carry no frontmatter and are never hits, not even for `null` diff --git a/src/basic_memory/man/man1/find(1).md b/src/basic_memory/man/man1/find(1).md index ffb23751d..baaa79268 100644 --- a/src/basic_memory/man/man1/find(1).md +++ b/src/basic_memory/man/man1/find(1).md @@ -106,7 +106,9 @@ confident zero. Numbers must be finite. `score=NaN`, `score=Infinity` and an overflowing exponent like `score=1e999` are refused by the grammar rather than failing later as an encoding error; quote one (`score="NaN"`) to match the literal -text. +text. A magnitude no float can hold — a 400-digit integer, which JSON keeps as +an ordinary finite `int` — travels, and the search API refuses it as the filter +error it is rather than as a server error. Keys accept dot-paths into nested frontmatter (`review.approved`), and `note_type` is accepted as a spelling of the frontmatter `type` key, matching diff --git a/src/basic_memory/repository/metadata_filters.py b/src/basic_memory/repository/metadata_filters.py index fd72ce371..9cbdda7a7 100644 --- a/src/basic_memory/repository/metadata_filters.py +++ b/src/basic_memory/repository/metadata_filters.py @@ -9,6 +9,7 @@ from dataclasses import dataclass from datetime import date, datetime +import math import re from typing import Any, Iterable, List, cast @@ -94,9 +95,36 @@ def _normalize_scalar(value: Any) -> Any: return value -def _normalize_numeric(value: object) -> float: - """Normalize a value already proven numeric by _is_numeric_value.""" - return float(cast(str | int | float, value)) +def _normalize_numeric(value: object, raw_key: str) -> float: + """Normalize a value already proven numeric by _is_numeric_value. + + A comparison bound has to be a finite float — it is what the SQL predicate + compares against, and neither an infinity nor a magnitude beyond float names + a value any indexed note can hold. + + Trigger: a number too large for a float reaches a comparison or a range + bound. `json.loads` keeps a 400-digit literal as an ordinary finite + `int`, so it passes _is_numeric_value and every check before this. + Why: the two spellings failed differently and both failed badly. `float()` + raises OverflowError on the int, and OverflowError is not a ValueError, + so the search router's translation missed it and the request became a + 500 for what is a filter typo. The same magnitude written as a string + does not raise at all — `float()` answers it with inf — which silently + made the bound infinite, matching every note or none. + Outcome: one refusal covering both, worded like this module's other filter + errors, so every surface that builds filters (find's predicates, + search_notes' metadata_filters) reports it as the bad value it is + and the router answers 400. + """ + try: + normalized = float(cast(str | int | float, value)) + except OverflowError: + normalized = math.inf + if not math.isfinite(normalized): + raise ValueError( + f"numeric metadata filter value for '{raw_key}' is not a finite number: {value}" + ) + return normalized def _refuse_null(values: Iterable[Any], raw_key: str, op: str) -> None: @@ -163,7 +191,7 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter if op in _COMPARISON_OPERATORS: _refuse_null([value], raw_key, op) if _is_numeric_value(value): - normalized = _normalize_numeric(value) + normalized = _normalize_numeric(value, raw_key) comparison = "numeric" else: normalized = _normalize_scalar(value) @@ -183,7 +211,7 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter raise ValueError(f"$between requires [min, max] for '{raw_key}'") _refuse_null(value, raw_key, op) if _is_numeric_collection(value): - normalized = [_normalize_numeric(v) for v in value] + normalized = [_normalize_numeric(v, raw_key) for v in value] comparison = "numeric" else: normalized = [_normalize_scalar(v) for v in value] diff --git a/tests/mcp/test_tool_posix.py b/tests/mcp/test_tool_posix.py index c0641adcb..7563059dc 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -1299,6 +1299,32 @@ async def test_find_meta_without_fields_still_returns_the_full_search_shape( assert "fields" not in row +@pytest.mark.asyncio +@pytest.mark.parametrize( + "predicate", + [ + f"score>{'9' * 400}", + f"score<=-{'9' * 400}", + f"score between 0,{'9' * 400}", + ], +) +async def test_find_meta_oversized_integer_refuses_as_a_filter_error( + client, test_project, meta_notes, predicate +): + """REGRESSION: a 400-digit comparison bound came back as a server error. + + The grammar's finite-number check reads what json.loads built, and + json.loads keeps an oversized *integer* literal as an ordinary finite + Python int — only the float spellings (NaN, Infinity, 1e999) are caught + there. So the int travelled, and _normalize_numeric raised OverflowError + server-side. OverflowError is not a ValueError, which is the only thing the + search router translates, so a predicate typo surfaced as a 500 rather than + a refusal naming the filter. + """ + with pytest.raises(ToolError, match="not a finite number"): + await find(meta=[predicate], project=test_project.name) + + @pytest.mark.asyncio @pytest.mark.parametrize("field", ["review..approved", ".owner", "owner."]) async def test_find_meta_malformed_field_path_refuses_without_reading_anything( diff --git a/tests/repository/test_metadata_filters.py b/tests/repository/test_metadata_filters.py index 32500deda..231f50a25 100644 --- a/tests/repository/test_metadata_filters.py +++ b/tests/repository/test_metadata_filters.py @@ -83,6 +83,44 @@ def test_null_refused_outside_equality(filters): parse_metadata_filters(filters) +_TOO_LARGE_FOR_FLOAT = 10**400 + + +@pytest.mark.parametrize( + "filters", + [ + {"score": {"$gt": _TOO_LARGE_FOR_FLOAT}}, + {"score": {"$lte": -_TOO_LARGE_FOR_FLOAT}}, + {"score": {"$between": [0, _TOO_LARGE_FOR_FLOAT]}}, + # The same magnitude spelled as a numeric string. float() does not raise + # for this one — it answers inf — so it reached the SQL bound intact. + {"score": {"$gte": "9" * 400}}, + {"score": {"$between": ["0", "9" * 400]}}, + ], +) +def test_oversized_numeric_bound_is_a_filter_error(filters): + """REGRESSION: an unrepresentable bound was a server error, or worse, silent. + + json.loads keeps a 400-digit literal as a finite Python int, so it passed + _is_numeric_value and every check before it, then reached float() — which + raises OverflowError. OverflowError is not a ValueError, and ValueError is + the only thing the search router translates, so the request became a 500 for + what is a filter typo. The string spelling of the same magnitude did not + raise at all: float() answers it with inf, making the bound infinite so the + comparison matched every note or none, silently. + """ + with pytest.raises(ValueError, match="not a finite number"): + parse_metadata_filters(filters) + + +def test_oversized_numeric_refusal_names_the_key(): + """Worded like this module's other filter errors: which key, and what is wrong.""" + with pytest.raises(ValueError) as excinfo: + parse_metadata_filters({"schema.confidence": {"$gt": _TOO_LARGE_FOR_FLOAT}}) + + assert "numeric metadata filter value for 'schema.confidence'" in str(excinfo.value) + + def test_invalid_filter_key(): with pytest.raises(ValueError): parse_metadata_filters({"bad key": "value"}) From 82b1edfedb2a7043eb3e83a38714e4263fa380d1 Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 2 Sep 2026 00:52:45 -0500 Subject: [PATCH 12/12] docs(skills): scope metadata queries by file path, not permalink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The literary-analysis skill twice taught that a positional `--meta` scope matches slugified permalinks. It does not: `_find_by_metadata` sends `file_path_prefix`, and both repositories compare `search_index.file_path` on a directory boundary. find(1) was corrected when the behavior changed during this PR; the skill was not, so an agent following it would form the wrong expectation for notes that pin an explicit `permalink:` in frontmatter, or that were moved with update_permalinks_on_move off (the default) and kept their old permalink. Both places now describe the directory-boundary match on the indexed file path, and say what that buys: `/characters` reaches everything filed under `characters/`, nested subdirectories included, and never `characters-cut/`. No example command depended on the permalink reading — the scopes in the skill (`/characters`, the coverage-check block) are directories, and the convention it teaches files notes under `characters/major/` and `characters/minor/`, which a file-path prefix reaches. Signed-off-by: phernandez --- skills/memory-literary-analysis/SKILL.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/skills/memory-literary-analysis/SKILL.md b/skills/memory-literary-analysis/SKILL.md index d36627715..c24cccd8f 100644 --- a/skills/memory-literary-analysis/SKILL.md +++ b/skills/memory-literary-analysis/SKILL.md @@ -64,9 +64,13 @@ audited: and exit 0. The value a result row displays is the value to query with. - **`find` pages, and the default page is 10.** Any query whose answer is "all N chapters" needs `--page-size 200` (the maximum) — see [Coverage Checks](#coverage-checks). -- **`--name` cannot combine with `--meta`.** The metadata search matches slugified permalinks, - not filenames. Scope a `--meta` query with the positional path instead: `find /characters - --meta 'note_type=character'`. +- **`--name` cannot combine with `--meta`.** The metadata search has no filename glob. Scope a + `--meta` query with the positional path instead: `find /characters --meta + 'note_type=character'`. That path is matched on a directory boundary against the *file path* + a note is indexed under — where the note actually lives, not its permalink, which stops + mirroring the file path once a note pins `permalink:` in frontmatter or is moved. So + `/characters` reaches everything filed under `characters/` (including `characters/major/`), + and never `characters-cut/`. If the POSIX verbs are unavailable, every step below still works with `search_notes`, `read_note`, and `list_directory` — it just costs more. @@ -568,8 +572,11 @@ bm find /characters --meta 'note_type=character' --fields role --page-size 200 `--page-size 10`, so the un-sized form of the first query "proves" a 138-chapter work has 10 chapters. 200 is the maximum page size; past that, iterate with `--page 2`, `--page 3`, … . Scope a `--meta` query with the positional path (`/characters`), never `--name` — the two -options are mutually exclusive, because metadata search matches slugified permalinks rather -than filenames. +options are mutually exclusive, because the metadata search has no filename glob. The +positional path scopes by the *file path* a note is indexed under, matched on a directory +boundary: `/characters` admits `characters/major/ahab.md` but never `characters-cut/`. It is +not a permalink match, so a note that pins its own `permalink:` is still found where its file +lives. Read the count off the footer, not off the rows you can see. Every `find` result reports `page 1 • total 138`, and appends `• more available (--page)` when the page truncated the