fix(scribe): a delete must not depend on the lookup that names it (#3273)
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 29s
CI & Build / TypeScript typecheck (push) Successful in 38s
CI & Build / Python tests (push) Successful in 1m15s
CI & Build / Build & push image (push) Successful in 36s
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 29s
CI & Build / TypeScript typecheck (push) Successful in 38s
CI & Build / Python tests (push) Successful in 1m15s
CI & Build / Build & push image (push) Successful in 36s
CI caught the naming work reaching for Postgres from the unit lane, and the connection error was the symptom of a real design fault rather than a test gap: reading the title BEFORE the delete put a live query on the delete path, so a lookup that failed would have stopped the delete happening. That is a decoration breaking its payload — the same mistake just fixed in rules_etag, made again two commits later. Every title lookup now fails open: delete_task, delete_note, delete_milestone, delete_snippet and rule_history lose the name, never the operation. The five unit tests mock the lookup rather than reaching for a database, and delete_note gains one asserting the delete still happens when the lookup raises — the behaviour, not just the absence of a crash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user