diff --git a/CLAUDE.md b/CLAUDE.md index c180e44..05c6c48 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,8 +84,9 @@ package. `paths.py` (confinement), `markdown.py` (scanning: links, headings, tags, atomic write, revs), `frontmatter.py` (writing properties without rewriting the block), `index.py` (lazy link/tag/property index, invalidated on mtime, never - rebuilt per call), `search.py`, `periodic.py` (daily notes from the vault's own - settings), `writes.py` (the write half, as a mixin), `provider.py`. + rebuilt per call), `search.py`, `excerpts.py` (the note opening a listing shows + when there is no match to quote), `periodic.py` (daily notes from the vault's + own settings), `writes.py` (the write half, as a mixin), `provider.py`. - **The index holds hidden notes and the lookups exclude them.** Indexing with `include_hidden=False` meant `include_hidden=True` on a search had nothing to find, because the notes were never there. So the walk takes everything, and @@ -209,7 +210,7 @@ make test-all | `__main__.py` | CLI entry: argparse, transport selection | | `config.py` | `KnapConfig` dataclass + env loading | | `providers/protocol.py` | `VaultProvider` protocol + value objects | -| `providers/filesystem/` | The backend: paths, markdown, frontmatter, index, search, periodic, writes | +| `providers/filesystem/` | The backend: paths, markdown, frontmatter, index, search, excerpts, periodic, writes | | `providers/factory.py` | Backend selection from config | | `tools/handler.py` | `VaultToolHandler` (mixins) + `register_tools` | | `tools/vault/` | Tools as mixins: browse, query, read, write, organize, graph, periodic, attachments | diff --git a/knap_mcp/providers/filesystem/excerpts.py b/knap_mcp/providers/filesystem/excerpts.py new file mode 100644 index 0000000..733a7e9 --- /dev/null +++ b/knap_mcp/providers/filesystem/excerpts.py @@ -0,0 +1,79 @@ +"""The opening of a note: what a listing shows when there is no match to quote. + +A search hit can quote the text around the query. A listing has no query, so the +only honest thing to show is how the note starts, and both `protocol.NoteSummary` +and `schemas.NoteSummaryResult` promise exactly that. + +It sits in its own module because the two callers that need it are on opposite +sides of the backend and neither owns it: `index.py` derives it while it has the +note parsed anyway, and `search.py` hands it back for a filter-only query. The +obvious third home, `markdown.py` next to the other `*_of(note)` scanners, is 18 +lines from the 500-line budget, and this is not the change that should spend them. +""" + +from __future__ import annotations + +import re +from typing import List + +from . import markdown as md + +#: How much of the opening to keep. Long enough to tell two notes apart, short +#: enough that a page of fifty of them is not a wall of text. +OPENING_CHARS = 160 + +#: Lines of prose to draw the opening from. More than a couple, because the first +#: line of a note is often a single short sentence; few enough that the cost is a +#: slice rather than a scan of a long note. Blank lines do not count against it: +#: markdown is written double-spaced, so counting raw lines would have meant two +#: lines of prose on most notes and four on the ones that happen to be dense. +OPENING_LINES = 4 + +# An ATX heading, mirroring the form `markdown._HEADING_RE` accepts: hashes then +# whitespace then text. Deliberately not `startswith("#")`, which would read a +# note opening on the inline tag `#meeting` as a heading and skip the line. +_HEADING_LINE_RE = re.compile(r"^#{1,6}([ \t]|$)") + + +def opening_of(note: md.ParsedNote, *, chars: int = OPENING_CHARS) -> str: + """The lead-in of a note, flattened to one line. + + The frontmatter is already off ``body``. Headings above the prose go too, + because "# Meeting notes" on a note titled "Meeting notes" tells a client + nothing that `title` did not already tell it, and a summary whose excerpt + repeats its own title is the same as having no excerpt. The first heading + *after* the prose ends it instead: that is where the lead-in stops and the + note's sections begin, and "## Log" is structure, not a description. + + Code is not content, the rule `markdown.py` states and every scanner beside + this one follows, so the code-blanked copy decides which lines are prose and + the real body supplies their text. The two share their line structure by + construction, the same trick `title_of` uses to quote a heading it matched + on the copy. Without it a note that opens on a fenced example was described + to the client as "```". + + Whitespace is collapsed for the same reason it is collapsed around a search + match: this lands in a listing as one line, and a note that opens with an + indented list or a table should not arrive full of gaps. + """ + prose: List[str] = [] + # Not strict: the two are the same length by construction, and if that ever + # stops being true the scanners that resolve links are the place to hear + # about it. A listing should not raise over the shape of a summary field. + for line, scannable in zip( + note.body.split("\n"), note.body_scannable.split("\n"), strict=False + ): + if not scannable.strip(): + continue # blank, or a line that was nothing but code + if _HEADING_LINE_RE.match(scannable): + if prose: + break + continue + prose.append(line) + if len(prose) >= OPENING_LINES: + break + opening = re.sub(r"\s+", " ", " ".join(prose)).strip() + return opening[:chars] + ("..." if len(opening) > chars else "") + + +__all__ = ["OPENING_CHARS", "OPENING_LINES", "opening_of"] diff --git a/knap_mcp/providers/filesystem/index.py b/knap_mcp/providers/filesystem/index.py index eff420e..118afb4 100644 --- a/knap_mcp/providers/filesystem/index.py +++ b/knap_mcp/providers/filesystem/index.py @@ -23,13 +23,20 @@ from pathlib import Path from typing import Any, Dict, Iterable, List, Optional, Set, Tuple +from . import excerpts from . import markdown as md from . import paths as vault_paths @dataclass class NoteEntry: - """One note as the index knows it. No body: that is read on demand.""" + """One note as the index knows it. No body: that is read on demand. + + ``excerpt`` is not a body and not a step towards holding one. It is a couple + of lines, derived at parse time from a body that was in memory anyway, and + dropped the moment the note's mtime moves -- the same terms the headings and + the properties beside it are held on. + """ path: str rev: str @@ -42,6 +49,8 @@ class NoteEntry: properties: Dict[str, Any] = field(default_factory=dict) #: Link targets exactly as written in the note, in document order. link_targets: List[str] = field(default_factory=list) + #: The note's opening prose, for a listing that has no match to quote. + excerpt: str = "" @property def basename(self) -> str: @@ -177,6 +186,11 @@ def _parse_note(self, rel: str) -> Optional[NoteEntry]: headings=md.headings_of(note), properties=dict(note.frontmatter), link_targets=[link.target for link in md.raw_links_of(note)], + # Derived here rather than when a listing asks for it, because here + # the body is already in hand. Read on demand it would cost one file + # open per note listed, and `backlinks` is not paginated: the MOC that + # the whole vault links to would open the whole vault. + excerpt=excerpts.opening_of(note), ) def _rebuild_lookups(self) -> None: diff --git a/knap_mcp/providers/filesystem/provider.py b/knap_mcp/providers/filesystem/provider.py index 0f600a6..3cba038 100644 --- a/knap_mcp/providers/filesystem/provider.py +++ b/knap_mcp/providers/filesystem/provider.py @@ -336,6 +336,14 @@ def _compose(self, frontmatter_raw: str, body: str) -> str: return f"{frontmatter_raw}\n{body}" def _summary(self, entry: NoteEntry, *, excerpt: str = "") -> NoteSummary: + """One summary shape for a listing, a search hit and a backlink. + + ``excerpt`` is the text around a search match. Without one -- a listing, + a backlink list, a filter-only search -- the note's opening stands in, + which is what the protocol and the tool schema both promise. Defaulting + it to "" here was the whole of #6: three call sites, two of them silently + answering with nothing. + """ return NoteSummary( path=entry.path, title=entry.title, @@ -343,7 +351,7 @@ def _summary(self, entry: NoteEntry, *, excerpt: str = "") -> NoteSummary: size=entry.size, modified=_iso(entry.mtime_ns), tags=list(entry.tags), - excerpt=excerpt, + excerpt=excerpt or entry.excerpt, ) def _link_refs(self, rel: str, note: md.ParsedNote) -> List[LinkRef]: diff --git a/knap_mcp/providers/filesystem/search.py b/knap_mcp/providers/filesystem/search.py index 1af7dbb..9dfb315 100644 --- a/knap_mcp/providers/filesystem/search.py +++ b/knap_mcp/providers/filesystem/search.py @@ -92,10 +92,11 @@ def query( candidates.sort(key=lambda item: item.mtime_ns, reverse=True) if not needle: + # No query, so nothing to quote: the opening stands in, and it comes + # off the index rather than out of the file. This used to open every + # note on the page to read text the index had already parsed. page = candidates[offset : offset + limit] - return [Hit(entry=entry, excerpt=_opening(self.root, entry)) for entry in page], len( - candidates - ) + return [Hit(entry=entry, excerpt=entry.excerpt) for entry in page], len(candidates) hits: List[Hit] = [] scanned = 0 @@ -146,27 +147,6 @@ def _match(self, entry: NoteEntry, needle: str) -> Optional[str]: return None return _window(note.body, position, len(needle)) - def _opening(self, entry: NoteEntry) -> str: - return _opening(self.root, entry) - - -def _opening(root: Path, entry: NoteEntry, chars: int = 160) -> str: - """The first prose of a note, for a listing with no search term. - - Skips the frontmatter and the leading H1, because "# Meeting notes" under a - note called "Meeting notes" tells a client nothing it does not have. - """ - try: - text, _, _, _ = md.read_text(root / entry.path) - except OSError: - return "" - body = md.parse(text).body.strip() - lines = [line for line in body.split("\n")] - while lines and (not lines[0].strip() or lines[0].lstrip().startswith("#")): - lines.pop(0) - opening = " ".join(line.strip() for line in lines[:4]).strip() - return opening[:chars] + ("..." if len(opening) > chars else "") - def _window(body: str, position: int, length: int) -> str: start = max(0, position - EXCERPT_RADIUS) diff --git a/tests/test_provider.py b/tests/test_provider.py index 8a17970..e4b1846 100644 --- a/tests/test_provider.py +++ b/tests/test_provider.py @@ -11,6 +11,7 @@ import pytest +from knap_mcp.providers.filesystem import excerpts from knap_mcp.providers.filesystem.provider import FilesystemVaultProvider from knap_mcp.providers.protocol import ProviderError @@ -266,6 +267,127 @@ def test_tags_are_most_used_first(self, provider) -> None: assert counts == sorted(counts, reverse=True) +class TestExcerpts: + """#6: the schema promises the opening for a listing, and got "" instead. + + `search` passed its match window through and the other two call sites did + not, so `vault_list_notes` and `vault_backlinks` answered with an empty + string on every note. A client that cannot see what a note is about from a + listing has to open all of them, which is the cost the excerpt exists to + avoid. + """ + + @staticmethod + def _excerpt_for(provider, path: str) -> str: + notes, _ = provider.list_notes(limit=100) + return next(note.excerpt for note in notes if note.path == path) + + def test_a_listing_carries_the_opening(self, provider) -> None: + excerpt = self._excerpt_for(provider, "Areas/Work/Acme.md") + assert excerpt.startswith("The renewal is due.") + + def test_every_note_in_a_listing_is_offered_one(self, provider) -> None: + """The bug was not partial: nothing in a listing had an excerpt.""" + notes, _ = provider.list_notes(limit=100) + with_prose = [note for note in notes if note.path != "Templates/Daily.md"] + assert with_prose + assert all(note.excerpt for note in with_prose) + + def test_backlinks_carry_it_too(self, provider) -> None: + """The second call site in the issue, and the one that cannot be paged.""" + backlinks = provider.backlinks("Areas/Work/Acme.md") + by_path = {note.path: note.excerpt for note in backlinks} + assert by_path["Areas/Work/Meetings/index.md"] == "See [[Acme Corp]]." + + def test_the_leading_heading_is_not_the_excerpt(self, provider) -> None: + """An excerpt that repeats the title is the same as not having one.""" + excerpt = self._excerpt_for(provider, "Projects/Meeting notes.md") + assert "# Meeting notes" not in excerpt + assert excerpt.startswith("Spoke to [[Acme Corp]]") + + def test_a_section_heading_ends_the_opening_rather_than_joining_it(self, provider) -> None: + """A heading like "## Log" describes the note's shape, not the note. + + Skipping only the *leading* heading left every interior one in the text, + so the listing #6 asked for would have answered with the lead-in plus a + stray "## Log" on any note organised into sections, which is most of them. + """ + excerpt = self._excerpt_for(provider, "Areas/Work/Acme.md") + assert excerpt == "The renewal is due. See [[Meeting notes]] and [[Something unwritten]]." + + def test_a_fenced_block_is_not_the_opening(self, provider) -> None: + """`markdown.py` says code is not content, and this is a scanner too. + + A note that opens on an example was described to the client as "```" + followed by the contents of the fence. + """ + assert "```" not in self._excerpt_for(provider, "Projects/Meeting notes.md") + provider.write( + "Fenced.md", "# Fenced\n\n```\nx = 1\n```\n\nWhat it is for.\n", mode="create" + ) + assert self._excerpt_for(provider, "Fenced.md") == "What it is for." + + def test_inline_code_keeps_its_text(self, provider) -> None: + """Blanking code decides which lines are prose; it must not eat them. + + The line is chosen on the code-blanked copy and read off the real body, + so "Run `npm install` first" arrives whole rather than as "Run first". + """ + provider.write("Inline.md", "Run `npm install` first.\n", mode="create") + assert self._excerpt_for(provider, "Inline.md") == "Run `npm install` first." + + def test_an_opening_tag_is_not_mistaken_for_a_heading(self, provider) -> None: + """`#meeting` starts with a hash and is a tag. A heading needs the space.""" + provider.write("Tagged.md", "#meeting was useful today\n", mode="create") + assert self._excerpt_for(provider, "Tagged.md") == "#meeting was useful today" + + def test_blank_lines_do_not_count_against_the_line_budget(self, provider) -> None: + """Markdown is written double-spaced, so raw lines would have halved it.""" + body = "\n\n".join(["One.", "Two.", "Three.", "Four.", "Five."]) + provider.write("Spaced.md", body, mode="create") + excerpt = self._excerpt_for(provider, "Spaced.md") + assert excerpt == "One. Two. Three. Four." + assert len(excerpt.split()) == excerpts.OPENING_LINES + + def test_a_note_of_only_headings_gets_nothing_rather_than_its_title(self, provider) -> None: + """`Templates/Daily.md` is headings and blanks. Empty is the honest answer.""" + assert self._excerpt_for(provider, "Templates/Daily.md") == "" + + def test_a_long_opening_is_capped_and_says_so(self, provider) -> None: + provider.write("Long.md", "word " * 200, mode="create") + excerpt = self._excerpt_for(provider, "Long.md") + assert excerpt.endswith("...") + assert len(excerpt) == excerpts.OPENING_CHARS + 3 + + def test_a_search_still_quotes_the_match_not_the_opening(self, provider) -> None: + """The one call site that worked has to keep working. + + "Kickoff" sits under "## Log", past where the lead-in stops, so a hit + that mentions it can only have come from the match window. + """ + notes, _ = provider.search("Kickoff") + assert [note.path for note in notes] == ["Areas/Work/Acme.md"] + assert "Kickoff" in notes[0].excerpt + assert "Kickoff" not in self._excerpt_for(provider, "Areas/Work/Acme.md") + + def test_a_filter_only_search_falls_back_to_the_opening(self, provider) -> None: + """No query means no match to quote, so the opening stands in.""" + notes, _ = provider.search(tag="client") + assert [note.path for note in notes] == ["Areas/Work/Acme.md"] + assert notes[0].excerpt.startswith("The renewal is due.") + + def test_an_edit_in_obsidian_changes_the_excerpt(self, provider, obsidian_edits) -> None: + """The opening is held in the index, so it has to fall out on an edit. + + A cached excerpt that survives the note it was cut from is worse than no + excerpt: it reads as current and is not. + """ + assert self._excerpt_for(provider, "Templates/Daily.md") == "" + obsidian_edits(provider.root / "Templates" / "Daily.md", "\nCaptured the standup.\n") + provider.index.refresh(force=True) + assert self._excerpt_for(provider, "Templates/Daily.md") == "Captured the standup." + + class TestIndexFreshness: def test_a_note_written_by_obsidian_is_picked_up(self, provider) -> None: """The index cannot be built once and trusted: Obsidian edits under us."""