mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Fix: open findings cleanup (#122)
* refactor(usage): remove the unconsumed per-agent USAGE_UPDATE event USAGE_UPDATE was published per active agent each sweep, bridged, and broadcast to /ws/system, but no panel client ever consumed it — the dashboard reads only the aggregate USAGE_SNAPSHOT. Every emission was wasted event-bus and WebSocket traffic. Drop the UsageUpdate payload, publish_usage_update and its throttle, the EventType member, and the bridge subscription. Keep USAGE_SNAPSHOT, which already carries the per-agent breakdown, so no live data is lost. * refactor(prompter): remove the legacy local-LLM HTTP endpoints The panel uses only the live SDK-intake path (/prompter/live/*); the legacy /prompter/chat, /draft and /sessions/* endpoints — backed by the local Ollama LLM with hardcoded prompts — had no remaining caller. Remove the router, its mount in app.py, and its integration test. The live router and the shared draft-confirmation service are untouched. * refactor(prompter): drop the dead legacy local-LLM service + schemas With the legacy HTTP endpoints gone, the local-LLM chat/draft/session methods, their prompt constants, the ConfirmOverrides/TurnResult dataclasses, and the entire prompter schema module had no production caller (only their own tests). Remove them, keeping the live-intake path: create_task_from_draft / confirm_live_draft, the enum/priority/team coercion, and the pure description/readiness helpers. * refactor(agents): stop granting the Task sub-agent tool to roles Every agent role was granted the built-in Task tool, but no role prompt or workflow uses it and there are no custom sub-agent definitions — so a Task call only spawns a context-blind generic sub-agent that burns budget (ToolSearch, the comment's stated use, is MCP-only and not callable in agent containers). Drop Task from all three grant points in lockstep: the --tools spawn flag and both _ROLE_BUILTIN_TOOLS maps (system-prompt + briefing layers), with a regression guard added to each layer's test. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
@@ -1,14 +1,14 @@
|
||||
"""Unit tests for PrompterService.
|
||||
|
||||
Tests the service layer logic with mocked LLM calls. Uses an in-memory
|
||||
async session (via conftest fixtures) for DB-backed tests.
|
||||
Covers the live-intake draft → task flow (``create_task_from_draft`` /
|
||||
``confirm_live_draft`` + the enum/priority/team coercion) and the pure
|
||||
draft/description helpers. DB-backed tests use an in-memory async session via
|
||||
conftest fixtures.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, patch
|
||||
from uuid import UUID, uuid4
|
||||
|
||||
import pytest
|
||||
@@ -28,12 +28,9 @@ from roboco.models.base import (
|
||||
Team,
|
||||
)
|
||||
from roboco.seeds.initial_data import AGENT_UUIDS
|
||||
from roboco.services.base import NotFoundError, ServiceError, ValidationError
|
||||
from roboco.services.base import ServiceError
|
||||
from roboco.services.prompter import (
|
||||
PrompterService,
|
||||
_build_chat_prompt,
|
||||
_build_draft_prompt,
|
||||
_build_reasoning,
|
||||
compose_description,
|
||||
derive_scale,
|
||||
get_prompter_service,
|
||||
@@ -209,41 +206,6 @@ def test_coerce_draft_enums_keeps_valid_and_derives_missing_team() -> None:
|
||||
assert complexity is Complexity.MEDIUM
|
||||
|
||||
|
||||
def test_build_chat_prompt_basic() -> None:
|
||||
messages = [
|
||||
{"role": "user", "content": "I need a feature"},
|
||||
{"role": "assistant", "content": "Tell me more"},
|
||||
]
|
||||
prompt = _build_chat_prompt(messages, None)
|
||||
assert "user: I need a feature" in prompt
|
||||
assert "assistant: Tell me more" in prompt
|
||||
assert "Continue the conversation" in prompt
|
||||
|
||||
|
||||
def test_build_chat_prompt_with_context() -> None:
|
||||
messages = [{"role": "user", "content": "hello"}]
|
||||
prompt = _build_chat_prompt(messages, {"team": "backend"})
|
||||
assert "Context:" in prompt
|
||||
assert "team: backend" in prompt
|
||||
|
||||
|
||||
def test_build_draft_prompt() -> None:
|
||||
messages = [{"role": "user", "content": "I need a login page"}]
|
||||
prompt = _build_draft_prompt(messages, None)
|
||||
assert "valid JSON" in prompt
|
||||
assert "user: I need a login page" in prompt
|
||||
|
||||
|
||||
def test_build_reasoning() -> None:
|
||||
messages = [{"role": "user", "content": "Hello"}] * 3
|
||||
draft = {"title": "My Task", "team": "backend", "estimated_complexity": "medium"}
|
||||
reasoning = _build_reasoning(messages, draft)
|
||||
assert "My Task" in reasoning
|
||||
assert "backend" in reasoning
|
||||
assert "medium" in reasoning
|
||||
assert "3 messages" in reasoning
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Factory
|
||||
# =============================================================================
|
||||
@@ -262,178 +224,10 @@ def test_get_prompter_service_raises_without_db_for_session_methods() -> None:
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Stateless chat / draft (with mocked LLM)
|
||||
# DB-backed: assignee routing + confirm_live_draft
|
||||
# =============================================================================
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_chat_success_with_mock_llm() -> None:
|
||||
service = get_prompter_service()
|
||||
|
||||
with patch.object(
|
||||
service,
|
||||
"_create_message",
|
||||
new_callable=AsyncMock,
|
||||
return_value="Great, let's continue!",
|
||||
):
|
||||
result = await service.chat(
|
||||
messages=[{"role": "user", "content": "I need a feature"}]
|
||||
)
|
||||
|
||||
assert result["message"] == "Great, let's continue!"
|
||||
assert result["draft_ready"] is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_chat_draft_ready_signal() -> None:
|
||||
service = get_prompter_service()
|
||||
|
||||
reply = (
|
||||
"Got it — I have what I need.\n\n"
|
||||
'```roboco-meta\n{"covered": ["objective", "scope", "surface", '
|
||||
'"acceptance"], "ready": true, "scale": "single"}\n```'
|
||||
)
|
||||
with patch.object(
|
||||
service,
|
||||
"_create_message",
|
||||
new_callable=AsyncMock,
|
||||
return_value=reply,
|
||||
):
|
||||
result = await service.chat(
|
||||
messages=[{"role": "user", "content": "I need a feature"}]
|
||||
)
|
||||
|
||||
assert result["draft_ready"] is True
|
||||
assert result["scale"] == "single"
|
||||
# The control block is stripped from the user-visible reply.
|
||||
assert "roboco-meta" not in result["message"]
|
||||
assert result["message"] == "Got it — I have what I need."
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_chat_raises_on_empty_response() -> None:
|
||||
service = get_prompter_service()
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
service, "_create_message", new_callable=AsyncMock, return_value=""
|
||||
),
|
||||
pytest.raises(ServiceError, match="LLM returned empty content"),
|
||||
):
|
||||
await service.chat(messages=[{"role": "user", "content": "Hello"}])
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_chat_raises_on_llm_error() -> None:
|
||||
service = get_prompter_service()
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
service,
|
||||
"_create_message",
|
||||
new_callable=AsyncMock,
|
||||
side_effect=Exception("API unavailable"),
|
||||
),
|
||||
pytest.raises(ServiceError, match="LLM chat failed"),
|
||||
):
|
||||
await service.chat(messages=[{"role": "user", "content": "Hello"}])
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_draft_success_with_mock_llm() -> None:
|
||||
service = get_prompter_service()
|
||||
|
||||
draft_data = {
|
||||
"title": "Add login",
|
||||
"description": "Implement login functionality with JWT tokens",
|
||||
"acceptance_criteria": ["User can log in"],
|
||||
"team": "backend",
|
||||
"task_type": "code",
|
||||
"nature": "technical",
|
||||
"estimated_complexity": "medium",
|
||||
}
|
||||
|
||||
with patch.object(
|
||||
service,
|
||||
"_create_message",
|
||||
new_callable=AsyncMock,
|
||||
return_value=json.dumps(draft_data),
|
||||
):
|
||||
result = await service.draft(
|
||||
messages=[{"role": "user", "content": "I need a login feature"}]
|
||||
)
|
||||
|
||||
assert result["draft"]["title"] == "Add login"
|
||||
assert result["draft"]["source"] == "prompter"
|
||||
assert result["draft"]["confirmed_by_human"] is False
|
||||
assert "reasoning" in result
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_draft_raises_on_invalid_json() -> None:
|
||||
service = get_prompter_service()
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
service,
|
||||
"_create_message",
|
||||
new_callable=AsyncMock,
|
||||
return_value="Not JSON at all",
|
||||
),
|
||||
pytest.raises(ValidationError, match="not valid JSON"),
|
||||
):
|
||||
await service.draft(messages=[{"role": "user", "content": "Hello"}])
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_draft_raises_on_llm_error() -> None:
|
||||
service = get_prompter_service()
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
service,
|
||||
"_create_message",
|
||||
new_callable=AsyncMock,
|
||||
side_effect=Exception("API unavailable"),
|
||||
),
|
||||
pytest.raises(ServiceError, match="LLM draft generation failed"),
|
||||
):
|
||||
await service.draft(messages=[{"role": "user", "content": "Hello"}])
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Session-based: create_session (DB-backed via conftest)
|
||||
# =============================================================================
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_session_db(db_session: Any) -> None:
|
||||
"""create_session persists a PrompterSessionTable row."""
|
||||
service = get_prompter_service(db=db_session)
|
||||
|
||||
agent_id = uuid4()
|
||||
agent = AgentTable(
|
||||
id=agent_id,
|
||||
name="TestAgent",
|
||||
slug=f"test-{uuid4().hex[:8]}",
|
||||
role=AgentRole.DEVELOPER,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="dev",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(agent)
|
||||
await db_session.flush()
|
||||
|
||||
session = await service.create_session(agent_id=agent_id)
|
||||
assert session.id is not None
|
||||
assert session.status == "active"
|
||||
assert session.agent_id == agent_id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_assignee_is_board_distinguishes_roles(db_session: Any) -> None:
|
||||
"""Drives product team routing: a board reviewer keeps the root on the board.
|
||||
@@ -472,45 +266,6 @@ async def test_assignee_is_board_distinguishes_roles(db_session: Any) -> None:
|
||||
assert await service._assignee_is_board(uuid4()) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_session_not_found(db_session: Any) -> None:
|
||||
"""_get_session raises NotFoundError for unknown session ID."""
|
||||
service = get_prompter_service(db=db_session)
|
||||
with pytest.raises(NotFoundError):
|
||||
await service._get_session(uuid4(), uuid4())
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_draft_empty_session_raises(db_session: Any) -> None:
|
||||
"""get_or_generate_draft raises ValidationError if no messages exist."""
|
||||
service = get_prompter_service(db=db_session)
|
||||
|
||||
agent_id = uuid4()
|
||||
agent = AgentTable(
|
||||
id=agent_id,
|
||||
name="TestAgent",
|
||||
slug=f"test-{uuid4().hex[:8]}",
|
||||
role=AgentRole.DEVELOPER,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="dev",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(agent)
|
||||
await db_session.flush()
|
||||
|
||||
session = await service.create_session(agent_id=agent_id)
|
||||
|
||||
with pytest.raises(ValidationError, match="empty conversation"):
|
||||
await service.get_or_generate_draft(
|
||||
session_id=UUID(str(session.id)),
|
||||
agent_id=agent_id,
|
||||
)
|
||||
|
||||
|
||||
async def _seed_project_and_ceo(db_session: Any) -> tuple[UUID, UUID]:
|
||||
"""Seed a system agent + project + CEO; return (project_id, ceo_id).
|
||||
|
||||
|
||||
@@ -1,196 +1,16 @@
|
||||
"""Unit tests for roboco.services.usage_events.
|
||||
|
||||
Covers the _UsageThrottle class and the publish_usage_update /
|
||||
publish_usage_snapshot helpers. No real Redis or event bus is needed —
|
||||
we use AsyncMock to assert that bus.publish is called with the right
|
||||
Covers the publish_usage_snapshot helper. No real Redis or event bus is
|
||||
needed — we use AsyncMock to assert that bus.publish is called with the right
|
||||
payload and type.
|
||||
|
||||
The throttle suppression test is the acceptance-criterion gate:
|
||||
"Server-side throttle prevents more than 1 USAGE_UPDATE publish per
|
||||
agent per 5-second window."
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import UTC, datetime
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
from roboco.services.usage_events import (
|
||||
UsageSnapshot,
|
||||
UsageUpdate,
|
||||
_UsageThrottle,
|
||||
publish_usage_snapshot,
|
||||
publish_usage_update,
|
||||
)
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _UsageThrottle
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_throttle_allows_first_publish() -> None:
|
||||
"""A fresh agent has no prior timestamp — first publish is always allowed."""
|
||||
th = _UsageThrottle(window=5.0)
|
||||
assert th.should_publish("be-dev-1") is True
|
||||
|
||||
|
||||
def test_throttle_suppresses_second_publish_within_window() -> None:
|
||||
"""Second call within the 5-second window returns False (suppressed)."""
|
||||
th = _UsageThrottle(window=5.0)
|
||||
|
||||
with patch("roboco.services.usage_events.time") as mock_time:
|
||||
mock_time.monotonic.return_value = 100.0
|
||||
assert th.should_publish("be-dev-1") is True # first → allowed
|
||||
|
||||
mock_time.monotonic.return_value = 104.9 # 4.9 s later — still inside window
|
||||
assert th.should_publish("be-dev-1") is False # suppressed
|
||||
|
||||
|
||||
def test_throttle_allows_publish_after_window_expires() -> None:
|
||||
"""After the full window elapses, the next publish is allowed again."""
|
||||
th = _UsageThrottle(window=5.0)
|
||||
|
||||
with patch("roboco.services.usage_events.time") as mock_time:
|
||||
mock_time.monotonic.return_value = 100.0
|
||||
assert th.should_publish("be-dev-1") is True # first
|
||||
|
||||
mock_time.monotonic.return_value = 105.0 # exactly 5 s later
|
||||
assert th.should_publish("be-dev-1") is True # window elapsed → allowed
|
||||
|
||||
|
||||
def test_throttle_tracks_agents_independently() -> None:
|
||||
"""Different agents have independent throttle windows."""
|
||||
th = _UsageThrottle(window=5.0)
|
||||
|
||||
with patch("roboco.services.usage_events.time") as mock_time:
|
||||
mock_time.monotonic.return_value = 100.0
|
||||
|
||||
assert th.should_publish("be-dev-1") is True
|
||||
# be-dev-2 has never published, so it is always allowed.
|
||||
assert th.should_publish("be-dev-2") is True
|
||||
|
||||
mock_time.monotonic.return_value = 101.0
|
||||
# be-dev-1 is suppressed; be-dev-2 is also now suppressed.
|
||||
assert th.should_publish("be-dev-1") is False
|
||||
assert th.should_publish("be-dev-2") is False
|
||||
|
||||
|
||||
def test_throttle_records_timestamp_on_allow() -> None:
|
||||
"""should_publish records the current time when it returns True."""
|
||||
th = _UsageThrottle(window=5.0)
|
||||
recorded_at = 200.0
|
||||
|
||||
with patch("roboco.services.usage_events.time") as mock_time:
|
||||
mock_time.monotonic.return_value = recorded_at
|
||||
th.should_publish("be-dev-1")
|
||||
assert th._last["be-dev-1"] == recorded_at
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# publish_usage_update
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_publish_usage_update_calls_bus_publish() -> None:
|
||||
"""First call in a window publishes the event and returns True."""
|
||||
bus = MagicMock()
|
||||
bus.publish = AsyncMock()
|
||||
th = _UsageThrottle(window=5.0)
|
||||
expected_input = 100
|
||||
expected_output = 50
|
||||
|
||||
with patch("roboco.services.usage_events._throttle", th):
|
||||
result = await publish_usage_update(
|
||||
bus,
|
||||
UsageUpdate(
|
||||
agent_id="be-dev-1",
|
||||
task_id="task-abc",
|
||||
input_tokens=expected_input,
|
||||
output_tokens=expected_output,
|
||||
model="claude-sonnet-4-6",
|
||||
),
|
||||
)
|
||||
|
||||
assert result is True
|
||||
bus.publish.assert_awaited_once()
|
||||
event = bus.publish.await_args.args[0]
|
||||
assert event.type.value == "usage.update"
|
||||
assert event.data["agent_id"] == "be-dev-1"
|
||||
assert event.data["task_id"] == "task-abc"
|
||||
assert event.data["input_tokens"] == expected_input
|
||||
assert event.data["output_tokens"] == expected_output
|
||||
assert event.data["model"] == "claude-sonnet-4-6"
|
||||
assert "timestamp" in event.data
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_publish_usage_update_throttle_suppresses_second_call() -> None:
|
||||
"""Second publish within the throttle window is suppressed (returns False)."""
|
||||
bus = MagicMock()
|
||||
bus.publish = AsyncMock()
|
||||
th = _UsageThrottle(window=5.0)
|
||||
|
||||
with (
|
||||
patch("roboco.services.usage_events._throttle", th),
|
||||
patch("roboco.services.usage_events.time") as mock_time,
|
||||
):
|
||||
mock_time.monotonic.return_value = 100.0
|
||||
first = await publish_usage_update(
|
||||
bus,
|
||||
UsageUpdate(
|
||||
agent_id="be-dev-1",
|
||||
task_id=None,
|
||||
input_tokens=10,
|
||||
output_tokens=5,
|
||||
model="sonnet",
|
||||
),
|
||||
)
|
||||
|
||||
mock_time.monotonic.return_value = 102.0 # 2 s later — still suppressed
|
||||
second = await publish_usage_update(
|
||||
bus,
|
||||
UsageUpdate(
|
||||
agent_id="be-dev-1",
|
||||
task_id=None,
|
||||
input_tokens=20,
|
||||
output_tokens=10,
|
||||
model="sonnet",
|
||||
),
|
||||
)
|
||||
|
||||
assert first is True
|
||||
assert second is False
|
||||
# bus.publish should only have been called once.
|
||||
assert bus.publish.await_count == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_publish_usage_update_custom_timestamp() -> None:
|
||||
"""Custom timestamp is passed through to the event data."""
|
||||
bus = MagicMock()
|
||||
bus.publish = AsyncMock()
|
||||
ts = datetime(2026, 6, 11, 12, 0, 0, tzinfo=UTC)
|
||||
|
||||
# Use a fresh throttle so the first publish goes through.
|
||||
th = _UsageThrottle(window=5.0)
|
||||
with patch("roboco.services.usage_events._throttle", th):
|
||||
await publish_usage_update(
|
||||
bus,
|
||||
UsageUpdate(
|
||||
agent_id="be-dev-1",
|
||||
task_id=None,
|
||||
input_tokens=0,
|
||||
output_tokens=0,
|
||||
model="sonnet",
|
||||
timestamp=ts,
|
||||
),
|
||||
)
|
||||
|
||||
event = bus.publish.await_args.args[0]
|
||||
assert event.data["timestamp"] == ts.isoformat()
|
||||
|
||||
from roboco.services.usage_events import UsageSnapshot, publish_usage_snapshot
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# publish_usage_snapshot
|
||||
|
||||
Reference in New Issue
Block a user