mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Retire channels/sessions/messages; A2A becomes primary agent comms (#306)
* feat(a2a): deliver latest incoming message preview into the claim briefing
list_unread_a2a now carries last_message_preview (the latest message from the
OTHER agent, never the agent's own reply), fetched via a correlated subquery in
the same query — no N+1 on the per-verb briefing path.
* feat(a2a): read_a2a verb delivers unread message bodies to the agent
A2AService.get_unread_messages returns the caller's unread INCOMING messages
(never its own sends), marking exactly those rows read atomically so a message
arriving mid-call is preserved. Wired as the read_a2a content verb (route +
do_server tool + granted to every delivery role) — the content-bearing read the
A2A inbox lacked (read_messages only zeroed the counter).
* docs(rag): document read_a2a as the A2A content-read path
* fix(task): backlog activation no longer requires a discussion session
Removes the SessionTaskTable gate in activate() (and its dangling log field),
deletes _inherit_parent_session + its create() call, and drops the now-unused
SessionTaskTable import. Coordination rides task state; the session subsystem is
being retired. Tests updated to the new (no-session) behavior.
* fix(orchestrator): drop session sweep from _run_sweep
Removes the messaging import + sweep_timed_out_sessions call. That import sat
outside the try/except, so once messaging.py is deleted it would have killed the
entire sweep cascade (budget kill-switch, token rollups, retention, image prune,
superseded-PR reconcile). Notification sweep + all maintenance sweeps unchanged.
* release-manager --no-tags read-clone fix
* test: update evidence_repo unit test for a2a last_message_preview
* refactor(gateway): drop session propagation on delegate
Removes propagate_sessions_to_subtask from delegate(), the ChoreographerDeps
messaging field + property, and the ChoreographerDeps messaging arg in deps.py
(ContentActions messaging + import stay until the verbs are removed). Deletes the
propagation test; strips the now-invalid messaging kwarg from ChoreographerDeps
test builders.
* refactor(gateway): remove say/open_session/link_session/channels verbs
Removes the four channel/session verbs across content_actions (impls +
ContentActionsDeps.messaging), do_server (tools + registry), role_config (grants
+ _CHANNEL_DISCOVERY), do.py (routes), schemas/v1/do.py (request models), and
deps.py (MessagingService import + construction). Regenerates the prompt verb
tables. dm/notify/read_messages/read_a2a stay. Tests deleted/updated accordingly.
* uv.lock Upgrade
* refactor: remove conversation RAG indexing; Secretary announces via notification
Drops the CONVERSATIONS index (index_conversation, ConversationsIndexPlugin,
IndexType.CONVERSATIONS enum, IndexConversationParams, mentor.py type-label, the
messaging index hook) and its chunk-table manifest entries. The Secretary's
ANNOUNCE/RELAY_MESSAGE now fan out a BROADCAST notification to every agent's
inbox (NotificationService.broadcast) instead of posting to a dead channel.
* fix(panel): label RAG health error lines by subsystem
A red llm_error (e.g. the glm-5.2:cloud weekly-limit 429) rendered under
the 'Embedding: ok' header with no label, reading as an embedding failure.
Prefix each error line with LLM / Embedding / Vector store.
* refactor: remove channel/message reads from metrics, dashboard, git, events
MetricsService drops get_communication_volume + the MessageTable
message-count in get_agent_metrics (and the now-dead messages_sent_week
field). DashboardService drops get_channel_feeds/_compute_channel_status
and the message read in get_recent_activity (task activity kept);
get_auditor_metrics no longer reports communication_volume.
GitService's two primary-session-id helpers always return None now
(callers already treat None as "no primary session"). events/handlers.py
drops the SESSION_CLOSED/SESSION_TIMEOUT subscriptions + the
handle_session_boundary handler.
Forced follow-on: api/routes/dashboard.py + api/schemas/dashboard.py
dropped the now-dangling live_feeds/ChannelFeed surface and the
/metrics/communication route, which wrapped the removed service calls
directly (mypy would otherwise fail on the missing attributes).
* refactor: delete MessagingService + channel seeding
Edited db/__init__.py and services/__init__.py first (drop the unconditional
Channel/Group/Message/Session table + MessagingService re-exports), then
deleted services/messaging.py, then trimmed db/seed.py to only create_agents
(create_channels/create_channel_memberships/create_initial_messages gone).
Forced expansion: api/routes/{channels,groups,sessions,messages}.py import
roboco.services.messaging directly (not through the package __init__), as
does api/routes/tasks.py (the session-links embed on GET /tasks/{id} and the
GET /{id}/sessions route). Deleting messaging.py without addressing these
breaks `import roboco.api.app` immediately, since app.py eagerly imports all
route modules at startup. Since the 4 CRUD route files are 100%
MessagingService-backed with zero independent logic (and are wholesale
deletes in the plan's later API-routes task anyway), deleted them now +
unmounted from app.py/routes/__init__.py; tasks.py got the same surgical
trim its later task already specified (drop session-links embed +
TaskSessionLinkResponse/TaskResponse.sessions). This pulls a slice of that
later work forward — the routes/schemas for channels/groups/sessions/messages
still need their own pass, but their messaging-coupled parts are gone.
Verified with a full-suite collection sweep (12010 tests collected, zero
import errors) beyond the directly touched test dirs, given the expanded
blast radius.
* refactor: remove channel/session/message models, tables, and channel policy
Models: deleted channel.py/group.py/session.py/messaging.py wholesale
(zero external consumers besides the models/__init__.py re-export).
message.py surgically trimmed: removed MessageCreate (dead) and MessageEdit
(never instantiated; ExtractedMessage.edit_history retyped to
list[dict[str, Any]] to match how it's actually persisted — confirmed
ExtractedMessage was never written to any DB table, so MessageTable's
removal carries no functional risk to the kept extraction pipeline).
base.py: removed SessionStatus + ChannelType, kept MessageType. Also
removed the confirmed-dead channels_read/channels_write fields from
models/agent.py:AgentPermissions and models/dashboard.py:ChannelFeedData.
db/tables.py: deleted ChannelTable/GroupTable/SessionTable/SessionTaskTable/
MessageTable, TaskTable.session_links, and JournalEntryTable.session_id —
cascaded through models/journal.py, services/journal.py, and
api/schemas+routes/journals.py (22 plumbing sites).
foundation/policy/communications.py: removed the ChannelSpec/CHANNELS
catalog + TEAM_SCOPED_ROLES/_CELL_*/_AUDITOR_ONLY helpers, kept the
notification policy (Priority/parse_priority/NOTIFY_SENDER_ROLES/
ACK_REQUIRED_BY_TYPE). enforcement/channel_access.py deleted (confirmed
fully dead in production). agents_config.py: removed CHANNEL_ACCESS
(kept A2A_ALLOWED_PAIRS). seeds/initial_data.py: removed
DEFAULT_CHANNELS/CHANNEL_MEMBERSHIPS/AUDITOR_SILENT_ACCESS + the
never-consumed INITIAL_MESSAGES. config.py: removed
session_idle_timeout_seconds (zero consumers). exceptions.py: removed
dead ChannelError/ChannelAccessDeniedError/SessionClosedError.
Forced expansion beyond the original file list — ChannelType cascaded
into a live, mounted surface the plan didn't trace: agents_config.
CHANNEL_ACCESS -> services/permissions.py's channel-RBAC methods (not
models/permissions.py, which turned out to have no channel code at all)
-> two real endpoints in api/routes/stream.py (GET /permissions,
GET /permissions/channel/{name}) and two dependency factories in
api/deps.py. Removed the channel methods + fields, deleted the
channel-specific stream.py endpoint, deleted require_channel_read/write.
Also deleted api/schemas/{channels,sessions}.py (hard dependency on the
removed enums; already fully dead after the Task 10 route deletions) and
api/schemas/messages.py (a TYPE_CHECKING-only import of the deleted
MessageTable; likewise already fully dead) + its dedicated test file.
Test updates: test_permissions.py -14 channel tests (matches the planned
count exactly), test_communications.py / test_communications_consumers.py
split to keep only notification-policy coverage, test_exceptions.py -9,
test_deps.py -4, plus the journal/stream/foundation-smoke fallout. Also
fixed a pre-existing (Task 7) broken assertion in
test_foundation_phase3_smoke.py that inspected a `say()` method already
removed from ContentActions.
Verified: full-suite collection (11961 tests, zero import errors) and a
complete test run (11567 passed, 394 skipped, 0 failed) in addition to
the targeted suites.
* migration: drop channels/groups/sessions/session_tasks/messages + enum types
alembic/versions/060_drop_messaging.py: drop_column journal_entries.
session_id (sidesteps hardcoding the FK constraint name — verified
empirically against a live migrated DB that it's actually
fk_journal_entries_session_id_sessions, but drop_column doesn't care
either way); drop_table in FK order (messages -> session_tasks ->
sessions -> groups -> channels); DROP TABLE IF EXISTS chunks_conversations
(runtime-provisioned, not alembic-managed, would otherwise orphan); DROP
TYPE IF EXISTS for messagetype/sessionstatus/sessionscope/channeltype
(messagetype's Python enum stays for ExtractedMessage, but the DB type
had zero live columns left once MessageTable was dropped in the prior
commit). downgrade() raises NotImplementedError — one-way removal.
Pruned scripts/reset_runtime_state.sql + .sh: removed the DELETE/COUNT
lines for messages/session_tasks/sessions/groups/channels and the
groups.active_session_id reset block.
Verified end-to-end against a scratch Postgres DB: full migration chain
001->060 applies cleanly, alembic heads shows a single head, all 6 dropped
tables + 4 enum types + the journal_entries.session_id column are
confirmed gone, journal_entries keeps only its journal_id/task_id FKs,
downgrade correctly raises NotImplementedError without corrupting DB
state, and the pruned reset_runtime_state.sql runs clean (no errors)
against a fully-migrated DB.
* refactor(api): remove channel/session/message routes + WS streams
Most of this task's file list was already forced through in earlier
commits (routes/{channels,groups,sessions,messages}.py + app.py/__init__.py
unmounting in the MessagingService-deletion commit; tasks.py's
session-links embed + GET /{id}/sessions + schemas/tasks.py's
TaskResponse.sessions in that same commit; deps.py's require_channel_read/
write + schemas/{channels,sessions}.py in the models/tables commit). This
closes out what was left:
- api/websocket.py: deleted the channel_stream + session_stream routes,
ConnectionManager's channel_connections/session_connections dicts,
connect_channel/connect_session, broadcast_to_channel/broadcast_to_session,
get_channel_subscriber_count, and their cleanup lines in disconnect().
Agent streams, notification streams, and the operator system stream are
untouched.
- api/websocket_bridge.py: deleted _handle_session_event +
_handle_message_event and their SESSION_CREATED/SESSION_CLOSED/
SESSION_TIMEOUT/MESSAGE_SENT subscriptions. The A2A live-view, rate-limit,
usage, agent-lifecycle, and notification bridges are untouched.
- api/schemas/websocket.py: removed NewMessageBroadcast, WSMessageNew,
WSMessageEdit, WSMessageDelete, WSSessionClosed — kept the WSMessage base
class (still subclassed by the kept WSAgentStream/WSNotification) plus
those two.
- api/schemas/groups.py: deleted (already fully orphaned since routes/
groups.py was removed; its GroupResponse/GroupDetailResponse had zero
consumers).
Updated the 5 websocket test files accordingly (removed the channel/
session-specific tests + fixed imports); test_websocket_bridge.py's
registration-coverage test dropped the SESSION_*/MESSAGE_SENT assertions.
Verified: full-suite collection (11943 tests, zero import errors) and a
complete test run (11549 passed, 394 skipped, 0 failed).
* docs: retire channels/sessions/messages from agent-facing docs + CLAUDE.md
Rewrites docs/rag (RAG-indexed) + docs/map + CLAUDE.md to reflect A2A (dm +
read_a2a) as primary agent comms; deletes the channel docs, splits messaging-tools
+ messaging-notification (renamed notification.md), swaps the WS worked example to
A2A_MESSAGE_SENT. _complete_map.md still needs regeneration (generated file).
* refactor(panel): remove Communications surface (channels/sessions)
Deletes the /communications routes, message components, task-detail Sessions tab,
use-channels + channel/session WS hooks, and the channels/sessions/messages/groups
api clients; prunes the Channel/Session/Message/Group types + mock data. (Auditor
live-feeds + dashboard.ts dead-route cleanup is a follow-up.)
* refactor(panel): drop auditor channel-feed + dead communication-metric route
* docs(map): regenerate _complete_map from updated slices
* fix(a2a): reduce get_unread_messages complexity below xenon C + stale comments
Extract the per-conversation unread-counter recompute into _reset_unread_counter
(the CI quality gate flagged get_unread_messages as rank C). Also drop the deleted
open_session from a content_actions comment and reword an evidence_repo docstring
that cited the removed messaging._notify_mentions.
---------
Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
@@ -3,7 +3,6 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from roboco.services.optimal_brain.indexes.base import build_doc_source
|
||||
from roboco.services.optimal_brain.indexes.conversations import ConversationsIndexPlugin
|
||||
from roboco.services.optimal_brain.indexes.journals import JournalsIndexPlugin
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -20,15 +19,6 @@ def test_doc_source_with_id() -> None:
|
||||
assert result == "roboco://journals/abc-123"
|
||||
|
||||
|
||||
def test_doc_source_conversations_with_id() -> None:
|
||||
result = build_doc_source(kind="conversations", id_="sess-001-agent-007")
|
||||
assert result == "roboco://conversations/sess-001-agent-007"
|
||||
|
||||
|
||||
def test_doc_source_conversations_returns_none_when_id_missing() -> None:
|
||||
assert build_doc_source(kind="conversations", id_=None) is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests for JournalsIndexPlugin.build_source_uri
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -53,24 +43,3 @@ def test_journals_plugin_build_source_uri_falls_back_to_doc_id() -> None:
|
||||
plugin = JournalsIndexPlugin.__new__(JournalsIndexPlugin)
|
||||
result = plugin.build_source_uri(doc_id="fallback-id")
|
||||
assert result == "roboco://journals/fallback-id"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests for ConversationsIndexPlugin.build_source_uri
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_conversations_plugin_returns_none_when_session_id_none() -> None:
|
||||
"""build_source_uri returns None when session_id kwarg is None."""
|
||||
plugin = ConversationsIndexPlugin.__new__(ConversationsIndexPlugin)
|
||||
result = plugin.build_source_uri(doc_id=None, session_id=None, agent_id="agent-1")
|
||||
assert result is None
|
||||
|
||||
|
||||
def test_conversations_plugin_build_source_uri_with_session_id() -> None:
|
||||
"""build_source_uri returns correct URI when session_id is set."""
|
||||
plugin = ConversationsIndexPlugin.__new__(ConversationsIndexPlugin)
|
||||
result = plugin.build_source_uri(
|
||||
doc_id=None, session_id="sess-999", agent_id="agent-007"
|
||||
)
|
||||
assert result == "roboco://conversations/sess-999-agent-007"
|
||||
|
||||
@@ -2,8 +2,7 @@
|
||||
|
||||
Guards the HIGH fix: without a delete-by-source step, every startup/periodic/
|
||||
manual reindex appended a fresh copy of each doc's chunks, growing the tables
|
||||
unbounded and crowding out distinct results. The carve-out is conversations,
|
||||
whose many messages share one source URI — there, append must be preserved.
|
||||
unbounded and crowding out distinct results.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -11,7 +10,6 @@ from __future__ import annotations
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
from roboco.services.optimal_brain.indexes.conversations import ConversationsIndexPlugin
|
||||
from roboco.services.optimal_brain.indexes.standards import StandardsIndexPlugin
|
||||
from roboco.services.optimal_brain.text_chunker import Chunk, Document
|
||||
|
||||
@@ -37,10 +35,6 @@ def test_default_plugin_replaces_on_reingest() -> None:
|
||||
assert StandardsIndexPlugin.replace_on_reingest is True
|
||||
|
||||
|
||||
def test_conversations_plugin_appends_not_replaces() -> None:
|
||||
assert ConversationsIndexPlugin.replace_on_reingest is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reingest_replaces_source_chunks_atomically(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
@@ -73,19 +67,3 @@ async def test_reingest_replaces_source_chunks_atomically(
|
||||
called_source, called_chunks = store.replace_chunks.await_args.args
|
||||
assert called_source == source
|
||||
assert len(called_chunks) == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reingest_preserves_history_for_conversations(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
plugin = ConversationsIndexPlugin()
|
||||
source = "roboco://conversations/sess-1-agent-1"
|
||||
store = _wire_plugin(plugin, source, monkeypatch)
|
||||
doc = Document(content="x" * 250, source=source, metadata={})
|
||||
|
||||
count = await plugin._chunk_filter_embed_store(doc, {})
|
||||
|
||||
assert count == 1
|
||||
store.delete_by_source.assert_not_awaited()
|
||||
store.add_chunks.assert_awaited_once()
|
||||
|
||||
@@ -1,100 +0,0 @@
|
||||
"""Recover from a concurrent auto-create race on the channel slug's UNIQUE
|
||||
constraint instead of crashing the caller with an ``IntegrityError``.
|
||||
|
||||
Isolate the insert in a savepoint; on a unique-conflict ``IntegrityError``
|
||||
re-fetch the winner's row. A conflict that did NOT produce a row is re-raised.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
from roboco.services.messaging import MessagingService
|
||||
from sqlalchemy.exc import IntegrityError
|
||||
|
||||
_SLUG = "backend-cell"
|
||||
_LOOKUPS_BEFORE_AND_AFTER_RACE = 2
|
||||
|
||||
|
||||
def _bind(svc: object, name: str, value: object) -> Any:
|
||||
"""Stub `name` on `svc` without tripping mypy's method-assign check.
|
||||
Returns the value (typed ``Any``) so the caller can keep a reference for
|
||||
assertions — ``object.__setattr__`` does not narrow the attribute type, so
|
||||
assert on the returned local, not ``svc.<name>``."""
|
||||
object.__setattr__(svc, name, value)
|
||||
return value
|
||||
|
||||
|
||||
def _integrity_error() -> IntegrityError:
|
||||
return IntegrityError(
|
||||
"INSERT INTO channels ...",
|
||||
{},
|
||||
Exception("duplicate key value violates unique constraint channels_slug_key"),
|
||||
)
|
||||
|
||||
|
||||
def _svc(*, flush_side_effect: Any = None) -> tuple[MessagingService, AsyncMock]:
|
||||
session = AsyncMock()
|
||||
session.add = MagicMock()
|
||||
if flush_side_effect is not None:
|
||||
session.flush = AsyncMock(side_effect=flush_side_effect)
|
||||
else:
|
||||
session.flush = AsyncMock()
|
||||
# ``begin_nested`` returns an async context manager (savepoint). The default
|
||||
# AsyncMock magic-method config makes ``async with`` work; ``__aexit__``
|
||||
# returns falsy so an exception raised in the body propagates (mirroring
|
||||
# the real savepoint, which rolls back and re-raises).
|
||||
session.begin_nested = MagicMock(return_value=AsyncMock())
|
||||
svc = MessagingService(session)
|
||||
return svc, session
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_race_lost_refetches_existing_channel() -> None:
|
||||
"""Concurrent auto-create loser: flush raises IntegrityError, the method
|
||||
re-fetches the winner's channel and returns it (no crash)."""
|
||||
existing = MagicMock(name="existing-channel", slug=_SLUG)
|
||||
svc, session = _svc(flush_side_effect=_integrity_error())
|
||||
get_channel_by_slug = _bind(
|
||||
svc, "get_channel_by_slug", AsyncMock(side_effect=[None, existing])
|
||||
)
|
||||
|
||||
result = await svc.get_or_create_channel_by_slug(_SLUG)
|
||||
|
||||
assert result is existing
|
||||
# Savepoint isolated the failed insert; re-fetch was the recovery.
|
||||
session.begin_nested.assert_called_once()
|
||||
assert get_channel_by_slug.await_count == _LOOKUPS_BEFORE_AND_AFTER_RACE
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_race_lost_but_re_fetch_empty_reraises() -> None:
|
||||
"""If the conflict did NOT produce a row on re-fetch (a real failure, not a
|
||||
race), the IntegrityError is re-raised — never masked as a silent None."""
|
||||
svc, _session = _svc(flush_side_effect=_integrity_error())
|
||||
_bind(svc, "get_channel_by_slug", AsyncMock(side_effect=[None, None]))
|
||||
|
||||
with pytest.raises(IntegrityError):
|
||||
await svc.get_or_create_channel_by_slug(_SLUG)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_normal_auto_create_unaffected_by_savepoint() -> None:
|
||||
"""No race: the insert flushes cleanly inside the savepoint and the
|
||||
newly-created channel is returned (regression guard — the savepoint must
|
||||
not break the happy path)."""
|
||||
svc, session = _svc() # flush succeeds
|
||||
get_channel_by_slug = _bind(
|
||||
svc, "get_channel_by_slug", AsyncMock(side_effect=[None])
|
||||
) # not present, then never re-called
|
||||
|
||||
result = await svc.get_or_create_channel_by_slug(_SLUG)
|
||||
|
||||
assert result is not None
|
||||
assert result.slug == _SLUG
|
||||
session.begin_nested.assert_called_once()
|
||||
session.add.assert_called_once()
|
||||
assert session.flush.await_count == 1
|
||||
assert get_channel_by_slug.await_count == 1 # no recovery re-fetch
|
||||
@@ -1,111 +0,0 @@
|
||||
"""``create_session`` must not orphan an ACTIVE session under concurrent posts.
|
||||
|
||||
Lock the group row (``SELECT ... FOR UPDATE``) and re-read
|
||||
``active_session_id`` under the lock before creating, so concurrent callers
|
||||
serialize per group and the loser reuses the winner's session.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
from roboco.db.tables import SessionTable
|
||||
from roboco.models.base import SessionStatus
|
||||
from roboco.models.messaging import SessionCreateRequest
|
||||
from roboco.services.messaging import MessagingService
|
||||
from sqlalchemy.dialects import postgresql
|
||||
|
||||
_GROUP_ID = MagicMock(name="group-id")
|
||||
_WINNER_SESSION_ID = MagicMock(name="winner-session-id")
|
||||
|
||||
|
||||
def _bind(svc: object, name: str, value: object) -> Any:
|
||||
"""Stub `name` on `svc` without tripping mypy's method-assign check.
|
||||
Returns the value (typed ``Any``) so the caller can keep a reference for
|
||||
assertions — ``object.__setattr__`` does not narrow the attribute type, so
|
||||
assert on the returned local, not ``svc.<name>``."""
|
||||
object.__setattr__(svc, name, value)
|
||||
return value
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_lock_group_emits_for_update() -> None:
|
||||
"""``_lock_group`` must issue ``SELECT ... FOR UPDATE`` (the row lock that
|
||||
serializes concurrent session creation per group)."""
|
||||
session = AsyncMock()
|
||||
captured: list[Any] = []
|
||||
result_mock = MagicMock()
|
||||
result_mock.scalar_one_or_none.return_value = MagicMock(active_session_id=None)
|
||||
|
||||
async def _exec(stmt: Any) -> Any:
|
||||
captured.append(stmt)
|
||||
return result_mock
|
||||
|
||||
session.execute = AsyncMock(side_effect=_exec)
|
||||
svc = MessagingService(session)
|
||||
|
||||
await svc._lock_group(_GROUP_ID)
|
||||
|
||||
sql = str(
|
||||
captured[0].compile(
|
||||
dialect=postgresql.dialect(), compile_kwargs={"literal_binds": True}
|
||||
)
|
||||
)
|
||||
assert "FOR UPDATE" in sql
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_session_race_loser_reuses_winner_under_lock() -> None:
|
||||
"""Concurrent posts: caller A wins the race and links its session while
|
||||
caller B is between the check and the create. Caller B locks the group,
|
||||
re-reads ``active_session_id`` (now A's session), and reuses it — no second
|
||||
ACTIVE session is created (no orphan)."""
|
||||
session = AsyncMock()
|
||||
session.add = MagicMock()
|
||||
session.flush = AsyncMock()
|
||||
svc = MessagingService(session)
|
||||
|
||||
winner = MagicMock(name="winner-session", status=SessionStatus.ACTIVE)
|
||||
_bind(svc, "get_group", AsyncMock(return_value=MagicMock(active_session_id=None)))
|
||||
# Under the lock, the group now reflects the winner's link.
|
||||
lock_group = _bind(
|
||||
svc,
|
||||
"_lock_group",
|
||||
AsyncMock(return_value=MagicMock(active_session_id=_WINNER_SESSION_ID)),
|
||||
)
|
||||
get_session = _bind(svc, "get_session", AsyncMock(return_value=winner))
|
||||
|
||||
result = await svc.create_session(SessionCreateRequest(group_id=_GROUP_ID))
|
||||
|
||||
assert result is winner
|
||||
lock_group.assert_awaited_once()
|
||||
get_session.assert_awaited_once()
|
||||
session.add.assert_not_called() # no orphaning INSERT
|
||||
session.flush.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_session_creates_when_no_active_under_lock() -> None:
|
||||
"""No race: under the lock there is still no active session, so create a
|
||||
new ACTIVE session and link it on the group (regression guard — the lock
|
||||
must not break the happy path)."""
|
||||
session = AsyncMock()
|
||||
session.add = MagicMock()
|
||||
session.flush = AsyncMock()
|
||||
svc = MessagingService(session)
|
||||
|
||||
locked_group = MagicMock(active_session_id=None)
|
||||
_bind(svc, "get_group", AsyncMock(return_value=MagicMock(active_session_id=None)))
|
||||
lock_group = _bind(svc, "_lock_group", AsyncMock(return_value=locked_group))
|
||||
get_session = _bind(svc, "get_session", AsyncMock()) # NOT called (no active id)
|
||||
|
||||
result = await svc.create_session(SessionCreateRequest(group_id=_GROUP_ID))
|
||||
|
||||
assert isinstance(result, SessionTable)
|
||||
assert result.status == SessionStatus.ACTIVE
|
||||
lock_group.assert_awaited_once()
|
||||
get_session.assert_not_awaited()
|
||||
session.add.assert_called_once()
|
||||
assert session.flush.await_count >= 1
|
||||
@@ -19,7 +19,6 @@ from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.models.optimal import (
|
||||
IndexConversationParams,
|
||||
IndexJournalEntryParams,
|
||||
IndexReviewParams,
|
||||
IndexType,
|
||||
@@ -68,7 +67,6 @@ def _service_with_stub_plugin() -> _StubOptimalService:
|
||||
plugin.ingest = AsyncMock()
|
||||
svc._plugins = {
|
||||
IndexType.JOURNALS: plugin,
|
||||
IndexType.CONVERSATIONS: plugin,
|
||||
IndexType.REVIEWS: plugin,
|
||||
}
|
||||
svc._initialized = True
|
||||
@@ -115,31 +113,6 @@ async def test_index_journal_entry_succeeds_with_real_entry_id() -> None:
|
||||
await svc.index_journal_entry(params)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_index_conversation_raises_when_session_id_missing() -> None:
|
||||
"""``session_id`` is typed UUID (required) but the old code coerced
|
||||
falsy values to ``'unknown'``. Reject explicitly so the caller fixes
|
||||
the missing flush instead of writing junk doc-sources.
|
||||
|
||||
A real caller would have to bypass the dataclass type contract for
|
||||
this to fire (e.g. ``cast(UUID, None)``); we simulate that with a
|
||||
``SimpleNamespace`` so we don't have to reach inside a frozen-style
|
||||
dataclass to clobber a field.
|
||||
"""
|
||||
svc = _service_with_stub_plugin()
|
||||
fake_params = SimpleNamespace(
|
||||
content="hello",
|
||||
channel_id=uuid4(),
|
||||
session_id=None, # the runtime bug we now reject
|
||||
agent_id=uuid4(),
|
||||
task_id=None,
|
||||
message_type=None,
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError, match="session_id is required"):
|
||||
await svc.index_conversation(cast("IndexConversationParams", fake_params))
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_record_review_raises_when_file_path_empty() -> None:
|
||||
"""``file_path`` is typed ``str`` (required); empty strings produced
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
"""PermissionService coverage — RBAC for channels, notifications, tasks, KB.
|
||||
"""PermissionService coverage — RBAC for notifications, tasks, KB.
|
||||
|
||||
Pure-logic checks driven by ``agents_config`` constants — no DB needed.
|
||||
The service is a SingletonService, so we instantiate it directly with
|
||||
@@ -31,69 +31,6 @@ def _ctx(role: AgentRole, team: Team | None = None) -> AgentContext:
|
||||
return AgentContext(agent_id=uuid4(), role=role, team=team)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Channel read access
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_auditor_can_read_any_channel(svc: PermissionService) -> None:
|
||||
"""AUDITOR has silent read on every channel."""
|
||||
auditor = _ctx(AgentRole.AUDITOR)
|
||||
assert svc.can_read_channel(auditor, "backend-cell")
|
||||
assert svc.can_read_channel(auditor, "main-pm-board")
|
||||
assert svc.can_read_channel(auditor, "any-channel-name")
|
||||
|
||||
|
||||
def test_ceo_can_read_any_channel(svc: PermissionService) -> None:
|
||||
ceo = _ctx(AgentRole.CEO)
|
||||
assert svc.can_read_channel(ceo, "backend-cell")
|
||||
|
||||
|
||||
def test_main_pm_can_read_any_channel(svc: PermissionService) -> None:
|
||||
main_pm = _ctx(AgentRole.MAIN_PM)
|
||||
assert svc.can_read_channel(main_pm, "backend-cell")
|
||||
assert svc.can_read_channel(main_pm, "frontend-cell")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Channel write access
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_ceo_can_write_any_channel(svc: PermissionService) -> None:
|
||||
ceo = _ctx(AgentRole.CEO)
|
||||
assert svc.can_write_channel(ceo, "backend-cell")
|
||||
|
||||
|
||||
def test_auditor_cannot_write_any_channel(svc: PermissionService) -> None:
|
||||
"""Auditor is a silent, read-only observer — it cannot write to channels."""
|
||||
auditor = _ctx(AgentRole.AUDITOR)
|
||||
assert not svc.can_write_channel(auditor, "backend-cell")
|
||||
|
||||
|
||||
def test_main_pm_can_write_any_channel(svc: PermissionService) -> None:
|
||||
main_pm = _ctx(AgentRole.MAIN_PM)
|
||||
assert svc.can_write_channel(main_pm, "backend-cell")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Channel listing
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_get_accessible_channels_for_auditor(svc: PermissionService) -> None:
|
||||
"""Auditor sees every configured channel."""
|
||||
auditor = _ctx(AgentRole.AUDITOR)
|
||||
channels = svc.get_accessible_channels(auditor)
|
||||
assert len(channels) > 0
|
||||
|
||||
|
||||
def test_get_writable_channels_for_ceo(svc: PermissionService) -> None:
|
||||
ceo = _ctx(AgentRole.CEO)
|
||||
channels = svc.get_writable_channels(ceo)
|
||||
assert len(channels) > 0
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Notifications
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -254,50 +191,6 @@ def test_can_notify_cell_pm_to_dev_in_same_team(svc: PermissionService) -> None:
|
||||
assert svc.can_notify(sender, recipient) is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Channel bypass edge cases
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_can_read_channel_for_main_pm_unknown_bypasses(
|
||||
svc: PermissionService,
|
||||
) -> None:
|
||||
"""Main PM has bypass — unknown channel returns True (no DB lookup)."""
|
||||
main_pm = _ctx(AgentRole.MAIN_PM)
|
||||
assert svc.can_read_channel(main_pm, "ghost-channel") is True
|
||||
|
||||
|
||||
def test_can_write_channel_for_ceo_unknown_bypasses(
|
||||
svc: PermissionService,
|
||||
) -> None:
|
||||
"""CEO has bypass — unknown channel returns True."""
|
||||
ceo = _ctx(AgentRole.CEO)
|
||||
assert svc.can_write_channel(ceo, "ghost-channel") is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Channel read for non-bypass roles (covers _check_channel_access_for_agent)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_dev_read_unknown_channel_returns_false(svc: PermissionService) -> None:
|
||||
"""Unknown channel for non-bypass role → warns + returns False (lines 137-138)."""
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert svc.can_read_channel(dev, "ghost-channel-x") is False
|
||||
|
||||
|
||||
def test_dev_write_unknown_channel_returns_false(svc: PermissionService) -> None:
|
||||
"""Unknown channel for non-bypass role on write → False."""
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert svc.can_write_channel(dev, "ghost-channel-y") is False
|
||||
|
||||
|
||||
def test_dev_can_read_own_cell_channel(svc: PermissionService) -> None:
|
||||
"""Developer in backend can read backend-cell (regular role-based access)."""
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert svc.can_read_channel(dev, "backend-cell") is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# can_notify branches
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -340,16 +233,6 @@ def test_view_own_falls_back_to_view_all_for_ceo(svc: PermissionService) -> None
|
||||
assert svc.can_perform_task_action(ceo, TaskAction.VIEW_OWN, Team.BACKEND) is True
|
||||
|
||||
|
||||
def test_check_channel_access_silent_observer_grants_read(
|
||||
svc: PermissionService,
|
||||
) -> None:
|
||||
"""Line 151: agent slug in silent list grants read access via direct call."""
|
||||
auditor = _ctx(AgentRole.AUDITOR, team=Team.BOARD)
|
||||
# _check_channel_access_for_agent bypasses the auditor short-circuit at
|
||||
# can_read_channel and exercises the silent-list match (line 150-151).
|
||||
assert svc._check_channel_access_for_agent(auditor, "backend-cell", "read") is True
|
||||
|
||||
|
||||
def test_can_notify_unknown_scope_returns_false(svc: PermissionService) -> None:
|
||||
"""Line 258: scope is neither 'all', 'cell', nor list → defensive return False."""
|
||||
|
||||
|
||||
@@ -243,7 +243,6 @@ def _gateway_actions(
|
||||
deps = ContentActionsDeps(
|
||||
task=task,
|
||||
git=MagicMock(),
|
||||
messaging=MagicMock(),
|
||||
a2a=MagicMock(),
|
||||
journal=MagicMock(),
|
||||
workspace=MagicMock(),
|
||||
|
||||
@@ -1,96 +0,0 @@
|
||||
"""Task #156: MessagingService.propagate_sessions_to_subtask.
|
||||
|
||||
Tests the helper's call shape and idempotency contract without going
|
||||
through the DB. Integration coverage (real DB, real linking) lives in
|
||||
``tests/integration/test_messaging_service.py``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.models.session import SessionTaskRelationshipType
|
||||
from roboco.services.messaging import MessagingService
|
||||
|
||||
|
||||
def _link(session_id: object, relationship_type: str) -> MagicMock:
|
||||
link = MagicMock()
|
||||
link.session_id = session_id
|
||||
link.relationship_type = relationship_type
|
||||
return link
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_propagate_links_every_parent_session_to_subtask() -> None:
|
||||
"""Every link on the parent gets re-attached to the new subtask."""
|
||||
svc: Any = MessagingService.__new__(MessagingService)
|
||||
parent_session = uuid4()
|
||||
review_session = uuid4()
|
||||
svc.get_sessions_for_task = AsyncMock(
|
||||
return_value=[
|
||||
_link(parent_session, "discussion"),
|
||||
_link(review_session, "review"),
|
||||
]
|
||||
)
|
||||
|
||||
calls: list[dict[str, Any]] = []
|
||||
|
||||
async def fake_link(**kwargs: Any) -> Any:
|
||||
calls.append(kwargs)
|
||||
link = MagicMock()
|
||||
link.session_id = kwargs["session_id"]
|
||||
link.task_id = kwargs["task_id"]
|
||||
return link
|
||||
|
||||
parent_id = uuid4()
|
||||
subtask_id = uuid4()
|
||||
added_by = uuid4()
|
||||
with patch.object(svc, "link_session_to_task", new=fake_link):
|
||||
out = await svc.propagate_sessions_to_subtask(parent_id, subtask_id, added_by)
|
||||
expected_sessions = {parent_session, review_session}
|
||||
assert len(out) == len(expected_sessions)
|
||||
assert {c["session_id"] for c in calls} == expected_sessions
|
||||
# Every propagated link must be non-primary — primary is the subtask's
|
||||
# own slot, never inherited from the parent.
|
||||
assert all(c["is_primary"] is False for c in calls)
|
||||
# Every call must target the new subtask (not the parent).
|
||||
assert {c["task_id"] for c in calls} == {subtask_id}
|
||||
# Relationship types preserved.
|
||||
rels = {c["relationship_type"] for c in calls}
|
||||
assert SessionTaskRelationshipType.DISCUSSION in rels
|
||||
assert SessionTaskRelationshipType.REVIEW in rels
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_propagate_no_parent_sessions_returns_empty() -> None:
|
||||
"""When the parent has no session links, propagation is a no-op."""
|
||||
svc: Any = MessagingService.__new__(MessagingService)
|
||||
svc.get_sessions_for_task = AsyncMock(return_value=[])
|
||||
svc.link_session_to_task = AsyncMock()
|
||||
|
||||
out = await svc.propagate_sessions_to_subtask(uuid4(), uuid4(), uuid4())
|
||||
assert out == []
|
||||
svc.link_session_to_task.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_propagate_unknown_relationship_type_defaults_to_discussion() -> None:
|
||||
"""Garbage relationship_type on the parent link doesn't crash; it
|
||||
defaults to DISCUSSION so the subtask is still linked."""
|
||||
svc: Any = MessagingService.__new__(MessagingService)
|
||||
svc.get_sessions_for_task = AsyncMock(
|
||||
return_value=[_link(uuid4(), "definitely-not-a-real-type")]
|
||||
)
|
||||
calls: list[dict[str, Any]] = []
|
||||
|
||||
async def fake_link(**kwargs: Any) -> Any:
|
||||
calls.append(kwargs)
|
||||
return MagicMock()
|
||||
|
||||
with patch.object(svc, "link_session_to_task", new=fake_link):
|
||||
await svc.propagate_sessions_to_subtask(uuid4(), uuid4(), uuid4())
|
||||
assert len(calls) == 1
|
||||
assert calls[0]["relationship_type"] == SessionTaskRelationshipType.DISCUSSION
|
||||
@@ -24,9 +24,6 @@ def _session() -> MagicMock:
|
||||
|
||||
|
||||
def _patch(monkeypatch: pytest.MonkeyPatch) -> dict[str, MagicMock]:
|
||||
msg = MagicMock()
|
||||
msg.post_to_channel = AsyncMock()
|
||||
monkeypatch.setattr(sec_module, "get_messaging_service", lambda _s: msg)
|
||||
goals = MagicMock()
|
||||
goals.upsert = AsyncMock()
|
||||
monkeypatch.setattr(sec_module, "get_company_goals_service", lambda _s: goals)
|
||||
@@ -45,11 +42,12 @@ def _patch(monkeypatch: pytest.MonkeyPatch) -> dict[str, MagicMock]:
|
||||
monkeypatch.setattr(sec_module, "get_agent_by_slug", agent_lookup)
|
||||
notifier = MagicMock()
|
||||
notifier.send_ack_notification = AsyncMock()
|
||||
notifier.send_broadcast_notification = AsyncMock()
|
||||
monkeypatch.setattr(
|
||||
"roboco.services.notification.NotificationService", lambda: notifier
|
||||
)
|
||||
monkeypatch.setattr(sec_module, "NotificationService", lambda: notifier)
|
||||
return {
|
||||
"msg": msg,
|
||||
"goals": goals,
|
||||
"pitch": pitch,
|
||||
"task": task,
|
||||
@@ -74,12 +72,11 @@ async def test_relay_executes_directly(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
svc = SecretaryService(_session())
|
||||
row = await svc.submit_directive(
|
||||
DirectiveKind.RELAY_MESSAGE,
|
||||
{"channel": "all-hands", "text": "standup at 10"},
|
||||
{"text": "standup at 10"},
|
||||
uuid4(),
|
||||
)
|
||||
assert row.status == DirectiveStatus.EXECUTED.value
|
||||
svcs["msg"].post_to_channel.assert_awaited_once()
|
||||
svcs["notifier"].send_ack_notification.assert_not_awaited()
|
||||
svcs["notifier"].send_broadcast_notification.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -144,7 +141,7 @@ async def test_announce_queues_then_confirm_posts(
|
||||
monkeypatch.setattr(svc, "get_directive", AsyncMock(return_value=row))
|
||||
out = await svc.confirm_directive(row.id, uuid4())
|
||||
assert out.status == DirectiveStatus.EXECUTED.value
|
||||
svcs["msg"].post_to_channel.assert_awaited_once()
|
||||
svcs["notifier"].send_broadcast_notification.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -53,6 +53,32 @@ def test_sync_read_clone_advances_to_a_post_clone_commit(tmp_path: Path) -> None
|
||||
assert (clone / "NEW.txt").is_file()
|
||||
|
||||
|
||||
def test_sync_read_clone_fetches_tags(tmp_path: Path) -> None:
|
||||
"""The read clone must carry tags: the release manager derives "commits
|
||||
since last release" from the newest tag, and a tagless clone makes
|
||||
``git describe`` fail so it walks the entire history (the 729-commit bug)."""
|
||||
origin = tmp_path / "origin"
|
||||
origin.mkdir()
|
||||
_git(origin, "init", "-q", "-b", "master")
|
||||
(origin / "README.md").write_text("v1\n")
|
||||
_commit(origin, "first")
|
||||
|
||||
# The clone itself is tagless (--no-tags, as agent clones are) — the tag
|
||||
# lands on origin and the read-clone refresh must pull it in.
|
||||
clone = tmp_path / "clone"
|
||||
_git(tmp_path, "clone", "-q", "--no-tags", str(origin), str(clone))
|
||||
assert _git(clone, "tag").stdout.strip() == ""
|
||||
|
||||
# -c tag.gpgsign=false: create a lightweight tag regardless of a global
|
||||
# signing config that would otherwise force an annotated (signed) tag.
|
||||
_git(origin, "-c", "tag.gpgsign=false", "tag", "v0.17.0")
|
||||
|
||||
WorkspaceService._sync_read_clone(clone, f"file://{origin}", "master", None)
|
||||
|
||||
assert "v0.17.0" in _git(clone, "tag").stdout.split()
|
||||
assert _git(clone, "describe", "--tags", "--abbrev=0").stdout.strip() == "v0.17.0"
|
||||
|
||||
|
||||
def test_sync_read_clone_is_best_effort_on_unreachable_origin(tmp_path: Path) -> None:
|
||||
origin = tmp_path / "origin"
|
||||
origin.mkdir()
|
||||
|
||||
Reference in New Issue
Block a user