diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index e10e315..18fcab9 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -69,12 +69,11 @@ async def _attach_supersession(uid: int, note_id: int, data: dict) -> None: permanently-empty lists. A field that always says nothing trains readers to skip fields, which is the lesson `consolidated_at` cost us (#2483). """ - supersedes = await supersession_svc.get_supersedes(uid, note_id) - superseded_by = await supersession_svc.get_superseded_by(uid, note_id) - if supersedes: - data["supersedes"] = supersedes - if superseded_by: - data["superseded_by"] = superseded_by + rel = await supersession_svc.get_relations(uid, note_id) + if rel["supersedes"]: + data["supersedes"] = rel["supersedes"] + if rel["superseded_by"]: + data["superseded_by"] = rel["superseded_by"] data["superseded_note"] = ( "A later note claims to bring this up to date — see superseded_by. " "Read this as what was true when written, and check the newer one " diff --git a/src/scribe/routes/notes.py b/src/scribe/routes/notes.py index fd4a010..decd940 100644 --- a/src/scribe/routes/notes.py +++ b/src/scribe/routes/notes.py @@ -36,12 +36,11 @@ async def _attach_supersession(uid: int, note_id: int, data: dict) -> None: Omitted when empty: a field that always says nothing trains readers to skip fields, which is what `consolidated_at` cost (#2483). """ - supersedes = await supersession_svc.get_supersedes(uid, note_id) - superseded_by = await supersession_svc.get_superseded_by(uid, note_id) - if supersedes: - data["supersedes"] = supersedes - if superseded_by: - data["superseded_by"] = superseded_by + rel = await supersession_svc.get_relations(uid, note_id) + if rel["supersedes"]: + data["supersedes"] = rel["supersedes"] + if rel["superseded_by"]: + data["superseded_by"] = rel["superseded_by"] from scribe.services.note_versions import list_versions, get_version logger = logging.getLogger(__name__) diff --git a/src/scribe/services/supersession.py b/src/scribe/services/supersession.py index 48514cc..eb2fac1 100644 --- a/src/scribe/services/supersession.py +++ b/src/scribe/services/supersession.py @@ -129,33 +129,40 @@ async def set_supersedes( return wanted -async def get_supersedes(user_id: int, note_id: int) -> list[int]: - """Ids this note claims to supersede. Empty if the caller can't read it.""" - if not await access.can_read_note(user_id, note_id): - return [] - async with async_session() as session: - return [int(i) for i in (await session.execute( - select(NoteSupersession.superseded_id) - .where(NoteSupersession.superseder_id == note_id) - .order_by(NoteSupersession.superseded_id) - )).scalars().all()] +async def get_relations(user_id: int, note_id: int) -> dict[str, list[int]]: + """Both directions for one note: what it supersedes, and what supersedes it. + ONE query and ONE ACL check, because this runs on every note read. Asking + the two questions separately doubled the round trips on the hottest path in + the product to save a two-line partition — the wrong trade, and one I made + on the first attempt. -async def get_superseded_by(user_id: int, note_id: int) -> list[int]: - """Ids claiming to supersede this note. + Returns {"supersedes": [...], "superseded_by": [...]}, both sorted. Empty + lists when the caller cannot read the note. - The direction that matters to a READER, and the one the note itself cannot - know. An agent handed a stale record with no marker acts on it confidently; - that is worse than never surfacing it at all. + `superseded_by` is the direction that matters to a READER and the one the + note itself cannot know. An agent handed a stale record with no marker acts + on it confidently, which is worse than never surfacing it at all. """ + empty: dict[str, list[int]] = {"supersedes": [], "superseded_by": []} if not await access.can_read_note(user_id, note_id): - return [] + return empty async with async_session() as session: - return [int(i) for i in (await session.execute( - select(NoteSupersession.superseder_id) - .where(NoteSupersession.superseded_id == note_id) - .order_by(NoteSupersession.superseder_id) - )).scalars().all()] + rows = (await session.execute( + select( + NoteSupersession.superseder_id, NoteSupersession.superseded_id + ).where( + (NoteSupersession.superseder_id == note_id) + | (NoteSupersession.superseded_id == note_id) + ) + )).all() + supersedes = sorted( + int(old) for new, old in rows if int(new) == note_id + ) + superseded_by = sorted( + int(new) for new, old in rows if int(old) == note_id + ) + return {"supersedes": supersedes, "superseded_by": superseded_by} async def superseded_ids(note_ids: list[int]) -> set[int]: diff --git a/tests/test_mcp_tool_notes.py b/tests/test_mcp_tool_notes.py index 6836f20..ff56c60 100644 --- a/tests/test_mcp_tool_notes.py +++ b/tests/test_mcp_tool_notes.py @@ -17,6 +17,21 @@ def _bind_user(): _user_id_ctx.reset(token) +@pytest.fixture(autouse=True) +def _no_supersession(): + """Every note read/write now asks for its supersession relations (#278). + + These are unit tests of the TOOL layer and this job has no database — the + same hazard the `_fake_note` comment below records for note 2109. Stubbed + to "no relations", which is the state of essentially every note; the + relation's own behaviour is covered in test_services_supersession.py, and + the attachment is covered explicitly below. + """ + with patch("scribe.mcp.tools.notes.supersession_svc.get_relations", + AsyncMock(return_value={"supersedes": [], "superseded_by": []})): + yield + + def _fake_note(*, user_id: int = 7, **overrides) -> MagicMock: note = MagicMock() base = {"id": 1, "title": "t", "body": "b", "tags": [], "is_task": False} @@ -123,6 +138,30 @@ async def test_get_note_returns_dict(): assert out["title"] == "found" # Own record: no provenance noise. assert "shared" not in out + # No supersession relations: both keys ABSENT, not present-and-empty. A + # field that always says nothing trains readers to skip fields (#2483). + assert "supersedes" not in out + assert "superseded_by" not in out + + +@pytest.mark.asyncio +async def test_get_note_warns_in_words_when_a_later_note_overtook_it(): + """The label is the point, not the ids. + + A superseded record still surfaces — supersession demotes, it never hides — + so an agent WILL read stale material. Handing it over with only a numeric + field to notice would be worse than not surfacing it, because the reader + acts on it confidently either way. + """ + fake = _fake_note(id=5, title="June's answer") + with patch("scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner"))), \ + patch("scribe.mcp.tools.notes.supersession_svc.get_relations", + AsyncMock(return_value={"supersedes": [], "superseded_by": [9]})): + out = await get_note(note_id=5) + assert out["superseded_by"] == [9] + assert "superseded_note" in out + assert "before acting" in out["superseded_note"] @pytest.mark.asyncio diff --git a/tests/test_services_supersession.py b/tests/test_services_supersession.py index 5fac61e..9c7d59c 100644 --- a/tests/test_services_supersession.py +++ b/tests/test_services_supersession.py @@ -144,5 +144,32 @@ async def test_superseded_ids_is_empty_for_an_empty_candidate_set(): async def test_reads_are_empty_when_the_caller_cannot_read_the_note(): with patch("scribe.services.supersession.access.can_read_note", AsyncMock(return_value=False)): - assert await supersession.get_supersedes(7, 1) == [] - assert await supersession.get_superseded_by(7, 1) == [] + assert await supersession.get_relations(7, 1) == { + "supersedes": [], "superseded_by": [] + } + + +@pytest.mark.asyncio +async def test_get_relations_partitions_both_directions_from_one_query(): + """ONE round trip for both directions, because this runs on every note read. + + Note 5 supersedes 2 and 3, and is itself superseded by 9. All four rows come + back from a single OR query and are partitioned by which column holds 5. + """ + session = AsyncMock() + session.__aenter__ = AsyncMock(return_value=session) + session.__aexit__ = AsyncMock(return_value=False) + result = MagicMock() + result.all.return_value = [(5, 3), (5, 2), (9, 5)] # (superseder, superseded) + session.execute = AsyncMock(return_value=result) + + with patch("scribe.services.supersession.access.can_read_note", + AsyncMock(return_value=True)), \ + patch("scribe.services.supersession.async_session", return_value=session): + rel = await supersession.get_relations(7, 5) + + assert rel == {"supersedes": [2, 3], "superseded_by": [9]} + assert session.execute.await_count == 1, ( + "both directions must come from one query — asking separately doubles " + "the round trips on the hottest path in the product" + )