diff --git a/knap_mcp/providers/__init__.py b/knap_mcp/providers/__init__.py index b5e230b..c46e274 100644 --- a/knap_mcp/providers/__init__.py +++ b/knap_mcp/providers/__init__.py @@ -21,6 +21,7 @@ PatchMode, PathNotAllowedError, PeriodicKind, + PeriodicNotesNotConfigured, ProviderError, QuotaExceeded, RevisionMismatch, @@ -28,6 +29,7 @@ VaultInfo, VaultNotFoundError, VaultProvider, + VaultSettingsUnavailable, WriteMode, ) @@ -44,6 +46,7 @@ "PatchMode", "PathNotAllowedError", "PeriodicKind", + "PeriodicNotesNotConfigured", "ProviderError", "QuotaExceeded", "RevisionMismatch", @@ -51,5 +54,6 @@ "VaultInfo", "VaultNotFoundError", "VaultProvider", + "VaultSettingsUnavailable", "WriteMode", ] diff --git a/knap_mcp/providers/filesystem/periodic.py b/knap_mcp/providers/filesystem/periodic.py index 455f068..d774325 100644 --- a/knap_mcp/providers/filesystem/periodic.py +++ b/knap_mcp/providers/filesystem/periodic.py @@ -14,6 +14,14 @@ 3. Nothing configured, in which case a read reports that and a create refuses, rather than inventing `YYYY-MM-DD.md` in the vault root. +Case 3 comes in two shapes and they need different sentences. A vault with an +`.obsidian` folder and no periodic-notes settings in it really does have the +plugin switched off, and saying so is useful. A vault with no `.obsidian` folder +at all has told us nothing, and a backend is free to hand over notes and +attachments without it, so "the plugin is off" would be a guess. Worse, it is a +guess that sends somebody to a setting that is probably already right, and they +come back to the same sentence. + Moment.js format tokens are what Obsidian stores, so a small translator lives here. Only the tokens people actually put in a daily-note format are supported; anything else is reported rather than approximated, because a filename that is @@ -29,7 +37,12 @@ from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple from ...logging_config import get_logger -from ..protocol import PeriodicKind, ProviderError +from ..protocol import ( + PeriodicKind, + PeriodicNotesNotConfigured, + ProviderError, + VaultSettingsUnavailable, +) from . import markdown as md from . import paths as vault_paths @@ -45,14 +58,6 @@ } -class PeriodicNotesNotConfigured(ProviderError): - """The vault has no periodic-notes settings for this kind. - - Its own error type because it is not a failure so much as an answer: the - client should tell the user to switch the plugin on, not retry. - """ - - def resolve( provider: "FilesystemVaultProvider", kind: PeriodicKind = "daily", @@ -66,11 +71,7 @@ def resolve( settings = read_settings(provider.root, kind) if settings is None: - raise PeriodicNotesNotConfigured( - f"This vault has no {kind} notes configured. Switch on the core Daily Notes " - "plugin (or Periodic Notes for weekly and monthly) in Obsidian first, so the " - "note lands where the rest of them are." - ) + raise _nothing_to_read(provider.root, kind) target = _parse_date(when) filename = format_moment(settings["format"], target, kind) @@ -123,6 +124,27 @@ def read_settings(root: Path, kind: str) -> Optional[Dict[str, Any]]: return None +def _nothing_to_read(root: Path, kind: str) -> PeriodicNotesNotConfigured: + """The error for a kind we could not resolve, and which of the two it is. + + The test is the settings folder itself, not the file for this kind: a vault + that has `.obsidian` and no daily-notes.json has the plugin switched off, + and one without `.obsidian` has not said. + """ + if not (root / ".obsidian").is_dir(): + return VaultSettingsUnavailable( + "There are no Obsidian settings in this vault, so nothing here says where the " + f"{kind} notes go. The plugin may well be on: some vaults arrive as notes and " + "attachments only, and the settings do not come with them. Ask the user which " + "folder they are in and what the filenames look like, then use that path." + ) + return PeriodicNotesNotConfigured( + f"This vault has no {kind} notes configured. Switch on the core Daily Notes " + "plugin (or Periodic Notes for weekly and monthly) in Obsidian first, so the " + "note lands where the rest of them are." + ) + + def _read_json(path: Path) -> Optional[Any]: try: return json.loads(path.read_text(encoding="utf-8")) diff --git a/knap_mcp/providers/protocol.py b/knap_mcp/providers/protocol.py index 35668fe..3238635 100644 --- a/knap_mcp/providers/protocol.py +++ b/knap_mcp/providers/protocol.py @@ -83,6 +83,26 @@ class QuotaExceeded(ProviderError): """ +class PeriodicNotesNotConfigured(ProviderError): + """The vault has no periodic-notes settings for this kind. + + Its own error type because it is not a failure so much as an answer: the + client should tell the user to switch the plugin on, not retry. + """ + + +class VaultSettingsUnavailable(PeriodicNotesNotConfigured): + """The vault carries no Obsidian settings, so there is nothing to read them from. + + A backend may hand over notes and attachments without the ``.obsidian`` + folder, and then no setting in it can be read either way. A subclass rather + than a sibling so that anything already catching the parent keeps working, + and its own type because the two answers are different: telling somebody the + plugin is off is a claim we cannot make about settings we cannot see, and it + sends them to change something that is probably already right. + """ + + # --------------------------------------------------------------------------- # # Transport-neutral value objects. # --------------------------------------------------------------------------- # @@ -439,7 +459,9 @@ def periodic_note( daily note that lands somewhere other than where the customer's Obsidian would have put it is a second daily note, not a daily note. Nothing is guessed: with no settings and no ``create``, this reports that the vault - has no periodic notes configured. + has no periodic notes configured. A backend that carries no ``.obsidian`` + folder at all raises ``VaultSettingsUnavailable`` instead, because it + cannot tell an unconfigured vault from a configured one. """ ... diff --git a/knap_mcp/tools/vault/periodic.py b/knap_mcp/tools/vault/periodic.py index 88cb984..6cd15c2 100644 --- a/knap_mcp/tools/vault/periodic.py +++ b/knap_mcp/tools/vault/periodic.py @@ -33,9 +33,12 @@ async def vault_daily_note( Then vault_append_note against the path this returns is how a captured thought reaches today's note. - If the vault has no periodic notes configured, this says so. Do not - invent a path: tell the user to switch on Daily Notes in Obsidian, so - the whole vault agrees where they go. + When the path cannot be resolved, this says why, and the two reasons + need different answers. A vault with the plugin switched off is a + vault where switching Daily Notes on in Obsidian fixes it. A vault + that carries no Obsidian settings cannot be fixed that way and the + refusal says so. Pass on what it tells you rather than sending the + user to a setting, and never invent a path. Args: date: ISO date (YYYY-MM-DD), or "today", "yesterday", diff --git a/tests/test_provider_writes.py b/tests/test_provider_writes.py index cff6c76..d01ac17 100644 --- a/tests/test_provider_writes.py +++ b/tests/test_provider_writes.py @@ -11,16 +11,30 @@ import pytest +from knap_mcp.error_handling import describe from knap_mcp.providers.filesystem.provider import FilesystemVaultProvider from knap_mcp.providers.protocol import ( NoteExistsError, NoteNotFoundError, PathNotAllowedError, + PeriodicNotesNotConfigured, ProviderError, RevisionMismatch, + VaultSettingsUnavailable, ) +def _vault_with(root: Path, files: dict[str, str]) -> FilesystemVaultProvider: + """A connected provider over a vault holding exactly these files.""" + for rel, text in files.items(): + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + provider = FilesystemVaultProvider(root) + provider.connect() + return provider + + class TestWrite: def test_create_then_read_back(self, provider) -> None: ref = provider.write("New/Note.md", "# New\n\nBody.\n", mode="create") @@ -343,24 +357,47 @@ def test_today_and_yesterday_are_understood(self, provider) -> None: != provider.periodic_note("daily", "today")[0] ) - def test_a_vault_with_no_settings_says_so(self, tmp_path: Path) -> None: - """Rather than inventing YYYY-MM-DD.md in the vault root.""" - from knap_mcp.providers.filesystem.periodic import PeriodicNotesNotConfigured + def test_a_vault_with_settings_and_no_daily_notes_says_switch_it_on( + self, tmp_path: Path + ) -> None: + """The plugin really is off here, so the advice really does fix it.""" + off = _vault_with(tmp_path / "off", {".obsidian/app.json": "{}"}) + + with pytest.raises(PeriodicNotesNotConfigured) as caught: + off.periodic_note("daily") + + assert not isinstance(caught.value, VaultSettingsUnavailable) + assert "Switch on" in str(caught.value) + + def test_a_vault_without_obsidian_settings_does_not_blame_the_plugin( + self, tmp_path: Path + ) -> None: + """The failure the message used to hide. + + A vault can arrive as notes and attachments with no ``.obsidian`` folder, + and then the setting cannot be read either way. Telling somebody to + switch Daily Notes on sends them to change something that is already + right, and the tool says the same thing when they come back. + """ + bare = _vault_with(tmp_path / "bare", {"note.md": "# x\n"}) - root = tmp_path / "bare" - root.mkdir() - (root / "note.md").write_text("# x\n") - bare = FilesystemVaultProvider(root) - bare.connect() - with pytest.raises(PeriodicNotesNotConfigured): + with pytest.raises(VaultSettingsUnavailable) as caught: bare.periodic_note("daily") - def test_weekly_needs_the_periodic_notes_plugin(self, provider) -> None: - from knap_mcp.providers.filesystem.periodic import PeriodicNotesNotConfigured + message = str(caught.value) + assert "no Obsidian settings in this vault" in message + assert "witch on" not in message, "there is nothing here to switch on" + assert "has no daily notes configured" not in message, "we cannot know that" + assert describe(caught.value) == message, "and that is the sentence a client sees" - with pytest.raises(PeriodicNotesNotConfigured): + def test_weekly_needs_the_periodic_notes_plugin(self, provider) -> None: + with pytest.raises(PeriodicNotesNotConfigured) as caught: provider.periodic_note("weekly") + assert not isinstance(caught.value, VaultSettingsUnavailable), ( + "this vault has settings, they just do not cover weekly" + ) + def test_the_periodic_notes_plugin_wins_over_daily_notes(self, provider) -> None: plugin = provider.root / ".obsidian" / "plugins" / "periodic-notes" plugin.mkdir(parents=True)