[F057] playbook: index/unindex as a post-commit step so the RAG corpus never leads the status transaction

This commit is contained in:
Renn F
2026-06-28 16:08:45 +02:00
parent adff12a85b
commit bffb0b7200
6 changed files with 428 additions and 64 deletions
+10 -4
View File
@@ -53,14 +53,17 @@ async def approve_playbook(
"""Approve a draft playbook → approved (and indexed into the KB)."""
_require_curator(agent)
try:
playbook = await get_playbook_service(db).approve(
playbook_id, approver_id=agent.agent_id
)
svc = get_playbook_service(db)
playbook = await svc.approve(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
# 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.
await db.commit()
await svc.index_approved(playbook)
return Playbook.model_validate(playbook)
@@ -74,12 +77,15 @@ async def reject_playbook(
"""Reject a playbook → archived, with the Auditor's reason."""
_require_curator(agent)
try:
playbook = await get_playbook_service(db).reject(
svc = get_playbook_service(db)
playbook = await svc.reject(
playbook_id, approver_id=agent.agent_id, reason=body.reason
)
except NotFoundError as exc:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND, detail="Playbook not found"
) 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)
@@ -793,6 +793,18 @@ class ContentActions:
status = "playbook_archived"
except NotFoundError:
return Envelope.not_found(message=f"playbook {playbook_id} not found")
# 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
# if this transaction rolled back — a divergence agents surface in
# briefings. ``get_db`` commits the session again after the route returns
# (a no-op on the now-clean transaction); this explicit commit is what
# gates the index.
await self.task.session.commit()
if action == "approve":
await svc.index_approved(playbook)
else:
await svc.unindex_playbook(playbook)
return Envelope.ok(
status=status,
task_id=None,
+20 -8
View File
@@ -68,21 +68,30 @@ class PlaybookService(BaseService):
return playbook
async def approve(self, playbook_id: UUID, approver_id: UUID) -> PlaybookTable:
"""Auditor approves a draft: draft -> approved, stamped."""
"""Auditor approves a draft: draft -> approved, stamped.
Flushes the status change ONLY the RAG index write (``index_approved``)
is a SEPARATE step the caller runs AFTER committing the status. The vector
store writes through its own auto-committing pool connection, so indexing
inline (before the status commit) would durably land an approved playbook
in the corpus even if the status transaction rolled back a divergence
agents then surfaced in briefings.
"""
playbook = await self._get_or_raise(playbook_id)
playbook.status = PlaybookStatus.APPROVED.value
playbook.approved_by = approver_id
playbook.approved_at = datetime.now(UTC)
await self.session.flush()
await self._index_approved(playbook)
self.log.info("Playbook approved", playbook_id=str(playbook_id))
return playbook
async def _index_approved(self, playbook: PlaybookTable) -> None:
async def index_approved(self, playbook: PlaybookTable) -> None:
"""Embed an approved playbook into the PLAYBOOKS RAG index (best-effort).
Gated on ``org_memory_enabled`` so the feature is fully inert when off;
a failure (e.g. the embedder is down) never blocks the approval.
Post-commit step: the caller commits the ``draft -> approved`` status
change FIRST, then runs this so the index never leads the status
transaction. Gated on ``org_memory_enabled`` so the feature is fully inert
when off; a failure (e.g. the embedder is down) never blocks the approval.
"""
if not settings.org_memory_enabled:
return
@@ -114,17 +123,20 @@ class PlaybookService(BaseService):
async def reject(
self, playbook_id: UUID, approver_id: UUID, reason: str
) -> PlaybookTable:
"""Auditor rejects a playbook: -> archived (reason recorded in the log)."""
"""Auditor rejects a playbook: -> archived (reason recorded in the log).
Flushes the status change ONLY ``unindex_playbook`` is a separate
post-commit step (see ``approve`` for the ordering rationale).
"""
playbook = await self._get_or_raise(playbook_id)
playbook.status = PlaybookStatus.ARCHIVED.value
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:
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
+14 -5
View File
@@ -48,7 +48,7 @@ def _actions(role: str) -> ContentActions:
agent = MagicMock()
agent.role = role
task.agent_for = AsyncMock(return_value=agent)
task.session = MagicMock()
task.session = AsyncMock()
deps = ContentActionsDeps(
task=task,
git=MagicMock(),
@@ -107,13 +107,17 @@ async def test_approve_playbook_for_auditor(monkeypatch: pytest.MonkeyPatch) ->
approved.status = "approved"
svc = MagicMock()
svc.approve = AsyncMock(return_value=approved)
svc.index_approved = AsyncMock()
monkeypatch.setattr("roboco.services.playbook.get_playbook_service", lambda _s: svc)
env = await _actions("auditor").approve_playbook(
agent_id=uuid4(), playbook_id=uuid4()
)
actions = _actions("auditor")
env = await actions.approve_playbook(agent_id=uuid4(), playbook_id=uuid4())
assert env.error is None
assert env.status == "playbook_approved"
svc.approve.assert_awaited_once()
# F057: the status commit gates the index — commit then index, never index
# before commit (the index write auto-commits on its own connection).
actions.task.session.commit.assert_awaited_once()
svc.index_approved.assert_awaited_once_with(approved)
@pytest.mark.asyncio
@@ -125,9 +129,14 @@ async def test_reject_playbook_archives_for_auditor(
archived.status = "archived"
svc = MagicMock()
svc.reject = AsyncMock(return_value=archived)
svc.unindex_playbook = AsyncMock()
monkeypatch.setattr("roboco.services.playbook.get_playbook_service", lambda _s: svc)
env = await _actions("auditor").reject_playbook(
actions = _actions("auditor")
env = await actions.reject_playbook(
agent_id=uuid4(), playbook_id=uuid4(), reason="duplicate"
)
assert env.status == "playbook_archived"
svc.reject.assert_awaited_once()
# 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)
@@ -0,0 +1,335 @@
"""F057: the PLAYBOOKS RAG index write must not commit independently of — and
BEFORE the playbook status transaction.
``approve()`` / ``reject()`` used to call ``_index_approved`` / ``_unindex_playbook``
inline, AFTER ``flush()`` but BEFORE the caller's ``commit()``. The vector store
writes chunks via its OWN pool connection (vector_store.py:211-237), which
auto-commits immediately and independently of the SQLAlchemy session
transaction. So a status-commit failure (or a crash between the index write and
the commit) left the RAG corpus with an approved/archived playbook whose DB row
was still DRAFT/APPROVED a divergence agents then surfaced in briefings.
The fix: ``approve()`` / ``reject()`` flush the status ONLY; the index/unindex
is a separate post-commit step (``index_approved`` / ``unindex_playbook``) the
caller runs AFTER the status transaction commits. Both entry points the panel
route (playbooks.py) and the Auditor gateway verb (content_actions.
_curate_playbook) commit-then-index, and skip the index if the commit fails.
"""
from __future__ import annotations
from typing import Any
from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4
import pytest
from roboco.api.routes.playbooks import approve_playbook, reject_playbook
from roboco.config import settings
from roboco.models import AgentRole
from roboco.models.permissions import AgentContext
from roboco.services.gateway.content_actions import ContentActions, ContentActionsDeps
from roboco.services.playbook import PlaybookService
_PID = uuid4()
_APPROVER = uuid4()
def _playbook_mock() -> Any:
pb = MagicMock(name="playbook")
pb.id = _PID
pb.status = "approved"
pb.title = "T"
pb.problem = "P"
pb.procedure = "Pr"
pb.tags = []
pb.team = "backend"
pb.scope = "team"
return pb
# ---------------------------------------------------------------------------
# Service-level: approve/reject flush ONLY; index/unindex are separate steps
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_approve_does_not_index_before_commit(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""``approve()`` flushes the status change but must NOT write to the RAG
index that writes through its own auto-committing connection, so it would
durably land before the caller commits the status (the F057 divergence)."""
monkeypatch.setattr(settings, "org_memory_enabled", True)
session = AsyncMock()
session.flush = AsyncMock()
svc = PlaybookService(session)
svc._get_or_raise = AsyncMock(return_value=_playbook_mock()) # type: ignore[assignment]
optimal = MagicMock()
optimal.index_playbook = AsyncMock()
monkeypatch.setattr(
"roboco.services.optimal.get_optimal_service",
AsyncMock(return_value=optimal),
)
await svc.approve(_PID, approver_id=_APPROVER)
optimal.index_playbook.assert_not_awaited()
@pytest.mark.asyncio
async def test_index_approved_is_a_separate_post_commit_step(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""``index_approved`` is the public post-commit index step the caller runs
AFTER committing the status it invokes the optimal index."""
monkeypatch.setattr(settings, "org_memory_enabled", True)
svc = PlaybookService(AsyncMock())
optimal = MagicMock()
optimal.index_playbook = AsyncMock()
monkeypatch.setattr(
"roboco.services.optimal.get_optimal_service",
AsyncMock(return_value=optimal),
)
await svc.index_approved(_playbook_mock())
optimal.index_playbook.assert_awaited_once()
@pytest.mark.asyncio
async def test_reject_does_not_unindex_before_commit(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""``reject()`` flushes the status change but must NOT de-index from the RAG
index inline same ordering gap as approve (the de-index auto-commits before
the status commit, dropping an approved playbook from briefings while its row
is still APPROVED on rollback)."""
monkeypatch.setattr(settings, "org_memory_enabled", True)
session = AsyncMock()
session.flush = AsyncMock()
svc = PlaybookService(session)
svc._get_or_raise = AsyncMock(return_value=_playbook_mock()) # type: ignore[assignment]
optimal = MagicMock()
optimal.unindex_playbook = AsyncMock()
optimal.deindex_playbook = AsyncMock()
monkeypatch.setattr(
"roboco.services.optimal.get_optimal_service",
AsyncMock(return_value=optimal),
)
await svc.reject(_PID, approver_id=_APPROVER, reason="nope")
# No de-index call of any name originated from reject() before commit.
optimal.unindex_playbook.assert_not_awaited()
optimal.deindex_playbook.assert_not_awaited()
@pytest.mark.asyncio
async def test_unindex_playbook_is_a_separate_post_commit_step(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""``unindex_playbook`` is the public post-commit de-index step."""
monkeypatch.setattr(settings, "org_memory_enabled", True)
svc = PlaybookService(AsyncMock())
optimal = MagicMock()
optimal.unindex_playbook = AsyncMock()
monkeypatch.setattr(
"roboco.services.optimal.get_optimal_service",
AsyncMock(return_value=optimal),
)
await svc.unindex_playbook(_playbook_mock())
optimal.unindex_playbook.assert_awaited_once()
# ---------------------------------------------------------------------------
# Route-level (panel): commit-then-index, skip index on commit failure
# ---------------------------------------------------------------------------
def _route_service_mock(
order: list[str], *, commit_fails: bool = False
) -> tuple[Any, Any]:
def _approve(*_a: Any, **_k: Any) -> Any:
order.append("approve")
return _playbook_mock()
def _reject(*_a: Any, **_k: Any) -> Any:
order.append("reject")
return _playbook_mock()
svc = MagicMock()
svc.approve = AsyncMock(side_effect=_approve)
svc.reject = AsyncMock(side_effect=_reject)
svc.index_approved = AsyncMock(side_effect=lambda *_a, **_k: order.append("index"))
svc.unindex_playbook = AsyncMock(
side_effect=lambda *_a, **_k: order.append("unindex")
)
db = AsyncMock()
if commit_fails:
db.commit = AsyncMock(side_effect=RuntimeError("commit failed"))
else:
db.commit = AsyncMock(side_effect=lambda: order.append("commit"))
return svc, db
@pytest.mark.asyncio
async def test_approve_route_commits_before_indexing(
monkeypatch: pytest.MonkeyPatch,
) -> None:
order: list[str] = []
svc, db = _route_service_mock(order)
monkeypatch.setattr(
"roboco.api.routes.playbooks.get_playbook_service", lambda _db: svc
)
monkeypatch.setattr(
"roboco.api.routes.playbooks.Playbook.model_validate", lambda obj: obj
)
agent = AgentContext(agent_id=_APPROVER, role=AgentRole.AUDITOR)
await approve_playbook(_PID, db, agent)
assert order == ["approve", "commit", "index"]
@pytest.mark.asyncio
async def test_approve_route_skips_index_when_commit_fails(
monkeypatch: pytest.MonkeyPatch,
) -> None:
order: list[str] = []
svc, db = _route_service_mock(order, commit_fails=True)
monkeypatch.setattr(
"roboco.api.routes.playbooks.get_playbook_service", lambda _db: svc
)
agent = AgentContext(agent_id=_APPROVER, role=AgentRole.AUDITOR)
with pytest.raises(RuntimeError):
await approve_playbook(_PID, db, agent)
svc.index_approved.assert_not_awaited()
assert order == ["approve"]
@pytest.mark.asyncio
async def test_reject_route_commits_before_unindexing(
monkeypatch: pytest.MonkeyPatch,
) -> None:
order: list[str] = []
svc, db = _route_service_mock(order)
monkeypatch.setattr(
"roboco.api.routes.playbooks.get_playbook_service", lambda _db: svc
)
monkeypatch.setattr(
"roboco.api.routes.playbooks.Playbook.model_validate", lambda obj: obj
)
agent = AgentContext(agent_id=_APPROVER, role=AgentRole.AUDITOR)
await reject_playbook(_PID, MagicMock(reason="nope"), db, agent)
assert order == ["reject", "commit", "unindex"]
# ---------------------------------------------------------------------------
# Gateway-level (Auditor verb): commit-then-index, skip index on commit failure
# ---------------------------------------------------------------------------
def _gateway_actions(
order: list[str], *, commit_fails: bool = False
) -> tuple[ContentActions, MagicMock]:
task = MagicMock()
task.session = AsyncMock()
if commit_fails:
task.session.commit = AsyncMock(side_effect=RuntimeError("commit failed"))
else:
task.session.commit = AsyncMock(side_effect=lambda: order.append("commit"))
task.agent_for = AsyncMock(return_value=MagicMock(role="auditor"))
deps = ContentActionsDeps(
task=task,
git=MagicMock(),
messaging=MagicMock(),
a2a=MagicMock(),
journal=MagicMock(),
workspace=MagicMock(),
notifications=MagicMock(),
)
return ContentActions(deps), task
def _gateway_svc_mock(order: list[str]) -> MagicMock:
def _approve(*_a: Any, **_k: Any) -> Any:
order.append("approve")
return _playbook_mock()
def _reject(*_a: Any, **_k: Any) -> Any:
order.append("reject")
return _playbook_mock()
svc = MagicMock()
svc.approve = AsyncMock(side_effect=_approve)
svc.reject = AsyncMock(side_effect=_reject)
svc.index_approved = AsyncMock(side_effect=lambda *_a, **_k: order.append("index"))
svc.unindex_playbook = AsyncMock(
side_effect=lambda *_a, **_k: order.append("unindex")
)
return svc
@pytest.mark.asyncio
async def test_gateway_approve_commits_before_indexing(
monkeypatch: pytest.MonkeyPatch,
) -> None:
order: list[str] = []
actions, _task = _gateway_actions(order)
svc = _gateway_svc_mock(order)
monkeypatch.setattr(
"roboco.services.playbook.get_playbook_service",
lambda _session: svc,
)
env = await actions.approve_playbook(agent_id=_APPROVER, playbook_id=_PID)
assert order == ["approve", "commit", "index"]
assert env.status == "playbook_approved"
@pytest.mark.asyncio
async def test_gateway_approve_skips_index_when_commit_fails(
monkeypatch: pytest.MonkeyPatch,
) -> None:
order: list[str] = []
actions, _task = _gateway_actions(order, commit_fails=True)
svc = _gateway_svc_mock(order)
monkeypatch.setattr(
"roboco.services.playbook.get_playbook_service",
lambda _session: svc,
)
with pytest.raises(RuntimeError):
await actions.approve_playbook(agent_id=_APPROVER, playbook_id=_PID)
svc.index_approved.assert_not_awaited()
assert order == ["approve"]
@pytest.mark.asyncio
async def test_gateway_reject_commits_before_unindexing(
monkeypatch: pytest.MonkeyPatch,
) -> None:
order: list[str] = []
actions, _task = _gateway_actions(order)
svc = _gateway_svc_mock(order)
monkeypatch.setattr(
"roboco.services.playbook.get_playbook_service",
lambda _session: svc,
)
env = await actions.reject_playbook(
agent_id=_APPROVER, playbook_id=_PID, reason="nope"
)
assert order == ["reject", "commit", "unindex"]
assert env.status == "playbook_archived"
+37 -47
View File
@@ -1,11 +1,15 @@
"""F011 — PlaybookService.reject must de-index the playbook.
"""PlaybookService.reject must flush the status change WITHOUT de-indexing
inline, and ``unindex_playbook`` is the separate post-commit de-index step.
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.
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
``org_memory_enabled`` (inert when the loop is off) and best-effort.
"""
from __future__ import annotations
@@ -32,26 +36,28 @@ def _mock_playbook(playbook_id, *, status=PlaybookStatus.APPROVED.value):
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)
def _session_with(pb) -> MagicMock:
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()
return session
@pytest.mark.asyncio
async def test_reject_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)."""
monkeypatch.setattr(settings, "org_memory_enabled", True)
playbook_id = uuid4()
pb = _mock_playbook(playbook_id, status=PlaybookStatus.APPROVED.value)
svc = PlaybookService(_session_with(pb))
optimal = MagicMock()
optimal.unindex_playbook = AsyncMock(return_value=None)
svc = PlaybookService(session)
with (
patch(
"roboco.services.optimal.get_optimal_service",
@@ -61,68 +67,52 @@ async def test_reject_deindexes_approved_playbook(
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))
optimal.unindex_playbook.assert_not_awaited()
@pytest.mark.asyncio
async def test_reject_of_unindexed_draft_does_not_error(
async def test_unindex_playbook_deindexes_approved_playbook(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""A draft (never indexed) reject still de-indexes (idempotent no-op) and
never errors the curation."""
"""``unindex_playbook`` (the post-commit step) removes an approved playbook
from the index."""
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()
pb = _mock_playbook(playbook_id, status=PlaybookStatus.APPROVED.value)
svc = PlaybookService(_session_with(pb))
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")
await svc.unindex_playbook(pb)
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(
async def test_unindex_playbook_skips_when_org_memory_disabled(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""org_memory_enabled=False ⇒ the index is inert; reject must not touch it."""
"""org_memory_enabled=False ⇒ the index is inert; ``unindex_playbook`` 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()
svc = PlaybookService(_session_with(pb))
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")
await svc.unindex_playbook(pb)
optimal.unindex_playbook.assert_not_awaited()