[F109] playbook curation status guards: approve/reject draft-only, archive approved-only

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)

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

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)

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

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())

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

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"

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()