From f0eec854d1f7bf4cfc8e42e7b22b99e7d572462d Mon Sep 17 00:00:00 2001 From: Renn F Date: Wed, 6 May 2026 22:25:39 +0200 Subject: [PATCH] Code quality --- roboco/api/routes/dashboard.py | 25 +- roboco/api/routes/git.py | 31 +- roboco/api/routes/optimal.py | 43 +-- roboco/api/routes/tasks.py | 109 +++--- .../services/gateway/choreographer/_impl.py | 14 +- tests/integration/test_a2a_service.py | 318 +++++++----------- tests/integration/test_dashboard_routes.py | 14 +- tests/integration/test_messaging_service.py | 40 +-- tests/integration/test_optimal_routes.py | 22 +- tests/integration/test_tasks_routes.py | 46 +-- .../test_choreographer_impl_branches.py | 36 -- 11 files changed, 283 insertions(+), 415 deletions(-) diff --git a/roboco/api/routes/dashboard.py b/roboco/api/routes/dashboard.py index d2633e3d..bb09bb27 100644 --- a/roboco/api/routes/dashboard.py +++ b/roboco/api/routes/dashboard.py @@ -314,6 +314,21 @@ async def get_ceo_team_details( # ============================================================================= +@router.get("/kanban/main-pm") +async def get_main_pm_kanban( + db: DbSession, +) -> dict[str, Any]: + """Get the Main PM cross-cell kanban board. + + Declared BEFORE `/kanban/{team}` so FastAPI matches the literal + `main-pm` segment instead of treating it as a `Team` enum value + (which would 422 since "main-pm" isn't a Team member). + """ + kanban_service = get_kanban_service(db) + board = await kanban_service.get_main_pm_board_flat() + return board.model_dump() + + @router.get("/kanban/{team}") async def get_team_kanban( team: Team, @@ -359,16 +374,6 @@ async def get_team_kanban( ) -@router.get("/kanban/main-pm") -async def get_main_pm_kanban( - db: DbSession, -) -> dict[str, Any]: - """Get the Main PM cross-cell kanban board.""" - kanban_service = get_kanban_service(db) - board = await kanban_service.get_main_pm_board_flat() - return board.model_dump() - - # ============================================================================= # AGENT STATUS ENDPOINTS # ============================================================================= diff --git a/roboco/api/routes/git.py b/roboco/api/routes/git.py index 9ab27a53..558154bb 100644 --- a/roboco/api/routes/git.py +++ b/roboco/api/routes/git.py @@ -48,7 +48,7 @@ from roboco.api.schemas.git import ( GitPushResponse, GitStatusResponse, ) -from roboco.exceptions import GitCommandError, GitTimeoutError +from roboco.exceptions import GitCommandError, GitError, GitTimeoutError from roboco.logging import get_logger from roboco.services.base import ( NotFoundError, @@ -66,8 +66,15 @@ router = APIRouter() # Expected number of parts in log format output _LOG_FORMAT_PARTS = 5 +# Catch tuple for service-layer errors. `roboco.exceptions.GitError` is a +# distinct class from `roboco.services.base.ServiceError` (it extends the +# `roboco.exceptions.ServiceError` class), so listing both is required for +# git timeouts/command failures to be translated to 504/500 instead of +# bubbling as 500 Internal Server Errors with no `detail`. +_TranslatableError = (ServiceError, GitError) -def _translate_error(e: ServiceError) -> HTTPException: + +def _translate_error(e: ServiceError | GitError) -> HTTPException: """Translate service errors to HTTP exceptions.""" if isinstance(e, NotFoundError): return HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=e.message) @@ -139,7 +146,7 @@ async def get_git_status( ahead, behind, ) = await git_service.get_status(workspace) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e return GitStatusResponse( @@ -190,7 +197,7 @@ async def get_git_log( stderr=log_result.stderr[:200] if log_result.stderr else "", ) return GitLogResponse(project_slug=project_slug, branch=branch, commits=[]) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e commits = [] @@ -237,7 +244,7 @@ async def list_branches( args.append("-a") branch_result = await git_service._run_git(workspace, args) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e branches = [] @@ -296,7 +303,7 @@ async def get_git_diff( if staged: stat_args.append("--staged") stat_result = await git_service._run_git(workspace, stat_args) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e files_changed = stat_result.stdout.count("\n") - 1 if stat_result.stdout else 0 @@ -331,7 +338,7 @@ async def create_commit( insertions, deletions, ) = await git_service.commit_for_task(agent.agent_id, data) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e return GitCommitResponse( @@ -355,7 +362,7 @@ async def push_commits( branch, commits_pushed = await git_service.push_for_task( agent.agent_id, agent.role, data ) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e return GitPushResponse( @@ -381,7 +388,7 @@ async def create_branch( branch_name, created_from = await git_service.create_branch_for_task( agent.agent_id, data ) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e return GitCreateBranchResponse( @@ -407,7 +414,7 @@ async def checkout_branch( git_service = get_git_service(db) try: await git_service.checkout_branch_for_agent(agent.agent_id, data) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e return GitCheckoutResponse( @@ -432,7 +439,7 @@ async def create_pull_request( source_branch, target_branch, ) = await git_service.create_pr_for_task(agent.agent_id, data) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e return GitCreatePRResponse( @@ -456,7 +463,7 @@ async def merge_pull_request( target_branch, merge_commit = await git_service.merge_pr_for_task( agent.agent_id, agent.role, data ) - except ServiceError as e: + except _TranslatableError as e: raise _translate_error(e) from e return GitMergePRResponse( diff --git a/roboco/api/routes/optimal.py b/roboco/api/routes/optimal.py index 5140f8be..e7c82997 100644 --- a/roboco/api/routes/optimal.py +++ b/roboco/api/routes/optimal.py @@ -432,6 +432,29 @@ async def get_stats( ) +@router.get("/stats/staleness") +async def check_staleness( + agent: CurrentAgentContext, + permissions: PermissionServiceDep, +) -> dict[str, Any]: + """ + Check if indexes are stale (source files modified after last indexing). + + Returns staleness info for file-based indexes (CODE, DOCUMENTATION). + + Declared BEFORE `/stats/{index_type}` so FastAPI matches the literal + `staleness` segment instead of treating it as an `index_type` param. + """ + if not permissions.can_perform_kb_action(agent, KBAction.VIEW_STATS): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Not authorized to view index staleness", + ) + + service = await get_optimal_service() + return await service.check_index_staleness() + + @router.get("/stats/{index_type}", response_model=SingleIndexStatsResponse) async def get_single_index_stats( index_type: str, @@ -465,26 +488,6 @@ async def get_single_index_stats( ) -@router.get("/stats/staleness") -async def check_staleness( - agent: CurrentAgentContext, - permissions: PermissionServiceDep, -) -> dict[str, Any]: - """ - Check if indexes are stale (source files modified after last indexing). - - Returns staleness info for file-based indexes (CODE, DOCUMENTATION). - """ - if not permissions.can_perform_kb_action(agent, KBAction.VIEW_STATS): - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Not authorized to view index staleness", - ) - - service = await get_optimal_service() - return await service.check_index_staleness() - - @router.get("/health", response_model=RAGHealthResponse) async def rag_health_check() -> RAGHealthResponse: """Check RAG system health (embedding, LLM, vector store).""" diff --git a/roboco/api/routes/tasks.py b/roboco/api/routes/tasks.py index 2bae4417..ff9d1119 100644 --- a/roboco/api/routes/tasks.py +++ b/roboco/api/routes/tasks.py @@ -115,18 +115,8 @@ async def create_task( detail="Not authorized to create tasks", ) - # Validate: all tasks require project_id - if not data.project_id: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail={ - "error": { - "code": "PROJECT_REQUIRED", - "message": "All tasks require project_id", - "hint": "Specify project_id for the git repository", - } - }, - ) + # `data.project_id` is `UUID` (required) on TaskCreate, so pydantic + # rejects missing/null values with 422 before this handler runs. # Acceptance criteria required — without them, QA has nothing to # verify and the task is structurally unclosable. @@ -389,6 +379,56 @@ async def get_task_stats_by_team( return TaskCountResponse(counts=counts) +# Static-segment routes must be declared BEFORE `/{task_id}` so FastAPI +# matches the literal path instead of treating the segment as a UUID +# (which would 422 on these names). + + +@router.get("/awaiting-pm-review", response_model=list[TaskResponse]) +async def get_awaiting_pm_review_tasks( + db: DbSession, + agent: CurrentAgentContext, + permissions: PermissionServiceDep, + team: Team | None = None, +) -> list[TaskResponse]: + """Get tasks awaiting PM review.""" + service = get_task_service(db) + + # Apply team filter based on permissions + can_view_all = permissions.can_perform_task_action(agent, TaskAction.VIEW_ALL) + effective_team = team if can_view_all else agent.team + + tasks = await service.list_awaiting_pm_review(effective_team) + return task_list_to_response(tasks) + + +@router.get("/awaiting-ceo-approval", response_model=list[TaskResponse]) +async def get_awaiting_ceo_approval_tasks( + db: DbSession, + agent: CurrentAgentContext, + permissions: PermissionServiceDep, +) -> list[TaskResponse]: + """Get tasks awaiting CEO approval. + + CEO approval queue is org-wide (no team filter). + Only visible to PMs and above. + """ + # Only PMs and above can view the CEO approval queue + can_view_all = permissions.can_perform_task_action(agent, TaskAction.VIEW_ALL) + is_pm = agent.role in (AgentRole.CELL_PM, AgentRole.MAIN_PM) + is_ceo = agent.role == AgentRole.CEO + + if not (can_view_all or is_pm or is_ceo): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only PMs and management can view CEO approval queue", + ) + + service = get_task_service(db) + tasks = await service.list_awaiting_ceo_approval() + return task_list_to_response(tasks) + + @router.get("/{task_id}", response_model=TaskResponse) async def get_task( task_id: UUID, @@ -1173,51 +1213,6 @@ async def cancel_task( # ============================================================================= -@router.get("/awaiting-pm-review", response_model=list[TaskResponse]) -async def get_awaiting_pm_review_tasks( - db: DbSession, - agent: CurrentAgentContext, - permissions: PermissionServiceDep, - team: Team | None = None, -) -> list[TaskResponse]: - """Get tasks awaiting PM review.""" - service = get_task_service(db) - - # Apply team filter based on permissions - can_view_all = permissions.can_perform_task_action(agent, TaskAction.VIEW_ALL) - effective_team = team if can_view_all else agent.team - - tasks = await service.list_awaiting_pm_review(effective_team) - return task_list_to_response(tasks) - - -@router.get("/awaiting-ceo-approval", response_model=list[TaskResponse]) -async def get_awaiting_ceo_approval_tasks( - db: DbSession, - agent: CurrentAgentContext, - permissions: PermissionServiceDep, -) -> list[TaskResponse]: - """Get tasks awaiting CEO approval. - - CEO approval queue is org-wide (no team filter). - Only visible to PMs and above. - """ - # Only PMs and above can view the CEO approval queue - can_view_all = permissions.can_perform_task_action(agent, TaskAction.VIEW_ALL) - is_pm = agent.role in (AgentRole.CELL_PM, AgentRole.MAIN_PM) - is_ceo = agent.role == AgentRole.CEO - - if not (can_view_all or is_pm or is_ceo): - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Only PMs and management can view CEO approval queue", - ) - - service = get_task_service(db) - tasks = await service.list_awaiting_ceo_approval() - return task_list_to_response(tasks) - - @router.post("/{task_id}/escalate-to-ceo", response_model=TaskResponse) async def escalate_to_ceo( task_id: UUID, diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 43d5fb13..935ef151 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -1293,15 +1293,13 @@ class Choreographer: parent: Any, inputs: DelegateInputs, ) -> Envelope | None: - """Slug / project_id / enum guards. Pure data-shape checks.""" - from roboco.seeds.initial_data import AGENT_UUIDS + """project_id / enum guards. Pure data-shape checks. - if inputs.assigned_to not in AGENT_UUIDS: - return Envelope.invalid_state( - message=f"unknown agent slug: {inputs.assigned_to!r}", - remediate=f"valid slugs: {sorted(AGENT_UUIDS)}", - context_briefing=await self._briefing_for(pm_agent_id, parent_task_id), - ) + The slug-validity check used to live here, but `_delegate_role_guards` + runs first and `_validate_delegation_chain` rejects any slug outside + the allowed delegation targets — which is a strict subset of + `AGENT_UUIDS` — so any AGENT_UUIDS check here was unreachable. + """ if parent.project_id is None: return Envelope.invalid_state( message="parent task has no project_id", diff --git a/tests/integration/test_a2a_service.py b/tests/integration/test_a2a_service.py index 8fcec201..e34ee76a 100644 --- a/tests/integration/test_a2a_service.py +++ b/tests/integration/test_a2a_service.py @@ -41,10 +41,13 @@ if TYPE_CHECKING: async def a2a_setup( db_session: AsyncSession, ) -> AsyncIterator[dict]: + # Use the canonical seed slugs so the A2A policy matrix recognizes + # role + team and lets same-cell pairs talk. Random suffixes would + # leave them with role="unknown" → policy denies everything. dev = AgentTable( id=uuid4(), name="Dev", - slug=f"be-dev-{uuid4().hex[:8]}", + slug="be-dev-1", role=AgentRole.DEVELOPER, team=Team.BACKEND, status=AgentStatus.ACTIVE, @@ -57,7 +60,7 @@ async def a2a_setup( qa = AgentTable( id=uuid4(), name="QA", - slug=f"be-qa-{uuid4().hex[:8]}", + slug="be-qa", role=AgentRole.QA, team=Team.BACKEND, status=AgentStatus.ACTIVE, @@ -299,14 +302,10 @@ async def test_get_or_create_conversation_self_a2a_denied(a2a_setup: dict) -> No @pytest.mark.asyncio async def test_get_or_create_conversation_creates(a2a_setup: dict) -> None: - """A2A between dev pairs is allowed by default; just exercise the create path.""" + """A2A between two same-cell devs is allowed by the policy.""" svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-dev-2") - assert conv is not None - except Exception: - # If the policy blocks this pair, skip — we're focused on the call path. - pytest.skip("A2A policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-dev-2") + assert conv is not None @pytest.mark.asyncio @@ -442,14 +441,11 @@ async def test_create_conversation_between_dev_and_qa_in_same_cell( ) -> None: """Cell members can A2A within their own cell.""" svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - assert conv is not None - # Idempotent — same agents, same conversation. - again = await svc.get_or_create_conversation("be-dev-1", "be-qa") - assert again.id == conv.id - except Exception: - pytest.skip("Policy denied this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + assert conv is not None + # Idempotent — same agents, same conversation. + again = await svc.get_or_create_conversation("be-dev-1", "be-qa") + assert again.id == conv.id @pytest.mark.asyncio @@ -457,47 +453,35 @@ async def test_send_chat_message_in_existing_conversation( a2a_setup: dict, ) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - msg = await svc.send_chat_message(UUID(conv.id), "be-dev-1", "hello") - assert msg.content == "hello" - except Exception: - pytest.skip("Policy denied this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + msg = await svc.send_chat_message(UUID(conv.id), "be-dev-1", "hello") + assert msg.content == "hello" @pytest.mark.asyncio async def test_get_messages_returns_chronological(a2a_setup: dict) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - cid = UUID(conv.id) - await svc.send_chat_message(cid, "be-dev-1", "first") - await svc.send_chat_message(cid, "be-dev-1", "second") - msgs = await svc.get_messages(cid, "be-dev-1") - _SENT_COUNT = 2 - assert len(msgs) == _SENT_COUNT - except Exception: - pytest.skip("Policy denied this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + cid = UUID(conv.id) + await svc.send_chat_message(cid, "be-dev-1", "first") + await svc.send_chat_message(cid, "be-dev-1", "second") + msgs = await svc.get_messages(cid, "be-dev-1") + _SENT_COUNT = 2 + assert len(msgs) == _SENT_COUNT @pytest.mark.asyncio async def test_close_conversation_with_resolution(a2a_setup: dict) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - await svc.close_conversation(UUID(conv.id), "be-dev-1", resolution="done") - except Exception: - pytest.skip("Policy denied this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + await svc.close_conversation(UUID(conv.id), "be-dev-1", resolution="done") @pytest.mark.asyncio async def test_mark_read_clears_unread(a2a_setup: dict) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - await svc.mark_read(UUID(conv.id), "be-dev-1") - except Exception: - pytest.skip("Policy denied this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + await svc.mark_read(UUID(conv.id), "be-dev-1") @pytest.mark.asyncio @@ -505,12 +489,9 @@ async def test_close_conversation_non_participant_raises( a2a_setup: dict, ) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - with pytest.raises(ValueError, match="Not a participant"): - await svc.close_conversation(UUID(conv.id), "ghost-agent") - except Exception: - pytest.skip("Policy denied this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + with pytest.raises(ValueError, match="Not a participant"): + await svc.close_conversation(UUID(conv.id), "ghost-agent") @pytest.mark.asyncio @@ -518,12 +499,9 @@ async def test_send_chat_message_non_participant_raises( a2a_setup: dict, ) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - with pytest.raises(ValueError, match="Not a participant"): - await svc.send_chat_message(UUID(conv.id), "ghost", "hi") - except Exception: - pytest.skip("Policy denied this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + with pytest.raises(ValueError, match="Not a participant"): + await svc.send_chat_message(UUID(conv.id), "ghost", "hi") # --------------------------------------------------------------------------- @@ -580,9 +558,7 @@ async def test_update_task_with_message_appends_to_notes( ) -> None: """Use a real DB-backed task instance to avoid SA private state issues.""" db = a2a_setup["db"] - task = (await db.execute(select(TaskTable).limit(1))).scalar_one_or_none() - if task is None: - pytest.skip("no task in DB") + task = (await db.execute(select(TaskTable).limit(1))).scalar_one() original_notes = task.dev_notes task.dev_notes = "existing notes" msg = A2AMessage(role="user", parts=[TextPart(text="new message")]) @@ -597,9 +573,7 @@ async def test_update_task_with_message_no_text_parts_noop( a2a_setup: dict, ) -> None: db = a2a_setup["db"] - task = (await db.execute(select(TaskTable).limit(1))).scalar_one_or_none() - if task is None: - pytest.skip("no task in DB") + task = (await db.execute(select(TaskTable).limit(1))).scalar_one() original = task.dev_notes task.dev_notes = "existing" msg = A2AMessage(role="user", parts=[]) @@ -1198,11 +1172,8 @@ async def test_list_conversations_with_status_and_task_filter( async def test_list_conversations_with_messages(a2a_setup: dict) -> None: """Seed a conversation + a message so the last_message preview path runs.""" svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - await svc.send_chat_message(UUID(conv.id), "be-dev-1", "preview text") - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + await svc.send_chat_message(UUID(conv.id), "be-dev-1", "preview text") convs = await svc.list_conversations("be-dev-1") # last_message_preview should be truthy for this conv. @@ -1218,19 +1189,23 @@ async def test_list_conversations_with_messages(a2a_setup: dict) -> None: async def test_send_chat_message_options_response_to( a2a_setup: dict, ) -> None: - """response_to_id and requires_response options surface on the message.""" + """response_to_id and requires_response options surface on the message. + + `response_to_id` has a FK to `a2a_messages.id`, so we send a real + "first" message to thread off — passing a random UUID would hit FK + violation. + """ svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - msg = await svc.send_chat_message( - UUID(conv.id), - "be-dev-1", - "needs answer", - options={"requires_response": True, "response_to_id": _u()}, - ) - assert msg.requires_response is True - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + first = await svc.send_chat_message(UUID(conv.id), "be-qa", "first message") + msg = await svc.send_chat_message( + UUID(conv.id), + "be-dev-1", + "needs answer", + options={"requires_response": True, "response_to_id": UUID(first.id)}, + ) + assert msg.requires_response is True + assert msg.response_to_id == first.id @pytest.mark.asyncio @@ -1238,18 +1213,15 @@ async def test_send_chat_message_from_agent_b_increments_unread_a( a2a_setup: dict, ) -> None: svc = a2a_setup["svc"] - try: - # First exchange establishes canonical pair (a < b lexicographically). - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - # Send from whichever agent is conv.agent_b (the "other" side). - result = await svc.session.execute( - _sel(A2AConversationTable).where(A2AConversationTable.id == UUID(conv.id)) - ) - row = result.scalar_one() - msg = await svc.send_chat_message(UUID(conv.id), row.agent_b, "hi from b") - assert msg.from_agent == row.agent_b - except Exception: - pytest.skip("Policy denies this pair") + # First exchange establishes canonical pair (a < b lexicographically). + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + # Send from whichever agent is conv.agent_b (the "other" side). + result = await svc.session.execute( + _sel(A2AConversationTable).where(A2AConversationTable.id == UUID(conv.id)) + ) + row = result.scalar_one() + msg = await svc.send_chat_message(UUID(conv.id), row.agent_b, "hi from b") + assert msg.from_agent == row.agent_b # --------------------------------------------------------------------------- @@ -1262,26 +1234,20 @@ async def test_get_messages_non_participant_returns_empty( a2a_setup: dict, ) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - msgs = await svc.get_messages(UUID(conv.id), "ghost-agent") - assert msgs == [] - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + msgs = await svc.get_messages(UUID(conv.id), "ghost-agent") + assert msgs == [] @pytest.mark.asyncio async def test_get_messages_with_before_filter(a2a_setup: dict) -> None: """Pass a `before` datetime — exercises the filter branch.""" svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - await svc.send_chat_message(UUID(conv.id), "be-dev-1", "first") - future = datetime.now(UTC).replace(year=2099) - msgs = await svc.get_messages(UUID(conv.id), "be-dev-1", before=future) - assert isinstance(msgs, list) - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + await svc.send_chat_message(UUID(conv.id), "be-dev-1", "first") + future = datetime.now(UTC).replace(year=2099) + msgs = await svc.get_messages(UUID(conv.id), "be-dev-1", before=future) + assert isinstance(msgs, list) # --------------------------------------------------------------------------- @@ -1292,26 +1258,20 @@ async def test_get_messages_with_before_filter(a2a_setup: dict) -> None: @pytest.mark.asyncio async def test_mark_read_non_participant(a2a_setup: dict) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - # Non-participant → silent return. - await svc.mark_read(UUID(conv.id), "ghost") - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + # Non-participant → silent return. + await svc.mark_read(UUID(conv.id), "ghost") @pytest.mark.asyncio async def test_mark_read_as_agent_b(a2a_setup: dict) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - result = await svc.session.execute( - _sel(A2AConversationTable).where(A2AConversationTable.id == UUID(conv.id)) - ) - row = result.scalar_one() - await svc.mark_read(UUID(conv.id), row.agent_b) - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + result = await svc.session.execute( + _sel(A2AConversationTable).where(A2AConversationTable.id == UUID(conv.id)) + ) + row = result.scalar_one() + await svc.mark_read(UUID(conv.id), row.agent_b) # --------------------------------------------------------------------------- @@ -1322,16 +1282,13 @@ async def test_mark_read_as_agent_b(a2a_setup: dict) -> None: @pytest.mark.asyncio async def test_list_pairs_with_conversations(a2a_setup: dict) -> None: svc = a2a_setup["svc"] - try: - await svc.get_or_create_conversation("be-dev-1", "be-qa") - pairs = await svc.list_pairs("be-dev-1") - assert any( - (p.agent_a, p.agent_b) == ("be-dev-1", "be-qa") - or (p.agent_a, p.agent_b) == ("be-qa", "be-dev-1") - for p in pairs - ) - except Exception: - pytest.skip("Policy denies this pair") + await svc.get_or_create_conversation("be-dev-1", "be-qa") + pairs = await svc.list_pairs("be-dev-1") + assert any( + (p.agent_a, p.agent_b) == ("be-dev-1", "be-qa") + or (p.agent_a, p.agent_b) == ("be-qa", "be-dev-1") + for p in pairs + ) # --------------------------------------------------------------------------- @@ -1367,17 +1324,14 @@ async def test_send_gateway_adapter_uuid_to_uuid( svc = a2a_setup["svc"] dev = a2a_setup["dev"] qa = a2a_setup["qa"] - try: - msg = await svc.send( - from_agent=dev.id, - to_agent=qa.id, - task_id=a2a_setup["task_id"], - body="hello", - skill="general", - ) - assert msg.content == "hello" - except Exception: - pytest.skip("Policy denies this pair") + msg = await svc.send( + from_agent=dev.id, + to_agent=qa.id, + task_id=a2a_setup["task_id"], + body="hello", + skill="general", + ) + assert msg.content == "hello" @pytest.mark.asyncio @@ -1387,16 +1341,13 @@ async def test_send_gateway_adapter_with_string_recipient( """Recipient as slug string → no DB lookup for it.""" svc = a2a_setup["svc"] dev = a2a_setup["dev"] - try: - msg = await svc.send( - from_agent=dev.id, - to_agent="be-qa", - task_id=a2a_setup["task_id"], - body="hello", - ) - assert msg.content == "hello" - except Exception: - pytest.skip("Policy denies this pair") + msg = await svc.send( + from_agent=dev.id, + to_agent="be-qa", + task_id=a2a_setup["task_id"], + body="hello", + ) + assert msg.content == "hello" # --------------------------------------------------------------------------- @@ -1458,12 +1409,9 @@ async def test_get_or_create_conversation_with_topic( a2a_setup: dict, ) -> None: svc = a2a_setup["svc"] - try: - a = await svc.get_or_create_conversation("be-dev-1", "be-qa", topic="Bug X") - b = await svc.get_or_create_conversation("be-dev-1", "be-qa", topic="Bug X") - assert a.id == b.id - except Exception: - pytest.skip("Policy denies this pair") + a = await svc.get_or_create_conversation("be-dev-1", "be-qa", topic="Bug X") + b = await svc.get_or_create_conversation("be-dev-1", "be-qa", topic="Bug X") + assert a.id == b.id # --------------------------------------------------------------------------- @@ -1474,12 +1422,9 @@ async def test_get_or_create_conversation_with_topic( @pytest.mark.asyncio async def test_get_conversation_non_participant(a2a_setup: dict) -> None: svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - result = await svc.get_conversation(UUID(conv.id), "ghost") - assert result is None - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + result = await svc.get_conversation(UUID(conv.id), "ghost") + assert result is None @pytest.mark.asyncio @@ -1488,13 +1433,10 @@ async def test_get_conversation_returns_model_when_participant( ) -> None: """Participant access → returns the conversation model.""" svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - result = await svc.get_conversation(UUID(conv.id), "be-dev-1") - assert result is not None - assert result.id == conv.id - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + result = await svc.get_conversation(UUID(conv.id), "be-dev-1") + assert result is not None + assert result.id == conv.id # --------------------------------------------------------------------------- @@ -1506,18 +1448,15 @@ async def test_get_conversation_returns_model_when_participant( async def test_get_inbox_summary_with_unread(a2a_setup: dict) -> None: """Send a message from a2 → a1 has unread.""" svc = a2a_setup["svc"] - try: - conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") - result = await svc.session.execute( - _sel(A2AConversationTable).where(A2AConversationTable.id == UUID(conv.id)) - ) - row = result.scalar_one() - # Send from agent_b → agent_a unread increments. - await svc.send_chat_message(UUID(conv.id), row.agent_b, "hi") - inbox = await svc.get_inbox_summary(row.agent_a) - assert inbox.total_unread >= 1 - except Exception: - pytest.skip("Policy denies this pair") + conv = await svc.get_or_create_conversation("be-dev-1", "be-qa") + result = await svc.session.execute( + _sel(A2AConversationTable).where(A2AConversationTable.id == UUID(conv.id)) + ) + row = result.scalar_one() + # Send from agent_b → agent_a unread increments. + await svc.send_chat_message(UUID(conv.id), row.agent_b, "hi") + inbox = await svc.get_inbox_summary(row.agent_a) + assert inbox.total_unread >= 1 # --------------------------------------------------------------------------- @@ -1532,16 +1471,13 @@ async def test_send_gateway_adapter_skill_none( """skill=None branch → options dict stays empty.""" svc = a2a_setup["svc"] dev = a2a_setup["dev"] - try: - msg = await svc.send( - from_agent=dev.id, - to_agent="be-qa", - task_id=a2a_setup["task_id"], - body="no skill", - ) - assert msg.content == "no skill" - except Exception: - pytest.skip("Policy denies this pair") + msg = await svc.send( + from_agent=dev.id, + to_agent="be-qa", + task_id=a2a_setup["task_id"], + body="no skill", + ) + assert msg.content == "no skill" @pytest.mark.asyncio diff --git a/tests/integration/test_dashboard_routes.py b/tests/integration/test_dashboard_routes.py index da827faf..c79d62d6 100644 --- a/tests/integration/test_dashboard_routes.py +++ b/tests/integration/test_dashboard_routes.py @@ -236,13 +236,15 @@ async def test_get_ceo_velocity(dashboard_client: AsyncClient) -> None: @pytest.mark.asyncio -async def test_get_main_pm_kanban_unreachable( - dashboard_client: AsyncClient, -) -> None: - """Route order quirk: /kanban/{team} matches first; main-pm is unreachable.""" +async def test_get_main_pm_kanban_via_http(dashboard_client: AsyncClient) -> None: + """`/kanban/main-pm` is now declared before `/kanban/{team}`, so it routes + correctly to `get_main_pm_kanban` instead of being matched as + `team=main-pm` (which would 422).""" response = await dashboard_client.get("/api/dashboard/kanban/main-pm", headers=_HDR) - # Matched as /kanban/{team} with team=main-pm → invalid Team enum - assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY + assert response.status_code == HTTPStatus.OK + body = response.json() + # main_pm board has columns; shape is from KanbanBoard.model_dump(). + assert "columns" in body @pytest.mark.asyncio diff --git a/tests/integration/test_messaging_service.py b/tests/integration/test_messaging_service.py index e00fa01a..88dabd2a 100644 --- a/tests/integration/test_messaging_service.py +++ b/tests/integration/test_messaging_service.py @@ -2355,46 +2355,12 @@ async def test_get_or_create_active_session_returns_existing_active( # --------------------------------------------------------------------------- -# _walk_task_ancestors — cycle-safety + parent missing +# (orphan-parent walk is covered by test_walk_task_ancestors_orphan_parent_breaks +# elsewhere in this file, which patches session.execute to bypass the FK +# constraint that prevents inserting a real orphan row.) # --------------------------------------------------------------------------- -@pytest.mark.asyncio -async def test_walk_task_ancestors_orphan_parent( - msg_setup: dict, db_session: AsyncSession -) -> None: - """parent_task_id points at non-existent task → walk stops.""" - aid = msg_setup["agent_id"] - base_task_result = await db_session.execute( - __import__("sqlalchemy") - .select(TaskTable) - .where(TaskTable.id == msg_setup["task_id"]) - ) - base_task = base_task_result.scalar_one() - ghost_parent = uuid4() - child = TaskTable( - id=uuid4(), - title="child-orphan", - description="d", - acceptance_criteria=["ac"], - status=TaskStatus.PENDING, - priority=2, - task_type=TaskType.CODE, - nature=TaskNature.TECHNICAL, - project_id=base_task.project_id, - created_by=aid, - team=base_task.team, - parent_task_id=ghost_parent, - ) - db_session.add(child) - # parent_task_id has FK to tasks; using a non-existent id will fail FK. - # Skip rather than committing — exercise the seen-set/path differently. - try: - await db_session.flush() - except Exception: - pytest.skip("FK enforces parent existence — orphan branch unreachable here") - - # --------------------------------------------------------------------------- # _resolve_group_for_session — auto-create default group when channel empty # --------------------------------------------------------------------------- diff --git a/tests/integration/test_optimal_routes.py b/tests/integration/test_optimal_routes.py index c2fdb76e..41c3dc8e 100644 --- a/tests/integration/test_optimal_routes.py +++ b/tests/integration/test_optimal_routes.py @@ -330,13 +330,21 @@ async def test_get_single_stats_success(optimal_client: AsyncClient) -> None: @pytest.mark.asyncio -async def test_check_staleness_unreachable_via_get( - optimal_client: AsyncClient, -) -> None: - """Route order quirk: /stats/{index_type} matches first → 400 invalid type.""" - response = await optimal_client.get("/api/optimal/stats/staleness", headers=_HDR) - # Matched as stats/{index_type} with type=staleness → invalid - assert response.status_code == HTTPStatus.BAD_REQUEST +async def test_check_staleness_via_http(optimal_client: AsyncClient) -> None: + """`/stats/staleness` is now declared before `/stats/{index_type}`, so it + routes correctly to `check_staleness` instead of being matched as + `index_type=staleness` (which would 400 as invalid IndexType).""" + with patch("roboco.api.routes.optimal.get_optimal_service") as mock_get: + mock_service = AsyncMock() + mock_service.check_index_staleness = AsyncMock( + return_value={"stale": False, "indexes": {}} + ) + mock_get.return_value = mock_service + response = await optimal_client.get( + "/api/optimal/stats/staleness", headers=_HDR + ) + assert response.status_code == HTTPStatus.OK + assert response.json() == {"stale": False, "indexes": {}} @pytest.mark.asyncio diff --git a/tests/integration/test_tasks_routes.py b/tests/integration/test_tasks_routes.py index 62984a04..bd2cb7e6 100644 --- a/tests/integration/test_tasks_routes.py +++ b/tests/integration/test_tasks_routes.py @@ -15,7 +15,6 @@ from httpx import ASGITransport, AsyncClient from roboco.api.deps import get_agent_context, get_db from roboco.api.routes.tasks import ( _translate_error, - create_task, get_awaiting_ceo_approval_tasks, get_awaiting_pm_review_tasks, ) @@ -31,7 +30,6 @@ from roboco.models.base import ( TaskType, ) from roboco.models.permissions import AgentContext -from roboco.models.task import TaskCreate from roboco.services.base import ( NotFoundError, ServiceError, @@ -1385,36 +1383,22 @@ async def test_list_tasks_developer_with_team_filters_to_own( @pytest.mark.asyncio -async def test_create_task_project_required_branch(task_client: dict) -> None: - """create_task PROJECT_REQUIRED branch — pydantic normally enforces UUID, - but invoke the helper directly with `model_construct` to cover the inline - `if not data.project_id` 400-raising branch. - """ - bypassed = TaskCreate.model_construct( - title="T", - description="d", - acceptance_criteria=["a"], - team=Team.BACKEND, - project_id=None, - priority=2, - nature=TaskNature.TECHNICAL, - task_type=TaskType.CODE, - sequence=0, - dependency_ids=[], +async def test_create_task_missing_project_id_returns_422(task_client: dict) -> None: + """`TaskCreate.project_id` is `UUID` (required); pydantic rejects missing + value with 422 before the route runs. (The previously-dead inline runtime + `if not data.project_id` branch was removed.)""" + response = await task_client["client"].post( + "/api/tasks", + json={ + "title": "T", + "description": "d", + "acceptance_criteria": ["a"], + "team": "backend", + # project_id intentionally missing + }, + headers=_HDR, ) - agent_ctx = AgentContext( - agent_id=task_client["agent"].id, role=AgentRole.MAIN_PM, team=None - ) - permissions = PermissionService() - with pytest.raises(HTTPException) as exc_info: - await create_task( - data=bypassed, - db=task_client["db"], - agent=agent_ctx, - permissions=permissions, - ) - assert exc_info.value.status_code == HTTPStatus.BAD_REQUEST - assert "PROJECT_REQUIRED" in str(exc_info.value.detail) + assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY # --------------------------------------------------------------------------- diff --git a/tests/unit/gateway/test_choreographer_impl_branches.py b/tests/unit/gateway/test_choreographer_impl_branches.py index 2c87b3be..7f64bf04 100644 --- a/tests/unit/gateway/test_choreographer_impl_branches.py +++ b/tests/unit/gateway/test_choreographer_impl_branches.py @@ -344,42 +344,6 @@ async def test_delegate_unknown_role_rejected() -> None: # --------------------------------------------------------------------------- -@pytest.mark.asyncio -async def test_delegate_static_guard_unknown_slug_via_helper() -> None: - """Lines 1299-1304: unknown slug rejection in _delegate_static_guards. - - Reached by calling the private helper directly — the public ``delegate`` - path is shielded by the chain validator which catches all slugs that - are not in the explicit cell_pm/main_pm target sets first. - """ - pm_id = uuid4() - parent_id = uuid4() - parent = MagicMock( - status="in_progress", - assigned_to=pm_id, - project_id=uuid4(), - title="p", - ) - task_svc = AsyncMock() - task_svc.get.return_value = parent - deps = _make_deps(task=task_svc) - c = Choreographer(deps) - env = await c._delegate_static_guards( - pm_id, - parent_id, - parent, - DelegateInputs( - title="x", - description="y", - assigned_to="ghost-agent", - team="backend", - ), - ) - body = env.as_dict() - assert body["error"] == "invalid_state" - assert "unknown agent slug" in body["message"] - - @pytest.mark.asyncio async def test_delegate_parent_no_project_rejected() -> None: """Line 1306: parent.project_id is None → invalid_state."""