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 diff --git a/knap_mcp/providers/filesystem/frontmatter.py b/knap_mcp/providers/filesystem/frontmatter.py index 7fb004c..89ee269 100644 --- a/knap_mcp/providers/filesystem/frontmatter.py +++ b/knap_mcp/providers/filesystem/frontmatter.py @@ -141,18 +141,38 @@ 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. + + 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 '""' needs_quotes = ( @@ -161,13 +181,35 @@ 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, 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(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, 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: {written}") == {"x": value} + except Exception: # noqa: BLE001 - any failure to parse means "do not write this" + return False def _looks_numeric(value: str) -> bool: 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..65e8e1c 100644 --- a/knap_mcp/providers/filesystem/paths.py +++ b/knap_mcp/providers/filesystem/paths.py @@ -159,6 +159,34 @@ 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. 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()) + + 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/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_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 875de0d..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,91 +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"], - ) - 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. - """ - new = md.edit_frontmatter("Body\n", {"status": value}) - assert md.parse(new).frontmatter["status"] == value - - 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.""" - values = {"a": "true", "b": "x: y", "c": "-dash", "d": "#hash", "e": "100%"} - new = md.edit_frontmatter("Body\n", values) - inner = new.split("---\n")[1] - assert yaml.safe_load(inner) == values - - class TestSections: RAW = "# Title\n\n## Log\n\n- one\n\n### Sub\n\n- deep\n\n## Next\n\n- later\n" @@ -382,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..74d7d57 100644 --- a/tests/test_path_safety.py +++ b/tests/test_path_safety.py @@ -277,3 +277,84 @@ 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. + + 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() + (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() + 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) + 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) 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) 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):