diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index b17e5f8..39b144d 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -140,8 +140,14 @@ async def delete_milestone(milestone_id: int) -> dict: # Read the title BEFORE the delete: afterwards the row is trashed and the # confirmation could only echo the number back. A deletion the operator # cannot recognise is one they cannot tell was the wrong one. - doomed = await milestones_svc.get_milestone(uid, milestone_id) - title = getattr(doomed, "title", "") if doomed else "" + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + doomed = await milestones_svc.get_milestone(uid, milestone_id) + title = getattr(doomed, "title", "") if doomed else "" + except Exception: + title = "" batch = await trash_svc.delete(uid, "milestone", milestone_id) if batch is None: raise ValueError(f"milestone {milestone_id} not found") diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index b6864c7..aa5195c 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -327,8 +327,14 @@ async def delete_note(note_id: int) -> dict: # Read the title BEFORE the delete: afterwards the row is trashed and the # confirmation could only echo the number back. A deletion the operator # cannot recognise is one they cannot tell was the wrong one. - loaded = await notes_svc.get_note_for_user(uid, note_id) - title = getattr(loaded[0], "title", "") if loaded else "" + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + loaded = await notes_svc.get_note_for_user(uid, note_id) + title = getattr(loaded[0], "title", "") if loaded else "" + except Exception: + title = "" batch = await trash_svc.delete(uid, "note", note_id) if batch is None: raise ValueError(f"note {note_id} not found") diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index b8f416c..ca6a8c3 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -604,7 +604,12 @@ async def rule_history(rule_id: int, version_id: int = 0) -> dict: versions = await rulebooks_svc.list_rule_versions(rule_id, uid) if versions is None: raise ValueError(f"rule {rule_id} not found") - rule = await rulebooks_svc.get_rule(rule_id, uid) + # Fail-open, like the deletes: a missing title must not turn a readable + # history into an error. + try: + rule = await rulebooks_svc.get_rule(rule_id, uid) + except Exception: + rule = None return { "rule_id": rule_id, "title": rule.title if rule else "", diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index 251c438..5b5c563 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -496,8 +496,14 @@ async def delete_snippet(snippet_id: int) -> dict: uid = current_user_id() # Read before deleting so the confirmation can NAME what went — an id # alone leaves the operator unable to tell which snippet this was. - doomed = await snippets_svc.get_snippet(uid, snippet_id) - title = getattr(doomed, "title", "") if doomed else "" + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + doomed = await snippets_svc.get_snippet(uid, snippet_id) + title = getattr(doomed, "title", "") if doomed else "" + except Exception: + title = "" if not await snippets_svc.delete_snippet(uid, snippet_id): raise ValueError(f"snippet {snippet_id} not found") return {"deleted": True, "id": snippet_id, "title": title} diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index c837c2f..e097292 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -367,8 +367,14 @@ async def delete_task(task_id: int) -> dict: # Read the title BEFORE the delete: afterwards the row is trashed and the # confirmation could only echo the number back. A deletion the operator # cannot recognise is one they cannot tell was the wrong one. - loaded = await notes_svc.get_note_for_user(uid, task_id) - title = getattr(loaded[0], "title", "") if loaded else "" + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + loaded = await notes_svc.get_note_for_user(uid, task_id) + title = getattr(loaded[0], "title", "") if loaded else "" + except Exception: + title = "" batch = await trash_svc.delete(uid, "task", task_id) if batch is None: raise ValueError(f"task {task_id} not found") diff --git a/tests/test_mcp_tool_notes.py b/tests/test_mcp_tool_notes.py index 1ea1b32..6637f25 100644 --- a/tests/test_mcp_tool_notes.py +++ b/tests/test_mcp_tool_notes.py @@ -1,5 +1,5 @@ """Tests for fable_*_note tools.""" -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -294,12 +294,37 @@ async def test_update_note_raises_when_not_found(): @pytest.mark.asyncio async def test_delete_note_soft_deletes_and_returns_batch(): + doomed = MagicMock() + doomed.title = "the note that went" with patch( "scribe.mcp.tools.notes.trash_svc.delete", AsyncMock(return_value="batch-1"), + ), patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=(doomed, "owner")), ): result = await delete_note(note_id=7) assert result["deleted_batch_id"] == "batch-1" + # The confirmation NAMES what went (#3273). After the delete the row is + # trashed, so this line is the last chance to say which note it was. + assert result["title"] == "the note that went" + assert "the note that went" in result["message"] + + +@pytest.mark.asyncio +async def test_delete_note_still_deletes_when_the_title_lookup_fails(): + """The title is a courtesy on top of the delete, never a precondition for + it. A lookup that errors costs the name, not the operation.""" + with patch( + "scribe.mcp.tools.notes.trash_svc.delete", + AsyncMock(return_value="batch-1"), + ), patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(side_effect=RuntimeError("database down")), + ): + result = await delete_note(note_id=7) + assert result["deleted_batch_id"] == "batch-1" + assert result["title"] == "" @pytest.mark.asyncio @@ -307,6 +332,9 @@ async def test_delete_note_raises_when_not_found(): with patch( "scribe.mcp.tools.notes.trash_svc.delete", AsyncMock(return_value=None), + ), patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=None), ): with pytest.raises(ValueError, match="note 999 not found"): await delete_note(note_id=999) diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index d80685e..8b96917 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -470,6 +470,11 @@ async def test_rule_history_lists_without_the_heavy_text(): with patch( "scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions", AsyncMock(return_value=[_fake_version(), _fake_version(id=4)]), + ), patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=fake_rule( + id=100, title="The runner has no bash", statement="s", topic_id=10, + )), ): from scribe.mcp.tools.rulebooks import rule_history out = await rule_history(rule_id=100) @@ -503,6 +508,11 @@ async def test_rule_history_says_an_empty_history_is_ordinary(): with patch( "scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions", AsyncMock(return_value=[]), + ), patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=fake_rule( + id=100, title="The runner has no bash", statement="s", topic_id=10, + )), ): from scribe.mcp.tools.rulebooks import rule_history out = await rule_history(rule_id=100) diff --git a/tests/test_mcp_tool_snippets.py b/tests/test_mcp_tool_snippets.py index 7773f14..c7bb5b5 100644 --- a/tests/test_mcp_tool_snippets.py +++ b/tests/test_mcp_tool_snippets.py @@ -138,9 +138,18 @@ async def test_merge_snippets_read_only_target_says_why(): @pytest.mark.asyncio async def test_delete_snippet_retires_or_raises(): from scribe.mcp.tools.snippets import delete_snippet - with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=True)): - assert await delete_snippet(1) == {"deleted": True, "id": 1} - with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=False)): + + doomed = MagicMock() + doomed.title = "debounce helper" + # The confirmation names what went (#3273) — after the delete there is + # nothing left to look the title up from. + with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=True)), \ + patch("scribe.services.snippets.get_snippet", AsyncMock(return_value=doomed)): + assert await delete_snippet(1) == { + "deleted": True, "id": 1, "title": "debounce helper", + } + with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=False)), \ + patch("scribe.services.snippets.get_snippet", AsyncMock(return_value=None)): with pytest.raises(ValueError): await delete_snippet(404)