diff --git a/docs/metadata-search.md b/docs/metadata-search.md index 6275fbb33..9e02800f0 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. @@ -55,6 +57,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 +89,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 +102,8 @@ 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. +- 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-literary-analysis/SKILL.md b/skills/memory-literary-analysis/SKILL.md index 2627cfd3a..c24cccd8f 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,58 @@ 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 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. + ## Phase 0: Setup ### Create the Project @@ -212,6 +261,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 +321,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 +379,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 +469,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 +558,123 @@ 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 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 +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 +713,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. diff --git a/skills/memory-metadata-search/SKILL.md b/skills/memory-metadata-search/SKILL.md index f045600bc..4f9bc2e03 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,12 @@ 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 +- 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` > **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/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 8f38634d4..098d2ecfa 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,28 @@ 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', " + "'owner=null' (key missing or null). " + "PATH still scopes the query, by file path" + ), + ), + ] = 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 +768,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 /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 @@ -703,6 +784,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 +796,8 @@ def find( depth=depth, page=page, page_size=page_size, + meta=meta, + fields=field_list, project=project, project_id=project_id, ) @@ -719,6 +805,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..baaa79268 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,123 @@ 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 +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. +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. + +`--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 +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 +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`. 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. 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 +`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 `>`; +quote one that genuinely does, as in `range=">=5"`. ## 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, 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. Projects each hit down to + its identity plus those fields — no note content. Requires `--meta` +- **--page, --page-size** — pagination (defaults 1 and 10) ## EXAMPLES @@ -41,10 +143,18 @@ 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 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 ``` ## 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/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 d905a1361..aa03ea254 100644 --- a/src/basic_memory/mcp/tools/posix_tools.py +++ b/src/basic_memory/mcp/tools/posix_tools.py @@ -28,11 +28,16 @@ ``project`` param names the manual project, not a data project. """ +import asyncio +import json +import math 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 @@ -45,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 MetadataPath, parse_metadata_path from basic_memory.schemas.directory import ( DEFAULT_DIRECTORY_PAGE_SIZE, MAX_DIRECTORY_PAGE_SIZE, @@ -52,7 +58,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 +150,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 +540,297 @@ 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 "=". +# The key capture admits '.' anywhere on purpose — it is looser than a dot path. +# 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+(.+)$") +_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 _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 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 + JSON; a JSON-quoted token ('"true"') forces a literal string; anything that + 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' + ) + # 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 + + +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. + + 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] = [] + 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 = [] + 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 _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. + + 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, and on any key outside + the search API's dot-path grammar. + """ + 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) + # 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 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 '-' " + "(e.g. 'status' or 'review.approved')" + ) + 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, 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] = ( + {"$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, predicate) + if op != "=": + _refuse_null_outside_equality([value], op, predicate) + filters[key] = value if op == "=" else {_SYMBOL_OPERATORS[op]: value} + return filters + + +def _project_metadata_fields( + 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 in fields: + value: Any = entity_metadata + for part in field.parts: + if not isinstance(value, dict): + value = None + break + value = value.get(part) + projected[field.key] = value + 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="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 +845,177 @@ 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, `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. + 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. - 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` (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 + 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 + "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"'). 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. 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`. 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). 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 + # 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 + # 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 has no " + "filename glob; 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" + ) + projected_fields: list[MetadataPath] | None = None + 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") + # 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 + # 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 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=projected_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 +1029,100 @@ async def find( return listing +async def _find_by_metadata( + *, + route_project: Optional[str], + scope: str, + metadata_filters: dict[str, Any], + fields: Optional[list[MetadataPath]], + 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 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, + active_project, + ): + # Import here to avoid circular import + from basic_memory.mcp.clients import KnowledgeClient, SearchClient + + query = SearchQuery( + # 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], + ) + 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 + + # 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) + payload["results"] = [ + _projected_row(row, metadata, fields) + for row, metadata in zip(payload["results"], hydrated, strict=True) + ] + return payload + + @mcp.tool( title="Tail", description="Show recently changed notes. Requires 'project' when several are addressable.", 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..9cbdda7a7 100644 --- a/src/basic_memory/repository/metadata_filters.py +++ b/src/basic_memory/repository/metadata_filters.py @@ -1,14 +1,24 @@ -"""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 from dataclasses import dataclass from datetime import date, datetime +import math import re from typing import Any, Iterable, List, cast -_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", @@ -18,6 +28,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.""" @@ -54,9 +95,55 @@ 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: + """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]: @@ -64,6 +151,7 @@ def parse_metadata_filters(filters: dict[str, Any]) -> List[ParsedMetadataFilter 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}} @@ -74,11 +162,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 _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): @@ -94,14 +182,16 @@ 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) + normalized = _normalize_numeric(value, raw_key) comparison = "numeric" else: normalized = _normalize_scalar(value) @@ -119,8 +209,9 @@ 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] + normalized = [_normalize_numeric(v, raw_key) for v in value] comparison = "numeric" else: normalized = [_normalize_scalar(v) for v in value] @@ -134,6 +225,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 +233,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 85e6d2e70..eb8a3f4e3 100644 --- a/src/basic_memory/repository/postgres_search_repository.py +++ b/src/basic_memory/repository/postgres_search_repository.py @@ -26,6 +26,9 @@ from basic_memory.repository.search_repository_base import ( SearchRepositoryBase, VectorChunkState, + file_path_prefix_condition, + metadata_contains_like_condition, + metadata_filter_content_type_condition, ) from basic_memory.repository.search_trace import ( SearchTraceCollector, @@ -968,6 +971,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 +1134,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 = [] @@ -1184,6 +1195,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): @@ -1197,6 +1212,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 @@ -1219,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 @@ -1341,6 +1367,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 +1389,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 +1416,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 +1563,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 +1580,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 +1601,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..daa561778 100644 --- a/src/basic_memory/repository/search_repository_base.py +++ b/src/basic_memory/repository/search_repository_base.py @@ -76,8 +76,13 @@ 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, 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 +155,108 @@ 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" + + +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" + + +# 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, @@ -308,6 +415,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 +436,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 +460,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 +2013,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 +2049,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 +2071,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 +2250,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 +2438,7 @@ def _log_vector_summary() -> None: search_item_types, categories, metadata_filters, + file_path_prefix, ] ) @@ -2338,6 +2453,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 +2517,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 +2590,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 +2629,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 +2648,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 +2799,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..8f5983066 100644 --- a/src/basic_memory/repository/sqlite_search_repository.py +++ b/src/basic_memory/repository/sqlite_search_repository.py @@ -28,7 +28,12 @@ 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, + metadata_contains_like_condition, + metadata_filter_content_type_condition, +) from basic_memory.repository.script_ngrams import analyze_script_query from basic_memory.repository.search_trace import ( SearchTraceCollector, @@ -782,6 +787,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 +874,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 = [] @@ -922,6 +935,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): @@ -941,6 +958,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 @@ -961,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 @@ -973,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)) @@ -1055,6 +1084,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 +1112,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 +1139,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 +1271,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 +1288,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 +1309,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..749d93243 100644 --- a/src/basic_memory/schemas/search.py +++ b/src/basic_memory/schemas/search.py @@ -30,6 +30,35 @@ class SearchRetrievalMode(str, Enum): HYBRID = "hybrid" +def normalize_file_path_prefix(value: Optional[str]) -> Optional[str]: + """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 + scope = value.removeprefix("./") + if not scope.strip().strip("/"): + return None + return scope.strip("/") + + class SearchQuery(BaseModel): """Search query parameters. @@ -45,6 +74,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 +97,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 +122,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 +151,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/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/cli/test_cli_posix_verbs.py b/tests/cli/test_cli_posix_verbs.py index e209ce55b..e9f425e19 100644 --- a/tests/cli/test_cli_posix_verbs.py +++ b/tests/cli/test_cli_posix_verbs.py @@ -180,6 +180,50 @@ 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 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": [ + { + "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", + "approved": True, + "missing": None, + }, + }, + { + "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, + "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 +750,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..7563059dc 100644 --- a/tests/mcp/test_tool_posix.py +++ b/tests/mcp/test_tool_posix.py @@ -5,19 +5,33 @@ assert on the JSON shapes the canonical `output_format="json"` paths produce. """ +import asyncio +import json +import re +from datetime import datetime, timezone from pathlib import Path +from textwrap import dedent from types import SimpleNamespace import pytest +import pytest_asyncio import yaml 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, tail, write_note +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 ( + MetadataPath, + ParsedMetadataFilter, + parse_metadata_filters, + parse_metadata_path, +) from basic_memory.schemas.search import SearchRetrievalMode @@ -506,6 +520,1133 @@ 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"]}}), + # 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 + # 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"]}), + # 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. + ("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"), + # 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'"), + ], +) +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( + "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( + "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_one_path_grammar_governs_predicates_and_filters(key): + """find's predicates and the search API accept exactly the same paths. + + 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. + """ + well_formed = parse_metadata_path(key) is not None + + if well_formed: + 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( + ("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", + [ + # 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 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 --- + + +@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_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. + + 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", + 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) + + 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"}), + # Only Alpha Spec carries review.approved, so null is its complement. + (["review.approved=null"], {"Beta Spec", "Gamma Note"}), + (["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 +async def test_find_meta_null_finds_the_notes_carrying_no_value(client, test_project, meta_notes): + """REGRESSION: 'key=null' answers "which notes have no value here?". + + It used to compile to `= NULL`, which no row satisfies, so find reported an + exact total of zero however many notes were missing the field — a wrong + answer wearing the same confident `total_is_exact` as a right one. Asserting + the complementary query in the same test keeps the null side honest: a + filter that matched nothing would pass a bare exclusion check. + """ + absent = await find(meta=["review.approved=null"], project=test_project.name) + present = await find(meta=["review.approved=true"], 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"} + + +@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): + """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"]} == {"Alpha Spec", "Gamma Note"} + assert result["total_is_exact"] is True + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("path", "expected_titles"), + [ + ("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()), + ], +) +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) + + assert {row["title"] for row in result["results"]} == expected_titles + assert result["total"] == len(expected_titles) + assert result["total_is_exact"] is True + + +@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_scope_follows_the_file_path_not_the_permalink( + client, test_project, diverged_permalink_notes +): + """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("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 + # 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") + + # 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 +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 +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( + "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( + 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"), + [ + # 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'"), + # 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"), + ({"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. + ({"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_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. + + 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_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_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) + + +@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 +1880,69 @@ 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. + + 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", + 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"}, + ) + + await write_note( + title="Second Root Note", + directory="specs", + content="# Second Root Note", + project="second-project", + metadata={"status": "active"}, + ) + + root = await find("second-project", meta=["status=active"]) + scoped = 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 +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 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_metadata_filters.py b/tests/repository/test_metadata_filters.py index a2112fb20..231f50a25 100644 --- a/tests/repository/test_metadata_filters.py +++ b/tests/repository/test_metadata_filters.py @@ -54,6 +54,73 @@ def test_parse_normalizes_scalar_types(): assert values["ratio"] == "0.5" +def test_parse_null_equality_is_an_is_null_clause(): + """A None value is not equality: `= NULL` is never true in SQL. + + The op is distinct so both dialects have to make a deliberate choice about + it — an is-null filter that fell through to the equality branch would report + a confident zero for every note in the project. + """ + parsed = parse_metadata_filters({"owner": None}) + assert parsed == [ParsedMetadataFilter(["owner"], "is_null", None)] + + +@pytest.mark.parametrize( + "filters", + [ + {"score": {"$gt": None}}, + {"score": {"$lte": None}}, + {"priority": {"$in": ["high", None]}}, + {"score": {"$between": [None, 0.8]}}, + {"tags": ["security", None]}, + ], +) +def test_null_refused_outside_equality(filters): + """Every operator but equality compares against its value, and a comparison + with NULL is never true — so a null bound is refused rather than answering + zero rows for a query it cannot express.""" + with pytest.raises(ValueError, match="null is not supported by"): + 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"}) diff --git a/tests/repository/test_metadata_filters_edge_cases.py b/tests/repository/test_metadata_filters_edge_cases.py index e6e8e884b..59560400a 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, @@ -70,6 +78,153 @@ async def test_filter_missing_metadata_field(search_repository, session_maker): assert len(results) == 0 +@pytest.mark.asyncio +async def test_null_filter_matches_a_missing_key_and_an_explicit_null( + search_repository, session_maker +): + """CONTRACT: {"key": None} is IS NULL, and both dialects draw the same line. + + SQLite's json_extract and Postgres's jsonb_extract_path_text each collapse a + missing key and an explicit JSON null to SQL NULL, so "which notes have no + owner?" answers row for row on either backend. The regression this pins is + the equality clause it replaced: `= NULL` satisfies no row, so the query + reported an exact zero no matter how many notes were missing the field. + """ + missing = await _index_entity_with_metadata( + search_repository, + session_maker, + "Owner Key Absent", + {"status": "active"}, + ) + explicit = await _index_entity_with_metadata( + search_repository, + session_maker, + "Owner Explicitly Null", + {"status": "active", "owner": None}, + ) + + results = await search_repository.search(metadata_filters={"owner": None}) + + assert {r.id for r in results} == {missing.id, explicit.id} + 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. + + The positive control matters: a filter that matched nothing at all would + also "exclude" this note, which is exactly the bug being fixed. + """ + alice = await _index_entity_with_metadata( + search_repository, + session_maker, + "Owner Alice", + {"owner": "alice"}, + ) + unowned = await _index_entity_with_metadata( + search_repository, + session_maker, + "Owner Unset", + {"status": "active"}, + ) + + null_matches = await search_repository.search(metadata_filters={"owner": None}) + alice_matches = await search_repository.search(metadata_filters={"owner": "alice"}) + + assert {r.id for r in null_matches} == {unowned.id} + assert {r.id for r in alice_matches} == {alice.id} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("field", "present_value"), + [("status", "active"), ("type", "spec"), ("tags", ["security"])], +) +async def test_null_filter_on_the_indexed_frontmatter_fields( + search_repository, session_maker, field, present_value +): + """The three fields SQLite answers from generated columns behave identically. + + status, type and tags take `entity.frontmatter_status` / `frontmatter_type` + / `tags_json` on SQLite instead of a json_extract call, while Postgres has + no such columns and always extracts from the JSONB. Those columns are + defined AS that json_extract, so IS NULL means the same thing on both sides + — this is the one place the dialects build structurally different SQL for + the same filter, so it gets its own contract test. + """ + missing = await _index_entity_with_metadata( + search_repository, + session_maker, + f"Indexed {field} absent", + {"unrelated": "x"}, + ) + await _index_entity_with_metadata( + search_repository, + session_maker, + f"Indexed {field} present", + {field: present_value}, + ) + + results = await search_repository.search(metadata_filters={field: None}) + + assert {r.id for r in results} == {missing.id} + + +@pytest.mark.asyncio +async def test_null_filter_walks_a_nested_path(search_repository, session_maker): + """A dot-path null match asks the same question one level down.""" + unreviewed = await _index_entity_with_metadata( + search_repository, + session_maker, + "Nested Review Missing", + {"review": {"reviewer": "alice"}}, + ) + await _index_entity_with_metadata( + search_repository, + session_maker, + "Nested Review Approved", + {"review": {"reviewer": "alice", "approved": True}}, + ) + + results = await search_repository.search(metadata_filters={"review.approved": None}) + + assert {r.id for r in results} == {unreviewed.id} + + @pytest.mark.asyncio async def test_filter_multiple_conditions_and_logic(search_repository, session_maker): """Multiple metadata_filters are combined with AND logic.""" @@ -118,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.""" 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..885cd96ba --- /dev/null +++ b/tests/repository/test_search_file_path_prefix.py @@ -0,0 +1,383 @@ +"""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, 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"), + "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"), + " specs /iota.md": ("Iota", "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", "./specs/"]) +async def test_scope_spellings_normalize_to_one_query(search_repository, seeded_paths, scope): + """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 +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( + 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), + (" / ", None), + # "./" is the relative spelling of the root, as it is for `ls`. + ("./", None), + ("specs", "specs"), + ("/specs/", "specs"), + ("/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_keeps_the_path_and_drops_only_the_notation(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)."""