From 7cba2f9793a0ac9d041c2a76c76cfc17c74792c8 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 10:38:28 +0200 Subject: [PATCH] [F011] playbook: de-index rejected/archived playbooks from the PLAYBOOKS RAG index --- roboco/services/optimal.py | 42 ++++++ .../optimal_brain/indexes/playbooks.py | 13 ++ roboco/services/playbook.py | 24 ++++ .../services/repositories/indexed_document.py | 21 ++- .../optimal_brain/test_playbooks_index.py | 21 +++ tests/unit/services/test_playbook_unindex.py | 128 ++++++++++++++++++ 6 files changed, 248 insertions(+), 1 deletion(-) create mode 100644 tests/unit/services/test_playbook_unindex.py diff --git a/roboco/services/optimal.py b/roboco/services/optimal.py index 5ad42757..0eff5e14 100644 --- a/roboco/services/optimal.py +++ b/roboco/services/optimal.py @@ -877,6 +877,48 @@ class OptimalService: }, ) + async def unindex_playbook(self, playbook_id: str) -> None: + """De-index a playbook from the PLAYBOOKS index (best-effort). + + The mirror of :meth:`index_playbook`: removes the playbook's embedded + chunks from the vector store AND drops its tracking row, so a + rejected/archived playbook stops surfacing in agent briefings. Both + steps are idempotent — a never-indexed draft playbook is a clean no-op. + Failures are logged and swallowed so a curation action (reject/archive) + never errors on the index side. + """ + from roboco.db import get_db_context + from roboco.services.repositories import IndexedDocumentRepository + + try: + plugin = self._get_plugin(IndexType.PLAYBOOKS) + if isinstance(plugin, PlaybooksIndexPlugin): + await plugin.delete_playbook(playbook_id) + else: + source = f"roboco://playbooks/{playbook_id}" + await plugin._require_store.delete_by_source(source) + except Exception as exc: + logger.warning( + "Playbook de-index (vector store) failed; continuing", + playbook_id=playbook_id, + error=str(exc), + ) + return + + try: + async with get_db_context() as db: + repo = IndexedDocumentRepository(db) + await repo.delete_by_source( + IndexType.PLAYBOOKS.value, + f"roboco://playbooks/{playbook_id}", + ) + except Exception as exc: + logger.warning( + "Playbook de-index (tracking row) failed; continuing", + playbook_id=playbook_id, + error=str(exc), + ) + # ========================================================================= # INDEXING OPERATIONS (New - Optimal Brain) # ========================================================================= diff --git a/roboco/services/optimal_brain/indexes/playbooks.py b/roboco/services/optimal_brain/indexes/playbooks.py index 3f9a7374..9ffad574 100644 --- a/roboco/services/optimal_brain/indexes/playbooks.py +++ b/roboco/services/optimal_brain/indexes/playbooks.py @@ -70,6 +70,19 @@ class PlaybooksIndexPlugin(BaseIndexPlugin): tags=params.tags, ) + async def delete_playbook(self, playbook_id: str) -> None: + """Remove a playbook's embedded chunks from the vector store. + + Used when a playbook is rejected/archived after it was approved+indexed, + so it stops surfacing in agent briefings as a stale, no-longer-canonical + procedure. Idempotent: the store's ``delete_by_source`` no-ops when no + chunks match the source URI. + """ + source = self.build_source_uri(doc_id=playbook_id) + if not source: + return + await self._require_store.delete_by_source(source) + async def search_playbooks( self, query: str, diff --git a/roboco/services/playbook.py b/roboco/services/playbook.py index a98422b9..b402aa92 100644 --- a/roboco/services/playbook.py +++ b/roboco/services/playbook.py @@ -120,9 +120,33 @@ class PlaybookService(BaseService): playbook.approved_by = approver_id playbook.approved_at = datetime.now(UTC) await self.session.flush() + await self._unindex_playbook(playbook) self.log.info("Playbook rejected", playbook_id=str(playbook_id), reason=reason) return playbook + async def _unindex_playbook(self, playbook: PlaybookTable) -> None: + """De-index a playbook from the PLAYBOOKS RAG index (best-effort). + + The mirror of :meth:`_index_approved`: a rejected/archived playbook that + was previously approved+indexed must stop surfacing in agent briefings, + so ``reject`` drops its chunks + tracking row. Gated on + ``org_memory_enabled`` (inert when the loop is off) and best-effort (a + failure never blocks the curation). Idempotent on a never-indexed draft. + """ + if not settings.org_memory_enabled: + return + try: + from roboco.services.optimal import get_optimal_service + + optimal = await get_optimal_service() + await optimal.unindex_playbook(str(playbook.id)) + except Exception as exc: + self.log.warning( + "Playbook de-index-on-reject failed (best-effort)", + playbook_id=str(playbook.id), + error=str(exc), + ) + async def list_drafts(self) -> list[PlaybookTable]: return await self._list_by_status(PlaybookStatus.DRAFT) diff --git a/roboco/services/repositories/indexed_document.py b/roboco/services/repositories/indexed_document.py index 25ab615b..d003fe6d 100644 --- a/roboco/services/repositories/indexed_document.py +++ b/roboco/services/repositories/indexed_document.py @@ -7,7 +7,7 @@ Repository for managing indexed documents in the knowledge base. import hashlib from typing import Any -from sqlalchemy import select +from sqlalchemy import delete, select from roboco.db.tables import IndexedDocumentTable from roboco.services.repositories.base import BaseRepository @@ -100,6 +100,25 @@ class IndexedDocumentRepository(BaseRepository[IndexedDocumentTable]): """Count documents in an index type.""" return await self.count(IndexedDocumentTable.index_type == index_type) + async def delete_by_source(self, index_type: str, source: str) -> bool: + """Delete the indexed-document tracking row for one source URI. + + Used by de-index paths (e.g. a rejected/archived playbook) to drop the + tracking row whose chunks the vector store has already removed by the + same source. Idempotent: returns ``False`` (nothing deleted) when no + tracking row exists, ``True`` when one was removed. + """ + source_hash = hashlib.sha256(source.encode()).hexdigest() + result = await self.session.execute( + delete(IndexedDocumentTable).where( + IndexedDocumentTable.index_type == index_type, + IndexedDocumentTable.source_hash == source_hash, + ) + ) + await self.session.flush() + rowcount: int = getattr(result, "rowcount", 0) or 0 + return rowcount > 0 + async def delete_by_index_type(self, index_type: str) -> int: """Delete all documents for an index type.""" docs = await self.get_by_index_type(index_type, limit=10000) diff --git a/tests/unit/services/optimal_brain/test_playbooks_index.py b/tests/unit/services/optimal_brain/test_playbooks_index.py index b1aa713a..fbe8e6c0 100644 --- a/tests/unit/services/optimal_brain/test_playbooks_index.py +++ b/tests/unit/services/optimal_brain/test_playbooks_index.py @@ -38,3 +38,24 @@ def test_build_source_uri_with_id() -> None: def test_build_source_uri_none_when_missing() -> None: assert _plugin().build_source_uri(doc_id=None) is None + + +def test_delete_playbook_removes_its_chunks_by_source() -> None: + """F011: deleting a playbook removes its embedded chunks from the vector + store by the playbook's source URI (idempotent — no-op if absent). A + rejected/archived playbook must not stay retrievable in the PLAYBOOKS index.""" + from unittest.mock import AsyncMock, MagicMock + + plugin = PlaybooksIndexPlugin.__new__(PlaybooksIndexPlugin) + store = MagicMock() + store.delete_by_source = AsyncMock(return_value=None) + # Bypass the initialized-guard property so the unit test doesn't need a + # live pgvector store. + object.__setattr__(plugin, "_initialized", True) + object.__setattr__(plugin, "_store", store) + + import asyncio + + asyncio.run(plugin.delete_playbook("pb-42")) + + store.delete_by_source.assert_awaited_once_with("roboco://playbooks/pb-42") diff --git a/tests/unit/services/test_playbook_unindex.py b/tests/unit/services/test_playbook_unindex.py new file mode 100644 index 00000000..697697fd --- /dev/null +++ b/tests/unit/services/test_playbook_unindex.py @@ -0,0 +1,128 @@ +"""F011 — PlaybookService.reject must de-index the playbook. + +A rejected/archived playbook that was previously approved stays in the +PLAYBOOKS RAG index (no de-index on reject/archive), so it keeps surfacing +in agent briefings as a stale, no-longer-canonical procedure. The fix +mirrors the index-on-approve path: ``reject`` calls an +``_unindex_playbook`` helper (gated on ``org_memory_enabled``, best-effort) +that removes the playbook's chunks + tracking row. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +import pytest +from roboco.config import settings +from roboco.models.base import PlaybookStatus +from roboco.services.playbook import PlaybookService + + +def _mock_playbook(playbook_id, *, status=PlaybookStatus.APPROVED.value): + pb = MagicMock() + pb.id = playbook_id + pb.status = status + pb.title = "Retry a flaky pg test" + pb.problem = "..." + pb.procedure = "..." + pb.tags = ["backend"] + pb.team = "backend" + pb.scope = "org" + return pb + + +@pytest.mark.asyncio +async def test_reject_deindexes_approved_playbook( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Rejecting an approved (indexed) playbook removes it from the index.""" + monkeypatch.setattr(settings, "org_memory_enabled", True) + playbook_id = uuid4() + pb = _mock_playbook(playbook_id, status=PlaybookStatus.APPROVED.value) + + session = MagicMock() + # _get_or_raise returns the playbook; flush is a no-op. + result = MagicMock() + result.scalar_one_or_none.return_value = pb + session.execute = AsyncMock(return_value=result) + session.flush = AsyncMock() + + optimal = MagicMock() + optimal.unindex_playbook = AsyncMock(return_value=None) + + svc = PlaybookService(session) + with ( + patch( + "roboco.services.optimal.get_optimal_service", + AsyncMock(return_value=optimal), + ), + ): + out = await svc.reject(playbook_id, approver_id=uuid4(), reason="stale") + + assert out.status == PlaybookStatus.ARCHIVED.value + optimal.unindex_playbook.assert_awaited_once_with(str(playbook_id)) + + +@pytest.mark.asyncio +async def test_reject_of_unindexed_draft_does_not_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A draft (never indexed) reject still de-indexes (idempotent no-op) and + never errors the curation.""" + monkeypatch.setattr(settings, "org_memory_enabled", True) + playbook_id = uuid4() + pb = _mock_playbook(playbook_id, status=PlaybookStatus.DRAFT.value) + + session = MagicMock() + result = MagicMock() + result.scalar_one_or_none.return_value = pb + session.execute = AsyncMock(return_value=result) + session.flush = AsyncMock() + + optimal = MagicMock() + optimal.unindex_playbook = AsyncMock(return_value=None) + + svc = PlaybookService(session) + with ( + patch( + "roboco.services.optimal.get_optimal_service", + AsyncMock(return_value=optimal), + ), + ): + out = await svc.reject(playbook_id, approver_id=uuid4(), reason="nope") + + assert out.status == PlaybookStatus.ARCHIVED.value + # De-index is still called (idempotent — the store no-ops on an absent + # source), so a draft reject mirrors the approved reject path uniformly. + optimal.unindex_playbook.assert_awaited_once_with(str(playbook_id)) + + +@pytest.mark.asyncio +async def test_reject_skips_deindex_when_org_memory_disabled( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """org_memory_enabled=False ⇒ the index is inert; reject must not touch it.""" + monkeypatch.setattr(settings, "org_memory_enabled", False) + playbook_id = uuid4() + pb = _mock_playbook(playbook_id) + + session = MagicMock() + result = MagicMock() + result.scalar_one_or_none.return_value = pb + session.execute = AsyncMock(return_value=result) + session.flush = AsyncMock() + + optimal = MagicMock() + optimal.unindex_playbook = AsyncMock(return_value=None) + + svc = PlaybookService(session) + with ( + patch( + "roboco.services.optimal.get_optimal_service", + AsyncMock(return_value=optimal), + ), + ): + await svc.reject(playbook_id, approver_id=uuid4(), reason="nope") + + optimal.unindex_playbook.assert_not_awaited()