From 72884f0f95442ea89ed64a03c169d468d5b3c054 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 13:30:46 +0000 Subject: [PATCH] Say what we know when a vault carries no Obsidian settings vault_daily_note had one answer for two different situations. A vault with .obsidian and no daily-notes.json really does have the plugin off, and "switch on Daily Notes in Obsidian" fixes it. A vault that arrives without an .obsidian folder has told us nothing, and that same sentence sends somebody to a setting that is already correct: they change nothing, and the tool keeps saying the same thing. The distinction is drawn where the vault is: periodic.resolve now asks whether the settings folder is there at all before it phrases a refusal. VaultSettingsUnavailable carries the honest version, and it is a subclass of PeriodicNotesNotConfigured so anything already catching the parent keeps working. Both classes now live in providers/protocol.py, which is where any backend can raise them and where the tool layer may look without reaching into the filesystem; periodic.py imports the parent back under its old name, so that import path is unchanged. The tool docstring stops telling the model to blame the plugin and tells it to pass on what the refusal says. Addresses pantalytics/knap-mcp-admin#111 (direction 3). Nothing here guesses a folder, which principle 6 rules out anyway. Co-Authored-By: Claude Opus 5 --- knap_mcp/providers/__init__.py | 4 ++ knap_mcp/providers/filesystem/periodic.py | 50 +++++++++++++------ knap_mcp/providers/protocol.py | 24 ++++++++- knap_mcp/tools/vault/periodic.py | 9 ++-- tests/test_provider_writes.py | 61 ++++++++++++++++++----- 5 files changed, 118 insertions(+), 30 deletions(-) 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)