diff --git a/roboco/api/routes/playbooks.py b/roboco/api/routes/playbooks.py index 50cfe78e..7ae38d1f 100644 --- a/roboco/api/routes/playbooks.py +++ b/roboco/api/routes/playbooks.py @@ -13,7 +13,7 @@ from roboco.api.deps import CurrentAgentContext, DbSession from roboco.api.schemas.playbook import PlaybookRejectBody from roboco.models import AgentRole from roboco.models.playbook import Playbook -from roboco.services.base import NotFoundError +from roboco.services.base import ConflictError, NotFoundError from roboco.services.playbook import get_playbook_service router = APIRouter() @@ -59,6 +59,12 @@ async def approve_playbook( raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="Playbook not found" ) from exc + except ConflictError as exc: + # The playbook is not a draft (already approved/archived) — the + # curation is already finished, not a missing resource. + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, detail=str(exc) + ) from exc # Commit the status change BEFORE indexing: the RAG index write runs through # its own auto-committing connection, so indexing before commit would durably # land an approved playbook in the corpus even if this commit rolled back. @@ -74,7 +80,7 @@ async def reject_playbook( db: DbSession, agent: CurrentAgentContext, ) -> Playbook: - """Reject a playbook → archived, with the Auditor's reason.""" + """Reject a draft playbook → archived, with the Auditor's reason.""" _require_curator(agent) try: svc = get_playbook_service(db) @@ -85,6 +91,33 @@ async def reject_playbook( raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="Playbook not found" ) from exc + except ConflictError as exc: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, detail=str(exc) + ) from exc + # Commit the status change BEFORE de-indexing (see approve_playbook). + await db.commit() + await svc.unindex_playbook(playbook) + return Playbook.model_validate(playbook) + + +@router.post("/{playbook_id}/archive", response_model=Playbook) +async def archive_playbook( + playbook_id: UUID, db: DbSession, agent: CurrentAgentContext +) -> Playbook: + """Retire an approved playbook → archived (and de-indexed from the KB).""" + _require_curator(agent) + try: + svc = get_playbook_service(db) + playbook = await svc.archive(playbook_id, approver_id=agent.agent_id) + except NotFoundError as exc: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail="Playbook not found" + ) from exc + except ConflictError as exc: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, detail=str(exc) + ) from exc # Commit the status change BEFORE de-indexing (see approve_playbook). await db.commit() await svc.unindex_playbook(playbook) diff --git a/roboco/services/gateway/content_actions.py b/roboco/services/gateway/content_actions.py index cdc40456..675baf8a 100644 --- a/roboco/services/gateway/content_actions.py +++ b/roboco/services/gateway/content_actions.py @@ -777,12 +777,11 @@ class ContentActions: ) async def archive_playbook(self, *, agent_id: UUID, playbook_id: UUID) -> Envelope: - """Auditor archives a playbook (-> archived).""" + """Auditor archives an approved playbook (-> archived, retired).""" return await self._curate_playbook( agent_id=agent_id, playbook_id=playbook_id, - action="reject", - reason="archived", + action="archive", ) async def _curate_playbook( @@ -801,7 +800,7 @@ class ContentActions: remediate="Only the Auditor approves/rejects/archives playbooks.", context_briefing={}, ) - from roboco.services.base import NotFoundError + from roboco.services.base import ConflictError, NotFoundError from roboco.services.playbook import get_playbook_service svc = get_playbook_service(self.task.session) @@ -809,6 +808,9 @@ class ContentActions: if action == "approve": playbook = await svc.approve(playbook_id, approver_id=agent_id) status = "playbook_approved" + elif action == "archive": + playbook = await svc.archive(playbook_id, approver_id=agent_id) + status = "playbook_archived" else: playbook = await svc.reject( playbook_id, approver_id=agent_id, reason=reason or action @@ -816,6 +818,20 @@ class ContentActions: status = "playbook_archived" except NotFoundError: return Envelope.not_found(message=f"playbook {playbook_id} not found") + except ConflictError as exc: + # A status-precondition violation (approve/reject on a non-draft, + # archive on a non-approved) is a clean invalid_state, not a 500 — + # the agent gets a remediate hint to re-fetch the playbook's + # current status before re-trying. + return Envelope.invalid_state( + message=str(exc), + remediate=( + "Only a draft can be approved/rejected; only an approved " + "playbook can be archived. Re-list drafts/approved to see " + "the playbook's current status before re-trying." + ), + context_briefing={"playbook_id": str(playbook_id)}, + ) # Commit the status change BEFORE touching the RAG index: the index write # runs through its own auto-committing connection, so indexing before the # status commit would durably land (or drop) a playbook in the corpus even diff --git a/roboco/services/playbook.py b/roboco/services/playbook.py index 4c472fba..59460f92 100644 --- a/roboco/services/playbook.py +++ b/roboco/services/playbook.py @@ -78,6 +78,12 @@ class PlaybookService(BaseService): agents then surfaced in briefings. """ playbook = await self._get_or_raise(playbook_id) + if playbook.status != PlaybookStatus.DRAFT.value: + raise ConflictError( + f"Playbook {playbook_id} is {playbook.status}, not draft — " + "only a draft can be approved", + resource_type="playbook", + ) playbook.status = PlaybookStatus.APPROVED.value playbook.approved_by = approver_id playbook.approved_at = datetime.now(UTC) @@ -85,6 +91,31 @@ class PlaybookService(BaseService): self.log.info("Playbook approved", playbook_id=str(playbook_id)) return playbook + async def archive(self, playbook_id: UUID, approver_id: UUID) -> PlaybookTable: + """Auditor retires an APPROVED playbook: approved -> archived. + + The distinct curation transition from :meth:`reject`: ``reject`` + declines a DRAFT (never published); ``archive`` retires an APPROVED + playbook already in circulation. Both end in ARCHIVED, but they start + from different states, so each guards its own precondition. An ARCHIVED + playbook is terminal — neither approve, reject, nor archive may touch + it again. Like reject, the status flush is the only in-tx step; the + post-commit ``unindex_playbook`` is the caller's separate step. + """ + playbook = await self._get_or_raise(playbook_id) + if playbook.status != PlaybookStatus.APPROVED.value: + raise ConflictError( + f"Playbook {playbook_id} is {playbook.status}, not approved — " + "only an approved playbook can be archived", + resource_type="playbook", + ) + playbook.status = PlaybookStatus.ARCHIVED.value + playbook.approved_by = approver_id + playbook.approved_at = datetime.now(UTC) + await self.session.flush() + self.log.info("Playbook archived", playbook_id=str(playbook_id)) + return playbook + async def index_approved(self, playbook: PlaybookTable) -> None: """Embed an approved playbook into the PLAYBOOKS RAG index (best-effort). @@ -129,6 +160,12 @@ class PlaybookService(BaseService): post-commit step (see ``approve`` for the ordering rationale). """ playbook = await self._get_or_raise(playbook_id) + if playbook.status != PlaybookStatus.DRAFT.value: + raise ConflictError( + f"Playbook {playbook_id} is {playbook.status}, not draft — " + "only a draft can be rejected", + resource_type="playbook", + ) playbook.status = PlaybookStatus.ARCHIVED.value playbook.approved_by = approver_id playbook.approved_at = datetime.now(UTC) diff --git a/tests/integration/test_playbook_routes.py b/tests/integration/test_playbook_routes.py index a6dccac4..364277d5 100644 --- a/tests/integration/test_playbook_routes.py +++ b/tests/integration/test_playbook_routes.py @@ -110,3 +110,52 @@ async def test_non_curator_is_forbidden(db_session: AsyncSession) -> None: assert get_resp.status_code == HTTPStatus.FORBIDDEN assert approve_resp.status_code == HTTPStatus.FORBIDDEN app.dependency_overrides.clear() + + +# --- F109: curation status-precondition guards at the route layer ------------- # +# approve/reject only act on a draft; archive only retires an approved playbook. +# A violation is a 409 Conflict (the curation is already finished), not a 200 +# that silently no-ops or a 500 from an uncaught ConflictError. + + +@pytest.mark.asyncio +async def test_approve_already_approved_is_409( + db_session: AsyncSession, auditor_client: AsyncClient +) -> None: + pid = await _seed_draft(db_session, title="Once only") + first = await auditor_client.post(f"/api/playbooks/{pid}/approve") + assert first.status_code == HTTPStatus.OK + second = await auditor_client.post(f"/api/playbooks/{pid}/approve") + assert second.status_code == HTTPStatus.CONFLICT + + +@pytest.mark.asyncio +async def test_reject_approved_is_409( + db_session: AsyncSession, auditor_client: AsyncClient +) -> None: + pid = await _seed_draft(db_session, title="Published route") + await auditor_client.post(f"/api/playbooks/{pid}/approve") + resp = await auditor_client.post( + f"/api/playbooks/{pid}/reject", json={"reason": "changed my mind"} + ) + assert resp.status_code == HTTPStatus.CONFLICT + + +@pytest.mark.asyncio +async def test_archive_approved_succeeds( + db_session: AsyncSession, auditor_client: AsyncClient +) -> None: + pid = await _seed_draft(db_session, title="Retire route") + await auditor_client.post(f"/api/playbooks/{pid}/approve") + resp = await auditor_client.post(f"/api/playbooks/{pid}/archive") + assert resp.status_code == HTTPStatus.OK + assert resp.json()["status"] == "archived" + + +@pytest.mark.asyncio +async def test_archive_draft_is_409( + db_session: AsyncSession, auditor_client: AsyncClient +) -> None: + pid = await _seed_draft(db_session, title="Not yet approved") + resp = await auditor_client.post(f"/api/playbooks/{pid}/archive") + assert resp.status_code == HTTPStatus.CONFLICT diff --git a/tests/integration/test_playbook_service.py b/tests/integration/test_playbook_service.py index 28106ae6..0d5a24e2 100644 --- a/tests/integration/test_playbook_service.py +++ b/tests/integration/test_playbook_service.py @@ -111,7 +111,11 @@ async def test_approve_indexes_when_org_memory_on( ) svc = PlaybookService(db_session) pb = await svc.draft(_create(title="Index me"), created_by=uuid4()) - await svc.approve(pb.id, approver_id=uuid4()) + approved = await svc.approve(pb.id, approver_id=uuid4()) + # approve() only flushes the status; indexing is the separate post-commit + # step the route/verb runs after committing (so the index never leads the + # status transaction). Mirror that ordering here. + await svc.index_approved(approved) fake_optimal.index_playbook.assert_awaited_once() @@ -143,3 +147,77 @@ async def test_approve_survives_index_failure( pb = await svc.draft(_create(title="Resilient"), created_by=uuid4()) approved = await svc.approve(pb.id, approver_id=uuid4()) # must not raise assert approved.status == PlaybookStatus.APPROVED + + +# --- F109: approve/reject/archive status-precondition guards ---------------- # +# The lifecycle is draft -> approved | archived, both terminal. approve/reject +# only act on a DRAFT; archive only retires an APPROVED playbook. An archived +# playbook is terminal — none of the three may touch it again. Without these +# guards an archived playbook could be re-approved and an approved one rejected, +# silently undoing a finished curation. + + +@pytest.mark.asyncio +async def test_approve_rejects_already_approved(db_session: AsyncSession) -> None: + svc = PlaybookService(db_session) + pb = await svc.draft(_create(title="Once"), created_by=uuid4()) + await svc.approve(pb.id, approver_id=uuid4()) + with pytest.raises(ConflictError): + await svc.approve(pb.id, approver_id=uuid4()) + + +@pytest.mark.asyncio +async def test_approve_rejects_archived(db_session: AsyncSession) -> None: + svc = PlaybookService(db_session) + pb = await svc.draft(_create(title="Done"), created_by=uuid4()) + await svc.reject(pb.id, approver_id=uuid4(), reason="duplicate") + with pytest.raises(ConflictError): + await svc.approve(pb.id, approver_id=uuid4()) + + +@pytest.mark.asyncio +async def test_reject_rejects_already_approved(db_session: AsyncSession) -> None: + svc = PlaybookService(db_session) + pb = await svc.draft(_create(title="Published svc"), created_by=uuid4()) + await svc.approve(pb.id, approver_id=uuid4()) + with pytest.raises(ConflictError): + await svc.reject(pb.id, approver_id=uuid4(), reason="changed my mind") + + +@pytest.mark.asyncio +async def test_reject_rejects_archived(db_session: AsyncSession) -> None: + svc = PlaybookService(db_session) + pb = await svc.draft(_create(title="Closed"), created_by=uuid4()) + await svc.reject(pb.id, approver_id=uuid4(), reason="duplicate") + with pytest.raises(ConflictError): + await svc.reject(pb.id, approver_id=uuid4(), reason="again") + + +@pytest.mark.asyncio +async def test_archive_retires_approved(db_session: AsyncSession) -> None: + svc = PlaybookService(db_session) + auditor = uuid4() + pb = await svc.draft(_create(title="Retire svc"), created_by=uuid4()) + await svc.approve(pb.id, approver_id=auditor) + archived = await svc.archive(pb.id, approver_id=auditor) + assert archived.status == PlaybookStatus.ARCHIVED + assert archived.approved_by == auditor + assert archived.approved_at is not None + + +@pytest.mark.asyncio +async def test_archive_rejects_draft(db_session: AsyncSession) -> None: + svc = PlaybookService(db_session) + pb = await svc.draft(_create(title="Not yet"), created_by=uuid4()) + with pytest.raises(ConflictError): + await svc.archive(pb.id, approver_id=uuid4()) + + +@pytest.mark.asyncio +async def test_archive_rejects_already_archived(db_session: AsyncSession) -> None: + svc = PlaybookService(db_session) + pb = await svc.draft(_create(title="Twice"), created_by=uuid4()) + await svc.approve(pb.id, approver_id=uuid4()) + await svc.archive(pb.id, approver_id=uuid4()) + with pytest.raises(ConflictError): + await svc.archive(pb.id, approver_id=uuid4()) diff --git a/tests/unit/gateway/test_playbook_verbs.py b/tests/unit/gateway/test_playbook_verbs.py index 5e471d0f..d4f5d0d9 100644 --- a/tests/unit/gateway/test_playbook_verbs.py +++ b/tests/unit/gateway/test_playbook_verbs.py @@ -11,6 +11,7 @@ from unittest.mock import AsyncMock, MagicMock from uuid import uuid4 import pytest +from roboco.services.base import ConflictError from roboco.services.gateway.content_actions import ContentActions, ContentActionsDeps from roboco.services.gateway.role_config import get_role_config @@ -140,3 +141,45 @@ async def test_reject_playbook_archives_for_auditor( # F057: de-index is the post-commit step (commit gates it). actions.task.session.commit.assert_awaited_once() svc.unindex_playbook.assert_awaited_once_with(archived) + + +@pytest.mark.asyncio +async def test_archive_playbook_retires_approved_for_auditor( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """archive_playbook is the distinct APPROVED->archived retire path (F109): + it calls ``svc.archive`` (NOT ``svc.reject``), commits, then de-indexes.""" + archived = MagicMock() + archived.id = uuid4() + archived.status = "archived" + svc = MagicMock() + svc.archive = AsyncMock(return_value=archived) + svc.reject = AsyncMock() + svc.unindex_playbook = AsyncMock() + monkeypatch.setattr("roboco.services.playbook.get_playbook_service", lambda _s: svc) + actions = _actions("auditor") + env = await actions.archive_playbook(agent_id=uuid4(), playbook_id=uuid4()) + assert env.status == "playbook_archived" + svc.archive.assert_awaited_once() + svc.reject.assert_not_awaited() + actions.task.session.commit.assert_awaited_once() + svc.unindex_playbook.assert_awaited_once_with(archived) + + +@pytest.mark.asyncio +async def test_approve_playbook_invalid_state_envelope( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A status-precondition ConflictError from the service becomes a clean + invalid_state envelope (not a 500) — the agent gets a remediate hint to + re-fetch the playbook's current status before re-trying (F109).""" + svc = MagicMock() + svc.approve = AsyncMock( + side_effect=ConflictError("not draft", resource_type="playbook") + ) + monkeypatch.setattr("roboco.services.playbook.get_playbook_service", lambda _s: svc) + env = await _actions("auditor").approve_playbook( + agent_id=uuid4(), playbook_id=uuid4() + ) + assert env.error == "invalid_state" + assert env.remediate # the agent is told how to recover diff --git a/tests/unit/services/test_playbook_index_ordering.py b/tests/unit/services/test_playbook_index_ordering.py index c218d42b..6d0b1409 100644 --- a/tests/unit/services/test_playbook_index_ordering.py +++ b/tests/unit/services/test_playbook_index_ordering.py @@ -37,7 +37,10 @@ _APPROVER = uuid4() def _playbook_mock() -> Any: pb = MagicMock(name="playbook") pb.id = _PID - pb.status = "approved" + # approve/reject act on a DRAFT (their legitimate starting state — F109 + # guards both to DRAFT-only). The index/unindex ordering assertions below + # are independent of the starting status. + pb.status = "draft" pb.title = "T" pb.problem = "P" pb.procedure = "Pr" diff --git a/tests/unit/services/test_playbook_unindex.py b/tests/unit/services/test_playbook_unindex.py index 82f05400..c7236c54 100644 --- a/tests/unit/services/test_playbook_unindex.py +++ b/tests/unit/services/test_playbook_unindex.py @@ -1,14 +1,16 @@ -"""PlaybookService.reject must flush the status change WITHOUT de-indexing -inline, and ``unindex_playbook`` is the separate post-commit de-index step. +"""PlaybookService.reject/archive must flush the status change WITHOUT +de-indexing inline, and ``unindex_playbook`` is the separate post-commit step. A rejected/archived playbook that was previously approved must stop surfacing in -agent briefings, so a reject drops its chunks + tracking row. Originally (F011) -``reject`` called an ``_unindex_playbook`` helper inline. F057 split that out: -the RAG index write runs through its own auto-committing pool connection, so -de-indexing inline (before the caller commits the status) would drop a playbook -from the corpus even if the status transaction rolled back — a divergence. So -``reject`` now flushes the status ONLY; ``unindex_playbook`` is a separate -public step the caller runs AFTER committing. Both helpers stay gated on +agent briefings, so the curation drop its chunks + tracking row. Originally +(F011) ``reject`` called an ``_unindex_playbook`` helper inline. F057 split that +out: the RAG index write runs through its own auto-committing pool connection, +so de-indexing inline (before the caller commits the status) would drop a +playbook from the corpus even if the status transaction rolled back — a +divergence. So ``reject``/``archive`` now flush the status ONLY; +``unindex_playbook`` is a separate public step the caller runs AFTER +committing. F109 split the APPROVED->archived retire path into ``archive`` +(distinct from ``reject``, which declines a DRAFT). Both helpers stay gated on ``org_memory_enabled`` (inert when the loop is off) and best-effort. """ @@ -49,11 +51,13 @@ def _session_with(pb: Any) -> MagicMock: @pytest.mark.asyncio -async def test_reject_archives_but_does_not_deindex_inline( +async def test_archive_archives_but_does_not_deindex_inline( monkeypatch: pytest.MonkeyPatch, ) -> None: - """``reject`` flushes the ARCHIVED status but must NOT touch the RAG index — - the de-index is a separate post-commit step (F057 ordering).""" + """``archive`` flushes the ARCHIVED status but must NOT touch the RAG index — + the de-index is a separate post-commit step (F057 ordering). Archive retires + an APPROVED playbook (F109); the previously-approved one was indexed on + approval, so the post-commit ``unindex_playbook`` is what removes it.""" monkeypatch.setattr(settings, "org_memory_enabled", True) playbook_id = uuid4() pb = _mock_playbook(playbook_id, status=PlaybookStatus.APPROVED.value) @@ -67,7 +71,7 @@ async def test_reject_archives_but_does_not_deindex_inline( AsyncMock(return_value=optimal), ), ): - out = await svc.reject(playbook_id, approver_id=uuid4(), reason="stale") + out = await svc.archive(playbook_id, approver_id=uuid4()) assert out.status == PlaybookStatus.ARCHIVED.value optimal.unindex_playbook.assert_not_awaited()