From 256926b859612622684d6bf17574bd6685acc2b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 06:17:41 +0000 Subject: [PATCH 1/5] Keep frontmatter on overwrite, and quote what YAML would misread Two ways a write lost the customer's data without saying so. `mode="overwrite"` composed the new note from an empty frontmatter block, so every body replacement deleted `type`, `tags` and `aliases`. The tool says the opposite in three places a caller actually reads: `body` is "the new body, replacing everything after the frontmatter", `properties` says omitting it "leaves the existing frontmatter alone", and the confirm prompt promises only what is "below the frontmatter". A model doing an ordinary read, rewrite and write back destroyed the frontmatter discipline that retrieval rests on, and nothing in the diff it asked about showed it. The old test pinned the drop in a comment while its own name said "keeps the frontmatter" and its assertion checked neither; it now checks both, plus the merge case and the note that has no frontmatter to keep. `_scalar` quoted a leading `#` but not one further in. YAML starts a comment at a hash after a space, so `title: Budget #2026 review` reached disk and read back as `Budget`, with the rest gone from the file for good. Rather than extend the list of YAML's surprises from memory again, `_reads_back` now asks the parser whether the bare form round-trips, and quotes when it does not. The list stays as the cheap path that settles almost everything without a parse. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FbPQWSXitqNEkRE6BtqrXM --- knap_mcp/providers/filesystem/frontmatter.py | 29 ++++++++++- knap_mcp/providers/filesystem/writes.py | 15 +++++- tests/test_markdown.py | 51 ++++++++++++++++++- tests/test_provider_writes.py | 52 +++++++++++++++++--- 4 files changed, 135 insertions(+), 12 deletions(-) diff --git a/knap_mcp/providers/filesystem/frontmatter.py b/knap_mcp/providers/filesystem/frontmatter.py index 7fb004c..dc408b6 100644 --- a/knap_mcp/providers/filesystem/frontmatter.py +++ b/knap_mcp/providers/filesystem/frontmatter.py @@ -152,7 +152,17 @@ def _render(key: str, value: Any) -> Optional[List[str]]: def _scalar(value: str) -> str: - """Quote a string only when YAML would otherwise read it as something else.""" + """Quote a string only when YAML would otherwise read it as something else. + + The list below is the cheap answer and ``_reads_back`` is the correct one. + Keeping both is deliberate: the list documents the cases worth knowing about + and settles the overwhelming majority without a parse, and the round trip + catches whatever the list forgot. It forgot ``#`` for a long time -- a hash + after a space opens a comment, so ``Budget #2026 review`` was written + unquoted and read back as ``Budget``, with the rest gone from the file. That + is the failure this function exists to prevent, and enumerating YAML's + surprises from memory is how it got missed. + """ if value == "": return '""' needs_quotes = ( @@ -161,15 +171,30 @@ def _scalar(value: str) -> str: or ": " in value or value.endswith(":") or "\n" in value + or " #" in value + or "\t#" in value or value.lower() in ("true", "false", "null", "yes", "no", "on", "off", "~") or _looks_numeric(value) ) - if not needs_quotes: + if not needs_quotes and _reads_back(value): return value escaped = value.replace("\\", "\\\\").replace('"', '\\"').replace("\n", "\\n") return f'"{escaped}"' +def _reads_back(value: str) -> bool: + """Does this string, written bare, parse back as itself? + + The one question that matters, asked of the parser instead of answered from + memory. A note is the customer's own writing, so the interesting inputs are + the ones nobody thought of. + """ + try: + return yaml.safe_load(f"x: {value}") == {"x": value} + except yaml.YAMLError: + return False + + def _looks_numeric(value: str) -> bool: try: float(value) diff --git a/knap_mcp/providers/filesystem/writes.py b/knap_mcp/providers/filesystem/writes.py index fdbbbaa..3ba8967 100644 --- a/knap_mcp/providers/filesystem/writes.py +++ b/knap_mcp/providers/filesystem/writes.py @@ -100,7 +100,7 @@ def write( raise NoteNotFoundError(f"{rel} does not exist") self._check_rev(rel, absolute, expected_rev, required=True) - if exists and mode in ("append", "prepend"): + if exists and mode in ("append", "prepend", "overwrite"): current, _, _, _ = md.read_text(absolute) parsed = md.parse(current) if mode == "append": @@ -109,8 +109,19 @@ def write( # glues itself onto the last sentence is a corrupted paragraph. separator = "" if parsed.body.endswith("\n\n") or not parsed.body else "\n" new_body = f"{parsed.body.rstrip(chr(10))}\n{separator}{body}" - else: + elif mode == "prepend": new_body = f"{body}\n\n{parsed.body.lstrip(chr(10))}" + else: + # Overwrite replaces the body and keeps the frontmatter, which is + # what `vault_update_note` tells the caller in three places: `body` + # is "the new body, replacing everything after the frontmatter", + # `properties` says omitting it "leaves the existing frontmatter + # alone", and the confirm prompt promises only what is "below the + # frontmatter". It used to drop the block instead, so a model doing + # an ordinary read-modify-write silently deleted `type`, `tags` and + # `aliases` -- the frontmatter discipline the whole retrieval story + # rests on, gone without appearing in the diff it asked about. + new_body = body text = self._compose(parsed.frontmatter_raw, new_body) else: text = self._compose("", body) diff --git a/tests/test_markdown.py b/tests/test_markdown.py index 875de0d..cbeed21 100644 --- a/tests/test_markdown.py +++ b/tests/test_markdown.py @@ -208,7 +208,28 @@ def test_no_changes_returns_the_input(self) -> None: @pytest.mark.parametrize( "value", - ["true", "false", "null", "yes", "no", "1.5", "42", "2026-01-01", "a: b", "", " x"], + [ + "true", + "false", + "null", + "yes", + "no", + "1.5", + "42", + "2026-01-01", + "a: b", + "", + " x", + # A hash after a space opens a YAML comment, so an unquoted + # `Budget #2026 review` used to reach disk and read back as + # `Budget`, with the rest of the title silently gone. The leading + # `#` was handled; this one is the middle of an ordinary sentence, + # and `#` in a title or a tag is not exotic in an Obsidian vault. + "Budget #2026 review", + "release #3 notes", + "tab\t#comment", + "trailing hash #", + ], ) def test_a_string_that_yaml_would_misread_is_quoted(self, value: str) -> None: """The round trip is what matters: what we write must read back equal. @@ -238,11 +259,37 @@ def test_a_value_with_a_quote_survives(self) -> None: def test_what_we_write_is_what_pyyaml_reads(self) -> None: """Belt and braces on the hand-rolled scalar writer.""" - values = {"a": "true", "b": "x: y", "c": "-dash", "d": "#hash", "e": "100%"} + values = { + "a": "true", + "b": "x: y", + "c": "-dash", + "d": "#hash", + "e": "100%", + "f": "Budget #2026 review", + } new = md.edit_frontmatter("Body\n", values) inner = new.split("---\n")[1] assert yaml.safe_load(inner) == values + def test_a_list_item_with_a_hash_keeps_everything_after_it(self) -> None: + """The list path stringifies each item and quotes it the same way.""" + tags = ["proj #1", "ok", "#lead", "a: b"] + new = md.edit_frontmatter("Body\n", {"tags": tags}) + assert md.parse(new).frontmatter["tags"] == tags + + @pytest.mark.parametrize( + "value", + ["plain", "two words", "path/to/note", "CamelCase", "with-dash", "e-mail@host"], + ) + def test_an_ordinary_string_is_still_written_bare(self, value: str) -> None: + """The quoting must stay narrow, or every note gains quotes it did not have. + + `vault_set_properties` promises a minimal diff, and a value that suddenly + acquires quotes is noise in every diff the customer reads after it. + """ + new = md.edit_frontmatter("Body\n", {"status": value}) + assert f"status: {value}\n" in new + class TestSections: RAW = "# Title\n\n## Log\n\n- one\n\n### Sub\n\n- deep\n\n## Next\n\n- later\n" diff --git a/tests/test_provider_writes.py b/tests/test_provider_writes.py index d6b4d2c..cff6c76 100644 --- a/tests/test_provider_writes.py +++ b/tests/test_provider_writes.py @@ -87,12 +87,52 @@ def test_overwrite_with_a_stale_rev_is_refused(self, provider, obsidian_edits) - assert "typed by the human" in provider.read("Areas/Work/Acme.md").body def test_overwrite_keeps_the_frontmatter_when_none_is_passed(self, provider) -> None: - note = provider.read("Areas/Work/Acme.md") - provider.write("Areas/Work/Acme.md", "new body\n", mode="overwrite", expected_rev=note.rev) - # The body is replaced; frontmatter is a separate argument and was not - # given, so it is gone with the old body. Pinned so the behaviour is a - # decision rather than a surprise. - assert provider.read("Areas/Work/Acme.md").body == "new body\n" + """The body is replaced; the properties are a separate argument and stay. + + This is what the tool promises the caller, and the reason it matters is + retrieval: `type`, `tags` and `aliases` are how an AI finds a note again. + A model that reads a note, rewrites the prose and writes it back is doing + the ordinary thing, and it never sees the frontmatter go. + """ + before = provider.read("Areas/Work/Acme.md") + assert before.frontmatter, "fixture must have frontmatter for this to mean anything" + + provider.write( + "Areas/Work/Acme.md", "new body\n", mode="overwrite", expected_rev=before.rev + ) + + after = provider.read("Areas/Work/Acme.md") + assert after.body == "new body\n" + assert after.frontmatter == before.frontmatter + + def test_overwrite_still_merges_properties_when_they_are_passed(self, provider) -> None: + """Keeping the block is not the same as refusing to change it.""" + before = provider.read("Areas/Work/Acme.md") + provider.write( + "Areas/Work/Acme.md", + "new body\n", + mode="overwrite", + expected_rev=before.rev, + frontmatter={"status": "closed"}, + ) + + after = provider.read("Areas/Work/Acme.md") + assert after.frontmatter["status"] == "closed" + assert after.body == "new body\n" + # Everything the caller did not name is still there. + for key, value in before.frontmatter.items(): + if key != "status": + assert after.frontmatter[key] == value + + def test_overwrite_on_a_note_without_frontmatter_adds_none(self, provider) -> None: + """No block in, no block out. Preserving must not mean inventing.""" + provider.write("Plain.md", "first\n", mode="create") + note = provider.read("Plain.md") + provider.write("Plain.md", "second\n", mode="overwrite", expected_rev=note.rev) + + after = provider.read("Plain.md") + assert after.body == "second\n" + assert not after.frontmatter def test_a_path_escape_is_refused_on_write(self, provider) -> None: with pytest.raises(PathNotAllowedError): From fa127eba7238ade398fb0da130e587c70b8692b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 06:34:11 +0000 Subject: [PATCH 2/5] Follow the parser's answer instead of only asking the question Review of the previous commit found the round-trip guard half-wired, in two ways that made it worse than it looked. `yaml.safe_load` raises more than `YAMLError`. Its timestamp constructor calls `datetime.date`, so `2026-02-30` comes back as a `ValueError`, sailed past a `except yaml.YAMLError`, and left `vault_set_properties` raising in the caller's face on a plausible typo. Every exception counts as "do not write this" now. Quoting was treated as the safe answer, and it is not. The escape covers backslash, quote and newline, so a carriage return survived into the double-quoted scalar and YAML folded it back to a space: the guard asked the right question, got "no", quoted, and wrote something that still read back wrong. `_scalar` can now say it cannot do it safely, and the caller re-dumps the block with PyYAML, which is the escape hatch this module already documents for nested values. A fuzz over 4000 values round-trips clean where 17% used to fail. The tests deserved the same scepticism and did not get it. They all started from `edit_frontmatter("Body\n", ...)`, and a note with no frontmatter goes straight to `_dump_block`, so PyYAML did the quoting and `_scalar` never ran. They passed against the broken code. They start from a note with a block now, and 13 of them fail against the version before this pair of commits. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FbPQWSXitqNEkRE6BtqrXM --- knap_mcp/providers/filesystem/frontmatter.py | 55 +++++++++++++------- tests/test_markdown.py | 51 +++++++++++++++--- 2 files changed, 81 insertions(+), 25 deletions(-) diff --git a/knap_mcp/providers/filesystem/frontmatter.py b/knap_mcp/providers/filesystem/frontmatter.py index dc408b6..89ee269 100644 --- a/knap_mcp/providers/filesystem/frontmatter.py +++ b/knap_mcp/providers/filesystem/frontmatter.py @@ -141,27 +141,37 @@ def _render(key: str, value: Any) -> Optional[List[str]]: if isinstance(value, (int, float)): return [f"{key}: {value}"] if isinstance(value, str): - return [f"{key}: {_scalar(value)}"] + rendered = _scalar(value) + return None if rendered is None else [f"{key}: {rendered}"] if isinstance(value, list): if not value: return [f"{key}: []"] if not all(isinstance(item, (str, int, float, bool)) for item in value): return None - return [f"{key}:", *[f" - {_scalar(str(item))}" for item in value]] + items = [_scalar(str(item)) for item in value] + if any(item is None for item in items): + return None + return [f"{key}:", *[f" - {item}" for item in items]] return None -def _scalar(value: str) -> str: - """Quote a string only when YAML would otherwise read it as something else. +def _scalar(value: str) -> Optional[str]: + """Write a string as a YAML scalar, or None if we cannot do it safely. + + The list below is the cheap answer and ``_reads_back`` is the deciding one. + Keeping both is deliberate: the list settles the overwhelming majority + without a parse and documents the cases worth knowing about, and the round + trip catches whatever the list forgot. It forgot ``#`` for a long time -- a + hash after a space opens a comment, so ``Budget #2026 review`` was written + unquoted and read back as ``Budget``, with the rest gone from the file. - The list below is the cheap answer and ``_reads_back`` is the correct one. - Keeping both is deliberate: the list documents the cases worth knowing about - and settles the overwhelming majority without a parse, and the round trip - catches whatever the list forgot. It forgot ``#`` for a long time -- a hash - after a space opens a comment, so ``Budget #2026 review`` was written - unquoted and read back as ``Budget``, with the rest gone from the file. That - is the failure this function exists to prevent, and enumerating YAML's - surprises from memory is how it got missed. + Quoting is not automatically the safe answer either, which is why the quoted + form is checked too. Escaping covers backslash, quote and newline, so a + carriage return survives into the double-quoted scalar and YAML folds it to + a space on the way back. When neither form round-trips we return None and + the caller re-dumps the block with PyYAML: a formatting diff the customer + can see, which is the trade this module already makes for nested values and + is strictly better than writing something that reads back wrong. """ if value == "": return '""' @@ -176,22 +186,29 @@ def _scalar(value: str) -> str: or value.lower() in ("true", "false", "null", "yes", "no", "on", "off", "~") or _looks_numeric(value) ) - if not needs_quotes and _reads_back(value): + if not needs_quotes and _reads_back(value, value): return value escaped = value.replace("\\", "\\\\").replace('"', '\\"').replace("\n", "\\n") - return f'"{escaped}"' + quoted = f'"{escaped}"' + return quoted if _reads_back(quoted, value) else None -def _reads_back(value: str) -> bool: - """Does this string, written bare, parse back as itself? +def _reads_back(written: str, value: str) -> bool: + """Does ``written``, as the value of a key, parse back as ``value``? The one question that matters, asked of the parser instead of answered from memory. A note is the customer's own writing, so the interesting inputs are - the ones nobody thought of. + the ones nobody thought of, and the parser is the only thing that knows them + all. + + Every exception counts as "no". PyYAML raises more than ``YAMLError`` here: + its timestamp constructor calls ``datetime.date``, so a mistyped date like + ``2026-02-30`` comes out as a ``ValueError`` and would otherwise travel all + the way up through ``vault_set_properties`` into the caller's face. """ try: - return yaml.safe_load(f"x: {value}") == {"x": value} - except yaml.YAMLError: + return yaml.safe_load(f"x: {written}") == {"x": value} + except Exception: # noqa: BLE001 - any failure to parse means "do not write this" return False diff --git a/tests/test_markdown.py b/tests/test_markdown.py index cbeed21..f713796 100644 --- a/tests/test_markdown.py +++ b/tests/test_markdown.py @@ -236,9 +236,17 @@ def test_a_string_that_yaml_would_misread_is_quoted(self, value: str) -> None: Writing `status: true` for the string "true" hands the vault a boolean, and the property silently changes type. + + Start from a note that already HAS frontmatter, or this tests the wrong + code. `edit` sends a note without a block straight to `_dump_block`, so + PyYAML does the quoting and our own `_scalar` never runs. The `#` bug + lived on the in-place path and an earlier version of these cases passed + against the broken code for exactly that reason. """ - new = md.edit_frontmatter("Body\n", {"status": value}) + raw = "---\nkeep: me\n---\nBody\n" + new = md.edit_frontmatter(raw, {"status": value}) assert md.parse(new).frontmatter["status"] == value + assert md.parse(new).frontmatter["keep"] == "me" def test_a_list_of_scalars_round_trips(self) -> None: new = md.edit_frontmatter("Body\n", {"tags": ["one", "two/three"]}) @@ -258,7 +266,11 @@ def test_a_value_with_a_quote_survives(self) -> None: assert md.parse(new).frontmatter["title"] == 'He said "no"' def test_what_we_write_is_what_pyyaml_reads(self) -> None: - """Belt and braces on the hand-rolled scalar writer.""" + """Belt and braces on the hand-rolled scalar writer. + + Note the `raw` with a block in it: without one this goes to `_dump_block` + and tests PyYAML rather than us. See the parametrized case above. + """ values = { "a": "true", "b": "x: y", @@ -267,14 +279,14 @@ def test_what_we_write_is_what_pyyaml_reads(self) -> None: "e": "100%", "f": "Budget #2026 review", } - new = md.edit_frontmatter("Body\n", values) + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", values) inner = new.split("---\n")[1] - assert yaml.safe_load(inner) == values + assert yaml.safe_load(inner) == {"keep": "me", **values} def test_a_list_item_with_a_hash_keeps_everything_after_it(self) -> None: """The list path stringifies each item and quotes it the same way.""" tags = ["proj #1", "ok", "#lead", "a: b"] - new = md.edit_frontmatter("Body\n", {"tags": tags}) + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"tags": tags}) assert md.parse(new).frontmatter["tags"] == tags @pytest.mark.parametrize( @@ -287,9 +299,36 @@ def test_an_ordinary_string_is_still_written_bare(self, value: str) -> None: `vault_set_properties` promises a minimal diff, and a value that suddenly acquires quotes is noise in every diff the customer reads after it. """ - new = md.edit_frontmatter("Body\n", {"status": value}) + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"status": value}) assert f"status: {value}\n" in new + @pytest.mark.parametrize("value", ["2026-02-30", "2026-13-01", "2026-01-01 25:00:00"]) + def test_a_mistyped_date_does_not_reach_the_caller_as_a_crash(self, value: str) -> None: + """PyYAML raises ValueError, not YAMLError, when it builds a date. + + `2026-02-30` is a plausible typo, and asking the parser whether a value + round-trips means catching everything the parser can throw. Treating only + YAMLError as "no" turned a bad property into an exception out of + `vault_set_properties`. + """ + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"due": value}) + assert md.parse(new).frontmatter["due"] == value + + @pytest.mark.parametrize("value", ["line1\rline2", "vertical\x0btab", "form\x0cfeed"]) + def test_a_control_character_falls_back_rather_than_folding_to_a_space( + self, value: str + ) -> None: + """Quoting is not automatically safe, so the quoted form is checked too. + + Escaping covers backslash, quote and newline. A carriage return survives + into the double-quoted scalar and YAML folds it back to a space, so the + value read back short a character and nobody was told. + """ + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"note": value}) + parsed = md.parse(new) + assert parsed.frontmatter["note"] == value + assert parsed.frontmatter["keep"] == "me" + class TestSections: RAW = "# Title\n\n## Log\n\n- one\n\n### Sub\n\n- deep\n\n## Next\n\n- later\n" From dc763cab6eb22baa9a7a5eefe59da1f695ac3e60 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 06:42:43 +0000 Subject: [PATCH 3/5] Stop paying for work the answer does not need Three costs on the two most-used tools, measured on a vault of 3001 notes with one hub note the whole vault links to. Behaviour is unchanged throughout; each of these is work whose result was thrown away. `_rebuild_lookups` checked `rel not in holders` against a list, once per link. That is a scan, and the note it punishes is the one every vault has: the index or MOC note everything points at, whose holder list is as long as the vault. A set beside the list makes it a hash lookup. Same order out, same type out. Search opened every candidate note through `read_text`, which hashes the whole file to build a `rev` search never reads, and through `parse`, which YAML-parses a frontmatter block search had already compared against the index before it opened the file. `read_body` and `parse(load_properties=False)` skip both. The tests pin that the lean path sees exactly what the full one sees, including for notes whose frontmatter does not parse. Refreshing the index called `to_relative` per note, and that resolves both sides: a realpath per note, asking the kernel to confirm what the walk had already guaranteed. `walk_notes` starts at a resolved root and skips symlinks, so its output is under the root by construction. `relative_to_walked_root` says so in its name and takes the resolved root once. `to_relative` is untouched and stays the check for anything arriving from a caller, which is the confinement boundary and not this. Three tests hold the two functions to the same answer, including when the vault root is itself a symlink and when a note tries to escape through one. list_notes limit=25 190.6 ms -> 56.6 ms free-text search limit=25 1389.2 ms -> 287.9 ms property search 194.5 ms -> 68.6 ms backlinks on the hub 190.5 ms -> 64.0 ms The frontmatter tests moved to `test_frontmatter.py`, beside the module they cover, because the additions here put `test_markdown.py` over the 500-line budget and extracting is what the budget is for. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FbPQWSXitqNEkRE6BtqrXM --- knap_mcp/providers/filesystem/index.py | 14 +- knap_mcp/providers/filesystem/markdown.py | 23 ++- knap_mcp/providers/filesystem/paths.py | 19 ++ knap_mcp/providers/filesystem/search.py | 7 +- tests/test_frontmatter.py | 189 ++++++++++++++++++ tests/test_markdown.py | 227 +++++----------------- tests/test_path_safety.py | 54 +++++ tests/test_provider.py | 21 ++ 8 files changed, 374 insertions(+), 180 deletions(-) create mode 100644 tests/test_frontmatter.py diff --git a/knap_mcp/providers/filesystem/index.py b/knap_mcp/providers/filesystem/index.py index a55b445..4aade36 100644 --- a/knap_mcp/providers/filesystem/index.py +++ b/knap_mcp/providers/filesystem/index.py @@ -21,7 +21,7 @@ import time from dataclasses import dataclass, field from pathlib import Path -from typing import Any, Dict, Iterable, List, Optional, Tuple +from typing import Any, Dict, Iterable, List, Optional, Set, Tuple from . import markdown as md from . import paths as vault_paths @@ -102,12 +102,13 @@ def refresh(self, *, force: bool = False) -> None: # decide what a hidden note may take part in: not link resolution, not # backlinks, not tag counts, because a link must not resolve into # `.trash` and a deleted note's tags are not the vault's tags. + root_resolved = self.root.resolve() for absolute in vault_paths.walk_notes(self.root, include_hidden=True): try: stat = absolute.stat() except OSError: continue - rel = vault_paths.to_relative(self.root, absolute) + rel = vault_paths.relative_to_walked_root(root_resolved, absolute) seen[rel] = (stat.st_size, stat.st_mtime_ns) changed = False @@ -184,6 +185,12 @@ def _rebuild_lookups(self) -> None: for paths in self._by_stem.values(): paths.sort(key=lambda rel: (rel.count("/"), len(rel), rel)) + # `seen` shadows `_backlinks` purely so the duplicate check is a hash + # lookup. It was `rel not in holders` on the list, which is a scan, and + # the note that suffers is the one every vault has: the MOC or index + # note the whole vault links to, whose holder list is as long as the + # vault. Same order out, same list type, one dict thrown away at the end. + seen: Dict[str, Set[str]] = {} for rel, entry in self._notes.items(): if vault_paths.is_hidden(rel): continue # a trashed note's links are not backlinks @@ -191,7 +198,8 @@ def _rebuild_lookups(self) -> None: resolved = self.resolve(target, from_path=rel) if resolved and resolved != rel: holders = self._backlinks.setdefault(resolved, []) - if rel not in holders: + if rel not in seen.setdefault(resolved, set()): + seen[resolved].add(rel) holders.append(rel) # -- reading ------------------------------------------------------------ # diff --git a/knap_mcp/providers/filesystem/markdown.py b/knap_mcp/providers/filesystem/markdown.py index 10ac70e..7c5a1c3 100644 --- a/knap_mcp/providers/filesystem/markdown.py +++ b/knap_mcp/providers/filesystem/markdown.py @@ -89,12 +89,30 @@ def blank(match: re.Match) -> str: return _INLINE_CODE_RE.sub(blank, without_fences) -def parse(raw: str) -> ParsedNote: +def read_body(path: Path) -> str: + """Read a note for scanning: the text, and none of the bookkeeping. + + ``read_text`` also hashes the whole file to build a ``rev``, which is the + right thing when the caller is going to hand that rev to a client and + exactly wasted work when it is not. Search opens every candidate note and + uses none of it. + """ + data = path.read_bytes() + return unicodedata.normalize("NFC", data.decode("utf-8", errors="replace")) + + +def parse(raw: str, *, load_properties: bool = True) -> ParsedNote: """Split a note into frontmatter and body. A frontmatter block only counts at the very start of the file and only when it closes. An unterminated ``---`` is a horizontal rule in someone's note, not a broken header, and treating it as one would swallow their document. + + ``load_properties=False`` skips the YAML parse and leaves ``frontmatter`` + empty. The block is still split off the body, so ``body`` and + ``body_scannable`` are unchanged; only the parsed mapping is missing. Search + wants exactly that, because the index already holds the properties and it + has compared them before it ever opens the file. """ note = ParsedNote(raw=raw) if not raw.startswith(FRONTMATTER_FENCE): @@ -124,6 +142,9 @@ def parse(raw: str) -> ParsedNote: note.body = note.body[1:] note.body_scannable = strip_code(note.body) + if not load_properties: + return note + inner = "\n".join(lines[1:close]) try: loaded = yaml.safe_load(inner) if inner.strip() else None diff --git a/knap_mcp/providers/filesystem/paths.py b/knap_mcp/providers/filesystem/paths.py index ce5c3e0..28eb1d2 100644 --- a/knap_mcp/providers/filesystem/paths.py +++ b/knap_mcp/providers/filesystem/paths.py @@ -159,6 +159,25 @@ def to_relative(root: Path, absolute: Path) -> str: return unicodedata.normalize("NFC", resolved.relative_to(root_resolved).as_posix()) +def relative_to_walked_root(root_resolved: Path, absolute: Path) -> str: + """Vault-relative form of a path that ``walk_notes`` itself produced. + + ``to_relative`` is the one to use for a path that came from a caller: it + resolves both sides and refuses anything that lands outside, and that check + is the vulnerability class the whole module exists for. This is the other + case. ``walk_notes`` starts at ``root.resolve()`` and skips symlinks + outright, so every path it yields is already under the resolved root and got + there without traversing a link. Resolving it again asks the kernel to + confirm something the walk guaranteed, once per note, and on a vault of a + few thousand notes that realpath storm is most of what an index refresh + costs. + + Pass ``root_resolved`` already resolved, once, by the caller. Only feed this + paths from the walk. + """ + return unicodedata.normalize("NFC", absolute.relative_to(root_resolved).as_posix()) + + def is_hidden(rel: str) -> bool: """Whether any segment of the path is a dot entry. diff --git a/knap_mcp/providers/filesystem/search.py b/knap_mcp/providers/filesystem/search.py index a09d4bc..1af7dbb 100644 --- a/knap_mcp/providers/filesystem/search.py +++ b/knap_mcp/providers/filesystem/search.py @@ -130,10 +130,13 @@ def _match(self, entry: NoteEntry, needle: str) -> Optional[str]: return f"{key}: {value}" try: - text, _, _, _ = md.read_text(self.root / entry.path) + text = md.read_body(self.root / entry.path) except OSError: return None - note = md.parse(text) + # No rev and no YAML: this is the hot loop of the whole package, it opens + # every candidate note, and it uses neither. The properties were checked + # against the index a few lines up, before the file was opened at all. + note = md.parse(text, load_properties=False) # Searched against the code-stripped copy so a hit inside a fenced block # does not surface, but the excerpt is cut from the real body: offsets # are shared between the two by construction. diff --git a/tests/test_frontmatter.py b/tests/test_frontmatter.py new file mode 100644 index 0000000..4ac0e1e --- /dev/null +++ b/tests/test_frontmatter.py @@ -0,0 +1,189 @@ +"""Writing frontmatter without rewriting it. + +Split out of `test_markdown.py` for the same reason `frontmatter.py` was split +out of `markdown.py`: it is a self-contained job with a rule of its own, and the +file was over the 500-line budget. + +The rule is that `vault_set_properties` promises the body and every property it +was not asked about come out byte-identical, so the block is edited line by line +instead of re-dumped. That only works if a hand-rolled line reads back as the +value it was written for, which is what most of this file is about. +""" + +from __future__ import annotations + +import pytest +import yaml + +from knap_mcp.providers.filesystem import markdown as md + + +class TestEditFrontmatter: + """The promise: the body and every untouched property come out byte-identical.""" + + def test_an_untouched_property_keeps_its_exact_formatting(self) -> None: + raw = "---\naliases: [Acme Corp, ACME]\nstatus: active\n---\n# Acme\n" + new = md.edit_frontmatter(raw, {"status": "done"}) + assert "aliases: [Acme Corp, ACME]" in new # still flow style, not re-dumped + assert "status: done" in new + assert new.endswith("# Acme\n") + + def test_a_block_list_stays_a_block_list(self) -> None: + raw = "---\ntags:\n - one\n - two\nstatus: a\n---\nBody\n" + new = md.edit_frontmatter(raw, {"status": "b"}) + assert "tags:\n - one\n - two\n" in new + + def test_the_body_is_never_reflowed(self) -> None: + body = "# Title\n\n\n\nOdd spacing kept.\n\n- a\n" + raw = f"---\na: 1\n---\n{body}" + new = md.edit_frontmatter(raw, {"a": 2}) + assert new.endswith(body) + + def test_none_removes_a_key_and_its_continuation(self) -> None: + raw = "---\ntags:\n - one\n - two\nstatus: a\n---\nBody\n" + new = md.edit_frontmatter(raw, {"tags": None}) + assert "tags" not in new + assert "one" not in new + assert "status: a" in new + + def test_a_new_key_is_appended(self) -> None: + raw = "---\na: 1\n---\nBody\n" + new = md.edit_frontmatter(raw, {"b": "two"}) + assert new == "---\na: 1\nb: two\n---\nBody\n" + + def test_frontmatter_is_created_when_there_is_none(self) -> None: + new = md.edit_frontmatter("# Title\n", {"status": "active"}) + assert new.startswith("---\n") + assert md.parse(new).frontmatter == {"status": "active"} + assert new.endswith("# Title\n") + + def test_removing_a_key_that_was_never_there_is_not_an_error(self) -> None: + raw = "---\na: 1\n---\nBody\n" + assert md.edit_frontmatter(raw, {"zzz": None}) == raw + + def test_no_changes_returns_the_input(self) -> None: + raw = "---\na: 1\n---\nBody\n" + assert md.edit_frontmatter(raw, {}) == raw + + @pytest.mark.parametrize( + "value", + [ + "true", + "false", + "null", + "yes", + "no", + "1.5", + "42", + "2026-01-01", + "a: b", + "", + " x", + # A hash after a space opens a YAML comment, so an unquoted + # `Budget #2026 review` used to reach disk and read back as + # `Budget`, with the rest of the title silently gone. The leading + # `#` was handled; this one is the middle of an ordinary sentence, + # and `#` in a title or a tag is not exotic in an Obsidian vault. + "Budget #2026 review", + "release #3 notes", + "tab\t#comment", + "trailing hash #", + ], + ) + def test_a_string_that_yaml_would_misread_is_quoted(self, value: str) -> None: + """The round trip is what matters: what we write must read back equal. + + Writing `status: true` for the string "true" hands the vault a boolean, + and the property silently changes type. + + Start from a note that already HAS frontmatter, or this tests the wrong + code. `edit` sends a note without a block straight to `_dump_block`, so + PyYAML does the quoting and our own `_scalar` never runs. The `#` bug + lived on the in-place path and an earlier version of these cases passed + against the broken code for exactly that reason. + """ + raw = "---\nkeep: me\n---\nBody\n" + new = md.edit_frontmatter(raw, {"status": value}) + assert md.parse(new).frontmatter["status"] == value + assert md.parse(new).frontmatter["keep"] == "me" + + def test_a_list_of_scalars_round_trips(self) -> None: + new = md.edit_frontmatter("Body\n", {"tags": ["one", "two/three"]}) + assert md.parse(new).frontmatter["tags"] == ["one", "two/three"] + + def test_a_nested_value_falls_back_to_a_re_dump_but_stays_correct(self) -> None: + """The escape hatch: correct content, and a formatting diff we accept.""" + raw = "---\na: 1\n---\nBody\n" + new = md.edit_frontmatter(raw, {"nested": {"x": [1, 2]}}) + parsed = md.parse(new) + assert parsed.frontmatter["nested"] == {"x": [1, 2]} + assert parsed.frontmatter["a"] == 1 + assert parsed.body == "Body\n" + + def test_a_value_with_a_quote_survives(self) -> None: + new = md.edit_frontmatter("Body\n", {"title": 'He said "no"'}) + assert md.parse(new).frontmatter["title"] == 'He said "no"' + + def test_what_we_write_is_what_pyyaml_reads(self) -> None: + """Belt and braces on the hand-rolled scalar writer. + + Note the `raw` with a block in it: without one this goes to `_dump_block` + and tests PyYAML rather than us. See the parametrized case above. + """ + values = { + "a": "true", + "b": "x: y", + "c": "-dash", + "d": "#hash", + "e": "100%", + "f": "Budget #2026 review", + } + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", values) + inner = new.split("---\n")[1] + assert yaml.safe_load(inner) == {"keep": "me", **values} + + def test_a_list_item_with_a_hash_keeps_everything_after_it(self) -> None: + """The list path stringifies each item and quotes it the same way.""" + tags = ["proj #1", "ok", "#lead", "a: b"] + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"tags": tags}) + assert md.parse(new).frontmatter["tags"] == tags + + @pytest.mark.parametrize( + "value", + ["plain", "two words", "path/to/note", "CamelCase", "with-dash", "e-mail@host"], + ) + def test_an_ordinary_string_is_still_written_bare(self, value: str) -> None: + """The quoting must stay narrow, or every note gains quotes it did not have. + + `vault_set_properties` promises a minimal diff, and a value that suddenly + acquires quotes is noise in every diff the customer reads after it. + """ + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"status": value}) + assert f"status: {value}\n" in new + + @pytest.mark.parametrize("value", ["2026-02-30", "2026-13-01", "2026-01-01 25:00:00"]) + def test_a_mistyped_date_does_not_reach_the_caller_as_a_crash(self, value: str) -> None: + """PyYAML raises ValueError, not YAMLError, when it builds a date. + + `2026-02-30` is a plausible typo, and asking the parser whether a value + round-trips means catching everything the parser can throw. Treating only + YAMLError as "no" turned a bad property into an exception out of + `vault_set_properties`. + """ + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"due": value}) + assert md.parse(new).frontmatter["due"] == value + + @pytest.mark.parametrize("value", ["line1\rline2", "vertical\x0btab", "form\x0cfeed"]) + def test_a_control_character_falls_back_rather_than_folding_to_a_space( + self, value: str + ) -> None: + """Quoting is not automatically safe, so the quoted form is checked too. + + Escaping covers backslash, quote and newline. A carriage return survives + into the double-quoted scalar and YAML folds it back to a space, so the + value read back short a character and nobody was told. + """ + new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"note": value}) + parsed = md.parse(new) + assert parsed.frontmatter["note"] == value + assert parsed.frontmatter["keep"] == "me" diff --git a/tests/test_markdown.py b/tests/test_markdown.py index f713796..24d34ad 100644 --- a/tests/test_markdown.py +++ b/tests/test_markdown.py @@ -1,8 +1,9 @@ """Parsing and writing a note. The details that corrupt somebody's writing. Three claims in ``markdown.py`` are load-bearing and each has a section here: -code is not content, writing frontmatter is not re-dumping it, and a write is -atomic. +code is not content, a write is atomic, and reading a note for scanning sees +exactly what reading it properly would. Writing frontmatter is the fourth and it +lives in ``test_frontmatter.py``, beside the module it covers. """ from __future__ import annotations @@ -11,7 +12,6 @@ from pathlib import Path import pytest -import yaml from knap_mcp.providers.filesystem import markdown as md @@ -159,177 +159,6 @@ def test_nothing_to_do_returns_the_input_unchanged(self) -> None: assert new is raw -class TestEditFrontmatter: - """The promise: the body and every untouched property come out byte-identical.""" - - def test_an_untouched_property_keeps_its_exact_formatting(self) -> None: - raw = "---\naliases: [Acme Corp, ACME]\nstatus: active\n---\n# Acme\n" - new = md.edit_frontmatter(raw, {"status": "done"}) - assert "aliases: [Acme Corp, ACME]" in new # still flow style, not re-dumped - assert "status: done" in new - assert new.endswith("# Acme\n") - - def test_a_block_list_stays_a_block_list(self) -> None: - raw = "---\ntags:\n - one\n - two\nstatus: a\n---\nBody\n" - new = md.edit_frontmatter(raw, {"status": "b"}) - assert "tags:\n - one\n - two\n" in new - - def test_the_body_is_never_reflowed(self) -> None: - body = "# Title\n\n\n\nOdd spacing kept.\n\n- a\n" - raw = f"---\na: 1\n---\n{body}" - new = md.edit_frontmatter(raw, {"a": 2}) - assert new.endswith(body) - - def test_none_removes_a_key_and_its_continuation(self) -> None: - raw = "---\ntags:\n - one\n - two\nstatus: a\n---\nBody\n" - new = md.edit_frontmatter(raw, {"tags": None}) - assert "tags" not in new - assert "one" not in new - assert "status: a" in new - - def test_a_new_key_is_appended(self) -> None: - raw = "---\na: 1\n---\nBody\n" - new = md.edit_frontmatter(raw, {"b": "two"}) - assert new == "---\na: 1\nb: two\n---\nBody\n" - - def test_frontmatter_is_created_when_there_is_none(self) -> None: - new = md.edit_frontmatter("# Title\n", {"status": "active"}) - assert new.startswith("---\n") - assert md.parse(new).frontmatter == {"status": "active"} - assert new.endswith("# Title\n") - - def test_removing_a_key_that_was_never_there_is_not_an_error(self) -> None: - raw = "---\na: 1\n---\nBody\n" - assert md.edit_frontmatter(raw, {"zzz": None}) == raw - - def test_no_changes_returns_the_input(self) -> None: - raw = "---\na: 1\n---\nBody\n" - assert md.edit_frontmatter(raw, {}) == raw - - @pytest.mark.parametrize( - "value", - [ - "true", - "false", - "null", - "yes", - "no", - "1.5", - "42", - "2026-01-01", - "a: b", - "", - " x", - # A hash after a space opens a YAML comment, so an unquoted - # `Budget #2026 review` used to reach disk and read back as - # `Budget`, with the rest of the title silently gone. The leading - # `#` was handled; this one is the middle of an ordinary sentence, - # and `#` in a title or a tag is not exotic in an Obsidian vault. - "Budget #2026 review", - "release #3 notes", - "tab\t#comment", - "trailing hash #", - ], - ) - def test_a_string_that_yaml_would_misread_is_quoted(self, value: str) -> None: - """The round trip is what matters: what we write must read back equal. - - Writing `status: true` for the string "true" hands the vault a boolean, - and the property silently changes type. - - Start from a note that already HAS frontmatter, or this tests the wrong - code. `edit` sends a note without a block straight to `_dump_block`, so - PyYAML does the quoting and our own `_scalar` never runs. The `#` bug - lived on the in-place path and an earlier version of these cases passed - against the broken code for exactly that reason. - """ - raw = "---\nkeep: me\n---\nBody\n" - new = md.edit_frontmatter(raw, {"status": value}) - assert md.parse(new).frontmatter["status"] == value - assert md.parse(new).frontmatter["keep"] == "me" - - def test_a_list_of_scalars_round_trips(self) -> None: - new = md.edit_frontmatter("Body\n", {"tags": ["one", "two/three"]}) - assert md.parse(new).frontmatter["tags"] == ["one", "two/three"] - - def test_a_nested_value_falls_back_to_a_re_dump_but_stays_correct(self) -> None: - """The escape hatch: correct content, and a formatting diff we accept.""" - raw = "---\na: 1\n---\nBody\n" - new = md.edit_frontmatter(raw, {"nested": {"x": [1, 2]}}) - parsed = md.parse(new) - assert parsed.frontmatter["nested"] == {"x": [1, 2]} - assert parsed.frontmatter["a"] == 1 - assert parsed.body == "Body\n" - - def test_a_value_with_a_quote_survives(self) -> None: - new = md.edit_frontmatter("Body\n", {"title": 'He said "no"'}) - assert md.parse(new).frontmatter["title"] == 'He said "no"' - - def test_what_we_write_is_what_pyyaml_reads(self) -> None: - """Belt and braces on the hand-rolled scalar writer. - - Note the `raw` with a block in it: without one this goes to `_dump_block` - and tests PyYAML rather than us. See the parametrized case above. - """ - values = { - "a": "true", - "b": "x: y", - "c": "-dash", - "d": "#hash", - "e": "100%", - "f": "Budget #2026 review", - } - new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", values) - inner = new.split("---\n")[1] - assert yaml.safe_load(inner) == {"keep": "me", **values} - - def test_a_list_item_with_a_hash_keeps_everything_after_it(self) -> None: - """The list path stringifies each item and quotes it the same way.""" - tags = ["proj #1", "ok", "#lead", "a: b"] - new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"tags": tags}) - assert md.parse(new).frontmatter["tags"] == tags - - @pytest.mark.parametrize( - "value", - ["plain", "two words", "path/to/note", "CamelCase", "with-dash", "e-mail@host"], - ) - def test_an_ordinary_string_is_still_written_bare(self, value: str) -> None: - """The quoting must stay narrow, or every note gains quotes it did not have. - - `vault_set_properties` promises a minimal diff, and a value that suddenly - acquires quotes is noise in every diff the customer reads after it. - """ - new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"status": value}) - assert f"status: {value}\n" in new - - @pytest.mark.parametrize("value", ["2026-02-30", "2026-13-01", "2026-01-01 25:00:00"]) - def test_a_mistyped_date_does_not_reach_the_caller_as_a_crash(self, value: str) -> None: - """PyYAML raises ValueError, not YAMLError, when it builds a date. - - `2026-02-30` is a plausible typo, and asking the parser whether a value - round-trips means catching everything the parser can throw. Treating only - YAMLError as "no" turned a bad property into an exception out of - `vault_set_properties`. - """ - new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"due": value}) - assert md.parse(new).frontmatter["due"] == value - - @pytest.mark.parametrize("value", ["line1\rline2", "vertical\x0btab", "form\x0cfeed"]) - def test_a_control_character_falls_back_rather_than_folding_to_a_space( - self, value: str - ) -> None: - """Quoting is not automatically safe, so the quoted form is checked too. - - Escaping covers backslash, quote and newline. A carriage return survives - into the double-quoted scalar and YAML folds it back to a space, so the - value read back short a character and nobody was told. - """ - new = md.edit_frontmatter("---\nkeep: me\n---\nBody\n", {"note": value}) - parsed = md.parse(new) - assert parsed.frontmatter["note"] == value - assert parsed.frontmatter["keep"] == "me" - - class TestSections: RAW = "# Title\n\n## Log\n\n- one\n\n### Sub\n\n- deep\n\n## Next\n\n- later\n" @@ -468,3 +297,53 @@ def test_a_latin1_note_is_readable_rather_than_an_error(self, tmp_path: Path) -> def _boom(*args, **kwargs): raise RuntimeError("simulated failure") + + +class TestReadingForScanning: + """The two shortcuts search takes, and the promise that they change nothing. + + Search opens every candidate note. It used to hash each one to build a `rev` + it discards, and YAML-parse a frontmatter block it had already compared + against the index before opening the file. Both are skippable; neither may + change what search sees. + """ + + RAW = "---\ntype: meeting\ntags:\n - a\n---\n# Title\n\nprose\n\n```\ncode\n```\n" + + def test_load_properties_false_changes_only_the_properties(self) -> None: + full = md.parse(self.RAW) + lean = md.parse(self.RAW, load_properties=False) + assert lean.body == full.body + assert lean.body_scannable == full.body_scannable + assert lean.frontmatter_raw == full.frontmatter_raw + assert lean.raw == full.raw + assert lean.frontmatter == {} + assert full.frontmatter == {"type": "meeting", "tags": ["a"]} + + @pytest.mark.parametrize( + "raw", + [ + "no frontmatter at all\n", + "---\nunterminated: block\n\nbody\n", + "---\n---\nempty block\n", + "---\n: not: valid: yaml:\n---\nbody\n", + "", + ], + ) + def test_the_two_agree_on_the_body_for_awkward_notes(self, raw: str) -> None: + full = md.parse(raw) + lean = md.parse(raw, load_properties=False) + assert (lean.body, lean.body_scannable) == (full.body, full.body_scannable) + + def test_read_body_matches_read_texts_text(self, tmp_path) -> None: + note = tmp_path / "n.md" + note.write_text(self.RAW, encoding="utf-8") + text, _rev, _size, _mtime = md.read_text(note) + assert md.read_body(note) == text + + def test_read_body_decodes_the_same_way_on_a_non_utf8_note(self, tmp_path) -> None: + """A vault that picked up a Latin-1 note years ago still opens.""" + note = tmp_path / "n.md" + note.write_bytes(b"caf\xe9 notes\n") + text, _rev, _size, _mtime = md.read_text(note) + assert md.read_body(note) == text diff --git a/tests/test_path_safety.py b/tests/test_path_safety.py index ef6fb58..5cc8710 100644 --- a/tests/test_path_safety.py +++ b/tests/test_path_safety.py @@ -277,3 +277,57 @@ def test_a_symlinked_note_is_not_listed_twice(self, vault_root: Path) -> None: found = [paths.to_relative(vault_root, p) for p in paths.walk_notes(vault_root)] assert len(found) == len(set(found)) assert "alias.md" not in found + + +class TestRelativeToWalkedRoot: + """The index skips the realpath per note; this is why it is allowed to. + + `to_relative` resolves both sides and refuses anything landing outside, and + that check is the reason this module exists. `relative_to_walked_root` skips + it because `walk_notes` already guarantees the property, and these tests are + the guarantee: if the two ever disagree on a path the walk produced, the + faster one is wrong and the index is indexing something it should not. + """ + + def test_it_agrees_with_to_relative_on_every_walked_path(self, vault_root: Path) -> None: + root_resolved = vault_root.resolve() + walked = list(paths.walk_notes(vault_root, include_hidden=True)) + assert walked, "fixture must contain notes or this proves nothing" + for absolute in walked: + assert paths.relative_to_walked_root(root_resolved, absolute) == paths.to_relative( + vault_root, absolute + ) + + @pytest.mark.skipif(os.name == "nt", reason="symlinks need privileges on Windows") + def test_they_still_agree_when_the_vault_root_is_itself_a_symlink( + self, vault_root: Path, tmp_path: Path + ) -> None: + """The walk resolves the root once, so a symlinked vault is the normal case.""" + link = tmp_path / "vault-link" + link.symlink_to(vault_root, target_is_directory=True) + root_resolved = link.resolve() + for absolute in paths.walk_notes(link, include_hidden=True): + assert paths.relative_to_walked_root(root_resolved, absolute) == paths.to_relative( + link, absolute + ) + + @pytest.mark.skipif(os.name == "nt", reason="symlinks need privileges on Windows") + def test_a_symlink_escaping_the_vault_never_reaches_it( + self, vault_root: Path, tmp_path: Path + ) -> None: + """The walk skips symlinks, so the fast path is never handed an escape. + + Belt and braces on the one thing that would make skipping the resolve + unsafe: a note that looks like it is inside and is not. + """ + outside = tmp_path / "outside" + outside.mkdir() + (outside / "Secret.md").write_text("not yours\n") + (vault_root / "escape.md").symlink_to(outside / "Secret.md") + (vault_root / "escape-dir").symlink_to(outside, target_is_directory=True) + + root_resolved = vault_root.resolve() + for absolute in paths.walk_notes(vault_root, include_hidden=True): + rel = paths.relative_to_walked_root(root_resolved, absolute) + assert not rel.startswith("..") + assert "Secret.md" not in rel diff --git a/tests/test_provider.py b/tests/test_provider.py index 22490c6..0e2d1fe 100644 --- a/tests/test_provider.py +++ b/tests/test_provider.py @@ -169,6 +169,27 @@ def test_backlinks_ignore_a_link_in_a_fence(self, provider) -> None: paths = {note.path for note in provider.backlinks("Areas/Work/Acme.md")} assert "Fenced.md" not in paths + def test_a_note_linking_the_same_target_twice_is_one_backlink(self, provider) -> None: + """The dedupe moved from scanning the holder list to a set beside it. + + The list was scanned per link, which is quadratic on the note every vault + has: the index or MOC note the whole vault points at. Order and + uniqueness both have to survive the change. + """ + provider.write( + "Repeater.md", + "See [[Acme]] and again [[Acme]] and [[Areas/Work/Acme]] once more.\n", + mode="create", + ) + paths = [note.path for note in provider.backlinks("Areas/Work/Acme.md")] + assert paths.count("Repeater.md") == 1 + assert len(paths) == len(set(paths)) + + def test_backlink_order_is_stable_across_refreshes(self, provider) -> None: + first = [note.path for note in provider.backlinks("Areas/Work/Acme.md")] + provider.index.refresh(force=True) + assert [note.path for note in provider.backlinks("Areas/Work/Acme.md")] == first + def test_links_include_unresolved_by_default(self, provider) -> None: links = provider.links("Areas/Work/Acme.md") assert any(link.resolved_path is None for link in links) From f8e919e419f284159b78065f59b4112b045c3d4b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 06:53:45 +0000 Subject: [PATCH 4/5] Say which of these names the other half actually depends on The contract listed seven names and got two of them wrong in each direction. `usage.track_event` and `server.SERVER_VERSION` are listed as things the private package imports, and it imports neither. `track_event` is a stub nothing in either package calls, kept alive by a claim that someone depends on it, which is how dead code outlives its reason. Missing, and more expensive: the private package imports `providers.filesystem.markdown` and `providers.filesystem.paths` directly, and builds its own backend on them. Those read as internal here, deliberately so: `providers/filesystem/` is described a few lines up as the only place that knows about the filesystem, and `paths.py` is the confinement boundary with its own regression file. Renaming either would have broken a downstream backend with nothing in this repo's tests or CI to notice. The list says so now, along with the exception hierarchy, the protocol dataclasses and `FilesystemVaultProvider`, which are also imported and were also unlisted. Split into overridden, imported and reserved, so the next reader can tell which kind of promise each name is. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FbPQWSXitqNEkRE6BtqrXM --- CLAUDE.md | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ec49e3e..c180e44 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -131,18 +131,39 @@ package. ## Open-core extension contract -The admin package subclasses/imports these -- rename only in coordination with it: +The private package extends this one through the names below. **Rename any of +them only in coordination with it**, because nothing in this repo's tests or CI +will notice if you break one. + +Overridden -- the hooks that exist to be replaced: - `server.create_fastmcp_app(*, auth=None, token_verifier=None, extra_instructions=None)` -- single source of truth for FastMCP construction. -- `tools.handler.VaultToolHandler._get_provider` -- the hook admin overrides to - resolve a per-workspace vault from the authenticated subject. -- `tools.handler.VaultToolHandler._list_vaults` -- the hook admin overrides to - list a workspace's vaults. +- `tools.handler.VaultToolHandler._get_provider` -- resolve a per-tenant vault + from the authenticated subject. +- `tools.handler.VaultToolHandler._list_vaults` -- list a tenant's vaults. - `tools.handler.VaultToolHandler._track_usage` -- usage-tracking hook (no-op here). - `tools._common._current_sub` -- contextvar carrying the authenticated subject. -- `usage.track_event` -- no-op stub here; the real tracker lives in admin. -- `config.KnapConfig` and `server.SERVER_VERSION`. + +Imported -- ordinary use of this package's API, listed because it is easy to +mistake some of it for internal: + +- `config.KnapConfig`. +- `error_handling`: `KnapError`, `ValidationError`, `sanitize`. +- `providers`: `VaultProvider` and the exception hierarchy. +- `providers.protocol`: the value dataclasses. +- `providers.filesystem.provider.FilesystemVaultProvider`, composed rather than + subclassed. +- **`providers.filesystem.markdown` and `providers.filesystem.paths`.** These + read as internal -- `providers/filesystem/` is described above as the only + place that knows about the filesystem, and `paths.py` is a security boundary + with its own regression file -- and a downstream backend is built on them + anyway. Treat their signatures as public. + +Reserved, and not used by anything today: `usage.track_event` is a stub that +nothing in either package calls, and `server.SERVER_VERSION` is used here but +imported by nobody. Keep or drop them on their own merits; "the private package +depends on it" is not currently a reason. ## Conventions From 832de8a957d0b5c8ef6a1264a604d0b6607c7768 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 07:15:53 +0000 Subject: [PATCH 5/5] Make the escape test fail when the escape is possible Review took the previous commit's safety test apart, and it deserved it. The test claimed to be belt and braces on the one thing that would make skipping the resolve unsafe, and it checked that the relative path did not begin with `..` and did not contain the secret's name. Neither can catch the case that matters: a symlink named `escape.md` produces the relative path `escape.md`, which says nothing about where its bytes are, and `relative_to` cannot emit `..` at all, so that assertion was always true. Deleting the symlink skip from `walk_notes` left the old test green while a file outside the vault was readable through it. The test now resolves what `root / rel` actually reaches, which is what a caller does with the value, and asserts the fast path agrees with `to_relative` on every walked path. Same counter-proof against the same edit: it fails now. `relative_to_walked_root` also raised pathlib's `ValueError` on misuse, in a module where every other refusal is a `PathNotAllowedError` that deliberately does not echo the path. Comparing `parts` costs no syscall, so the check is back and the error is the module's own, without a resolve per note. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FbPQWSXitqNEkRE6BtqrXM --- knap_mcp/providers/filesystem/paths.py | 11 ++++++++- tests/test_path_safety.py | 33 +++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/knap_mcp/providers/filesystem/paths.py b/knap_mcp/providers/filesystem/paths.py index 28eb1d2..65e8e1c 100644 --- a/knap_mcp/providers/filesystem/paths.py +++ b/knap_mcp/providers/filesystem/paths.py @@ -173,8 +173,17 @@ def relative_to_walked_root(root_resolved: Path, absolute: Path) -> str: costs. Pass ``root_resolved`` already resolved, once, by the caller. Only feed this - paths from the walk. + paths from the walk. The guarantee is a POSIX one: ``resolve`` does not cross + a bind mount, and ``walk_notes`` refuses symlinks. A Windows junction is + neither, so on Windows this is a check worth keeping rather than skipping. + + Misuse still raises ``PathNotAllowedError`` like everything else here, rather + than the ``ValueError`` ``relative_to`` would give: comparing ``parts`` + costs nothing, and a module whose whole job is one error type should not have + one entrance that throws a different one. """ + if not _is_within(absolute, root_resolved): + raise _reject("is not under the vault root") return unicodedata.normalize("NFC", absolute.relative_to(root_resolved).as_posix()) diff --git a/tests/test_path_safety.py b/tests/test_path_safety.py index 5cc8710..74d7d57 100644 --- a/tests/test_path_safety.py +++ b/tests/test_path_safety.py @@ -319,6 +319,15 @@ def test_a_symlink_escaping_the_vault_never_reaches_it( Belt and braces on the one thing that would make skipping the resolve unsafe: a note that looks like it is inside and is not. + + The assertion has to follow the path rather than read it. An earlier + version of this test checked that the relative form did not start with + `..` and did not contain the secret's name, and both are satisfied by + the case that matters: a symlink named `escape.md` produces the relative + path `escape.md`, which says nothing about where its bytes live. Deleting + the symlink skip from `walk_notes` left that version green while the file + outside the vault was readable. So resolve what `root / rel` actually + reaches, which is what a caller does with it. """ outside = tmp_path / "outside" outside.mkdir() @@ -327,7 +336,25 @@ def test_a_symlink_escaping_the_vault_never_reaches_it( (vault_root / "escape-dir").symlink_to(outside, target_is_directory=True) root_resolved = vault_root.resolve() - for absolute in paths.walk_notes(vault_root, include_hidden=True): + walked = list(paths.walk_notes(vault_root, include_hidden=True)) + assert walked, "fixture must contain notes or this proves nothing" + for absolute in walked: rel = paths.relative_to_walked_root(root_resolved, absolute) - assert not rel.startswith("..") - assert "Secret.md" not in rel + landed = (vault_root / rel).resolve() + assert landed.is_relative_to(root_resolved), f"{rel} reaches {landed}" + # And the fast path agrees with the checked one on every walked path, + # which is the property that makes skipping the resolve legitimate. + assert rel == paths.to_relative(vault_root, absolute) + + def test_misuse_raises_the_modules_own_error(self, vault_root: Path, tmp_path: Path) -> None: + """One module, one error type. `relative_to` would raise ValueError.""" + outside = tmp_path / "elsewhere" / "note.md" + with pytest.raises(PathNotAllowedError): + paths.relative_to_walked_root(vault_root.resolve(), outside) + + def test_the_error_does_not_echo_the_path(self, vault_root: Path, tmp_path: Path) -> None: + """Same rule as the rest of the module: an error is not a probe answering itself.""" + outside = tmp_path / "elsewhere" / "Secret.md" + with pytest.raises(PathNotAllowedError) as excinfo: + paths.relative_to_walked_root(vault_root.resolve(), outside) + assert "Secret" not in str(excinfo.value)