mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
It works pretty nicely with the ultimate test
This commit is contained in:
+10
-13
@@ -25,12 +25,15 @@ from mcp.server.fastmcp import FastMCP
|
||||
|
||||
from roboco.agents_config import (
|
||||
NOTIFICATION_PERMISSIONS,
|
||||
VALID_NOTIFICATION_PRIORITIES,
|
||||
VALID_NOTIFICATION_TYPES,
|
||||
can_send_notifications,
|
||||
get_agent_cell,
|
||||
get_agent_role,
|
||||
)
|
||||
from roboco.mcp.schemas import SendNotificationInput
|
||||
from roboco.mcp.utils import ApiClient, format_error_response
|
||||
from roboco.models.base import NotificationPriority, NotificationType
|
||||
|
||||
# =============================================================================
|
||||
# HELPER FUNCTIONS
|
||||
@@ -82,13 +85,6 @@ def _can_send_notification(sender_id: str, recipient_id: str) -> tuple[bool, str
|
||||
return can_send, reason
|
||||
|
||||
|
||||
# Valid notification types and priorities
|
||||
VALID_NOTIFICATION_TYPES = frozenset(
|
||||
["info", "alert", "task", "escalation", "approval"]
|
||||
)
|
||||
VALID_PRIORITIES = frozenset(["low", "normal", "high", "urgent"])
|
||||
|
||||
|
||||
def _validate_notification_type(notification_type: str) -> dict[str, Any] | None:
|
||||
"""Validate notification type. Returns error dict or None if valid."""
|
||||
if notification_type not in VALID_NOTIFICATION_TYPES:
|
||||
@@ -101,10 +97,11 @@ def _validate_notification_type(notification_type: str) -> dict[str, Any] | None
|
||||
|
||||
def _validate_priority(priority: str) -> dict[str, Any] | None:
|
||||
"""Validate priority. Returns error dict or None if valid."""
|
||||
if priority not in VALID_PRIORITIES:
|
||||
if priority not in VALID_NOTIFICATION_PRIORITIES:
|
||||
valid = sorted(VALID_NOTIFICATION_PRIORITIES)
|
||||
return format_error_response(
|
||||
"INVALID_PRIORITY",
|
||||
f"Invalid priority. Must be one of: {sorted(VALID_PRIORITIES)}",
|
||||
f"Invalid priority. Must be one of: {valid}",
|
||||
)
|
||||
return None
|
||||
|
||||
@@ -392,8 +389,8 @@ def create_notify_mcp_server(agent_id: str) -> FastMCP:
|
||||
recipients=[escalate_to],
|
||||
subject=f"[ESCALATION] {subject}",
|
||||
body=description,
|
||||
notification_type="escalation",
|
||||
priority="high",
|
||||
notification_type=NotificationType.BLOCKER_ESCALATION.value,
|
||||
priority=NotificationPriority.HIGH.value,
|
||||
requires_ack=True,
|
||||
related_task_id=task_id,
|
||||
)
|
||||
@@ -416,8 +413,8 @@ def create_notify_mcp_server(agent_id: str) -> FastMCP:
|
||||
recipients=[approver],
|
||||
subject=f"[APPROVAL NEEDED] {subject}",
|
||||
body=what_needs_approval,
|
||||
notification_type="approval",
|
||||
priority="normal",
|
||||
notification_type=NotificationType.REVIEW_REQUEST.value,
|
||||
priority=NotificationPriority.NORMAL.value,
|
||||
requires_ack=True,
|
||||
related_task_id=task_id,
|
||||
)
|
||||
|
||||
@@ -551,7 +551,11 @@ def _register_pm_completion_tools(
|
||||
"""Register PM-only task completion tools."""
|
||||
|
||||
@mcp.tool()
|
||||
async def roboco_task_complete(task_id: str) -> dict[str, Any]:
|
||||
async def roboco_task_complete(
|
||||
task_id: str,
|
||||
force_with_cancelled: bool = False,
|
||||
justification: str | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""
|
||||
Mark task as completed (PM only).
|
||||
|
||||
@@ -561,14 +565,24 @@ def _register_pm_completion_tools(
|
||||
ENFORCEMENT:
|
||||
- Only PMs can use this tool
|
||||
- Task must be in 'awaiting_pm_review' status
|
||||
- All subtasks must be completed (or use force_with_cancelled)
|
||||
|
||||
PM Override for cancelled subtasks:
|
||||
If some subtasks were cancelled but PM judges work is done anyway,
|
||||
use force_with_cancelled=True with justification explaining why.
|
||||
Only works if ALL non-completed subtasks are cancelled.
|
||||
|
||||
Args:
|
||||
task_id: The task UUID
|
||||
force_with_cancelled: Override cancelled subtask check
|
||||
justification: Required when force_with_cancelled=True
|
||||
|
||||
Returns:
|
||||
Completed task
|
||||
"""
|
||||
return await handle_task_complete(client, task_id, agent_id)
|
||||
return await handle_task_complete(
|
||||
client, task_id, agent_id, force_with_cancelled, justification
|
||||
)
|
||||
|
||||
|
||||
def _register_pm_tools(mcp: FastMCP, client: ApiClient, agent_id: str) -> None:
|
||||
|
||||
@@ -61,8 +61,13 @@ async def handle_docs_complete(
|
||||
):
|
||||
return error
|
||||
|
||||
payload = {"notes": doc_notes} if doc_notes else {}
|
||||
docs_resp = await client.post(f"/tasks/{task_id}/docs-complete", json=payload)
|
||||
# Only send payload if doc_notes provided (QANotes.notes is required)
|
||||
if doc_notes:
|
||||
docs_resp = await client.post(
|
||||
f"/tasks/{task_id}/docs-complete", json={"notes": doc_notes}
|
||||
)
|
||||
else:
|
||||
docs_resp = await client.post(f"/tasks/{task_id}/docs-complete")
|
||||
|
||||
if not docs_resp.ok:
|
||||
return format_error_response(
|
||||
@@ -98,26 +103,23 @@ async def _check_children_completed(
|
||||
Returns error if any children are not completed, None if OK.
|
||||
"""
|
||||
try:
|
||||
# Fetch children/subtasks for this task
|
||||
resp = await client.get(f"/tasks/{task_id}/subtasks")
|
||||
if not resp.ok:
|
||||
# If endpoint doesn't exist or fails, skip check (backwards compat)
|
||||
return None
|
||||
|
||||
subtasks = resp.json()
|
||||
if not subtasks:
|
||||
return None # No children, OK to complete
|
||||
return None
|
||||
|
||||
incomplete: list[dict[str, str]] = []
|
||||
for subtask in subtasks:
|
||||
subtask_status = subtask.get("status")
|
||||
# ONLY "completed" is acceptable - cancelled/pending/etc. block completion
|
||||
if subtask_status != "completed":
|
||||
incomplete.append({
|
||||
"id": str(subtask.get("id", "unknown")),
|
||||
"title": subtask.get("title", "Untitled"),
|
||||
"status": subtask_status or "unknown",
|
||||
})
|
||||
incomplete = [
|
||||
{
|
||||
"id": str(subtask.get("id", "unknown")),
|
||||
"title": subtask.get("title", "Untitled"),
|
||||
"status": subtask.get("status") or "unknown",
|
||||
}
|
||||
for subtask in subtasks
|
||||
if subtask.get("status") != "completed"
|
||||
]
|
||||
|
||||
if incomplete:
|
||||
return format_error_response(
|
||||
@@ -134,18 +136,59 @@ async def _check_children_completed(
|
||||
|
||||
return None
|
||||
except Exception:
|
||||
# If check fails for any reason, allow completion (backwards compat)
|
||||
return None
|
||||
|
||||
|
||||
async def _validate_children_or_force(
|
||||
client: ApiClient, task_id: str, force: bool, justification: str | None
|
||||
) -> dict[str, Any] | None:
|
||||
"""Validate children completion or force override. Returns error or None."""
|
||||
if force:
|
||||
if not justification:
|
||||
return format_error_response(
|
||||
"JUSTIFICATION_REQUIRED",
|
||||
"force_with_cancelled requires justification explaining "
|
||||
"why cancelled subtasks don't block completion.",
|
||||
)
|
||||
return None
|
||||
return await _check_children_completed(client, task_id)
|
||||
|
||||
|
||||
def _validate_completion_status(
|
||||
task: dict[str, Any], agent_id: str
|
||||
) -> dict[str, Any] | None:
|
||||
"""Validate task is in completable status. Returns error or None."""
|
||||
current_status = task.get("status")
|
||||
is_own_task = current_status == "in_progress" and _is_pm_own_task(task, agent_id)
|
||||
is_review_task = current_status == "awaiting_pm_review"
|
||||
|
||||
if is_own_task or is_review_task:
|
||||
return None
|
||||
|
||||
return format_error_response(
|
||||
"INVALID_STATE",
|
||||
f"Cannot complete task in '{current_status}' status. "
|
||||
"Expected 'awaiting_pm_review' (dev work) or 'in_progress' (own task).",
|
||||
{"current_status": current_status},
|
||||
)
|
||||
|
||||
|
||||
async def handle_task_complete(
|
||||
client: ApiClient, task_id: str, agent_id: str
|
||||
client: ApiClient,
|
||||
task_id: str,
|
||||
agent_id: str,
|
||||
force_with_cancelled: bool = False,
|
||||
justification: str | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""Handle task completion (PM only).
|
||||
|
||||
Two completion paths:
|
||||
1. Completing developer work: task must be in 'awaiting_pm_review'
|
||||
2. Completing PM's own task: task can be in 'in_progress' if assigned to PM
|
||||
|
||||
PM Override for cancelled subtasks:
|
||||
Use force_with_cancelled=True with justification to complete despite
|
||||
cancelled subtasks. Only works if ALL non-completed children are cancelled.
|
||||
"""
|
||||
if error := _validate_pm_role(agent_id, "complete tasks"):
|
||||
return error
|
||||
@@ -155,25 +198,19 @@ async def handle_task_complete(
|
||||
return error
|
||||
assert task is not None
|
||||
|
||||
current_status = task.get("status")
|
||||
|
||||
# PM completing their own task - allow from in_progress
|
||||
if current_status == "in_progress" and _is_pm_own_task(task, agent_id):
|
||||
pass # Valid - PM completing their own work
|
||||
# Normal path - developer work went through QA/Docs
|
||||
elif current_status != "awaiting_pm_review":
|
||||
return format_error_response(
|
||||
"INVALID_STATE",
|
||||
f"Cannot complete task in '{current_status}' status. "
|
||||
"Expected 'awaiting_pm_review' (dev work) or 'in_progress' (own task).",
|
||||
{"current_status": current_status},
|
||||
)
|
||||
|
||||
# Check all children are completed before allowing parent completion
|
||||
if error := await _check_children_completed(client, task_id):
|
||||
if error := _validate_completion_status(task, agent_id):
|
||||
return error
|
||||
|
||||
complete_resp = await client.post(f"/tasks/{task_id}/complete")
|
||||
if error := await _validate_children_or_force(
|
||||
client, task_id, force_with_cancelled, justification
|
||||
):
|
||||
return error
|
||||
|
||||
payload: dict[str, Any] = {}
|
||||
if force_with_cancelled:
|
||||
payload = {"force_with_cancelled": True, "justification": justification}
|
||||
|
||||
complete_resp = await client.post(f"/tasks/{task_id}/complete", json=payload)
|
||||
if not complete_resp.ok:
|
||||
return format_error_response(
|
||||
"COMPLETE_FAILED",
|
||||
|
||||
@@ -77,6 +77,60 @@ def validate_assignee_can_work_on_team(
|
||||
return None
|
||||
|
||||
|
||||
def get_role_mismatch_warning(task: dict[str, Any], assignee: str) -> str | None:
|
||||
"""Check if assignee role matches the task type and return warning if mismatch.
|
||||
|
||||
Returns a warning string or None if no mismatch.
|
||||
This is a SOFT warning, not a blocking error - PM has flexibility.
|
||||
"""
|
||||
assignee_role = get_agent_role(assignee)
|
||||
task_status = task.get("status")
|
||||
task_title = task.get("title", "").lower()
|
||||
|
||||
# Role suggestions based on task status
|
||||
status_role_map = {
|
||||
"awaiting_qa": "qa",
|
||||
"awaiting_documentation": "documenter",
|
||||
"awaiting_pm_review": ("cell_pm", "main_pm"),
|
||||
}
|
||||
|
||||
# Check status-based role matching
|
||||
if task_status in status_role_map:
|
||||
expected = status_role_map[task_status]
|
||||
if isinstance(expected, tuple):
|
||||
if assignee_role not in expected:
|
||||
return (
|
||||
f"Task is {task_status} - typically assigned to "
|
||||
f"{' or '.join(expected)}, not {assignee_role}."
|
||||
)
|
||||
elif assignee_role != expected:
|
||||
return (
|
||||
f"Task is {task_status} - typically assigned to "
|
||||
f"{expected}, not {assignee_role}."
|
||||
)
|
||||
|
||||
# Check title-based hints for pending tasks
|
||||
if task_status == "pending":
|
||||
# QA-related keywords in title
|
||||
qa_keywords = ["qa", "test", "validation", "quality", "review"]
|
||||
if any(kw in task_title for kw in qa_keywords) and assignee_role != "qa":
|
||||
return (
|
||||
f"Task title suggests QA work but assignee is {assignee_role}. "
|
||||
"Consider assigning to a QA agent."
|
||||
)
|
||||
|
||||
# Docs-related keywords in title
|
||||
doc_keywords = ["doc", "documentation", "readme", "guide", "reference"]
|
||||
is_doc_task = any(kw in task_title for kw in doc_keywords)
|
||||
if is_doc_task and assignee_role != "documenter":
|
||||
return (
|
||||
f"Task title suggests documentation but assignee is {assignee_role}. "
|
||||
"Consider assigning to a documenter."
|
||||
)
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def validate_cell_pm_assignment(
|
||||
role: str,
|
||||
agent_team: str | None,
|
||||
@@ -275,6 +329,13 @@ async def handle_task_create(
|
||||
task = assigned_task
|
||||
|
||||
guidance = _format_create_guidance(task, input_data.assigned_to)
|
||||
|
||||
# Check for role mismatch and add warning if found
|
||||
if input_data.assigned_to:
|
||||
role_warning = get_role_mismatch_warning(task, input_data.assigned_to)
|
||||
if role_warning:
|
||||
guidance += f"\n\n⚠️ ROLE WARNING: {role_warning}"
|
||||
|
||||
return format_task_response(task, "CREATED", guidance)
|
||||
|
||||
|
||||
@@ -312,6 +373,12 @@ async def handle_task_assign(
|
||||
f"Task assigned to {input_data.assignee} and set to pending. "
|
||||
"Orchestrator will spawn them to claim and work on it."
|
||||
)
|
||||
|
||||
# Check for role mismatch and add warning if found
|
||||
role_warning = get_role_mismatch_warning(task, input_data.assignee)
|
||||
if role_warning:
|
||||
guidance += f"\n\n⚠️ ROLE WARNING: {role_warning}"
|
||||
|
||||
return format_task_response(assigned_task, "ASSIGNED", guidance)
|
||||
|
||||
|
||||
|
||||
@@ -21,11 +21,13 @@ from roboco.models import SubstituteReason, TaskStatus
|
||||
HTTP_NOT_FOUND = 404
|
||||
|
||||
# Map substitute reasons to target task statuses
|
||||
# NOTE: OUT_OF_SCOPE_ROLE goes to awaiting_pm_review to avoid reassignment loops
|
||||
# (e.g., documenter can't self-document → substitute → gets reassigned same doc → loop)
|
||||
REASON_TO_STATUS: dict[SubstituteReason, TaskStatus] = {
|
||||
SubstituteReason.TASK_COMPLETE: TaskStatus.AWAITING_QA,
|
||||
SubstituteReason.LOW_CONTEXT: TaskStatus.PENDING,
|
||||
SubstituteReason.OUT_OF_SCOPE_TEAM: TaskStatus.PENDING,
|
||||
SubstituteReason.OUT_OF_SCOPE_ROLE: TaskStatus.PENDING,
|
||||
SubstituteReason.OUT_OF_SCOPE_ROLE: TaskStatus.AWAITING_PM_REVIEW, # PM decides
|
||||
SubstituteReason.MAX_RETRIES: TaskStatus.PENDING,
|
||||
SubstituteReason.BLOCKED_EXTERNAL: TaskStatus.BLOCKED,
|
||||
}
|
||||
@@ -190,6 +192,11 @@ async def handle_task_substitute(
|
||||
"Task marked as blocked. PM will be notified. "
|
||||
"You are now free to claim new work with roboco_task_scan()."
|
||||
)
|
||||
elif substitute_reason == SubstituteReason.OUT_OF_SCOPE_ROLE:
|
||||
next_action = (
|
||||
"Task sent to PM for reassignment (role conflict). "
|
||||
"You are now free to claim new work with roboco_task_scan()."
|
||||
)
|
||||
else:
|
||||
next_action = (
|
||||
"Task released and will be reassigned. "
|
||||
|
||||
@@ -91,7 +91,8 @@ def get_next_step_guidance(status: str) -> tuple[str, str]:
|
||||
"needs_revision": (
|
||||
"FIX_ISSUES",
|
||||
"QA found issues. Read the QA notes carefully. "
|
||||
"Fix all issues, then re-submit for QA.",
|
||||
"Call roboco_task_start() to resume work, fix all issues, "
|
||||
"then re-submit for QA with roboco_task_submit_qa().",
|
||||
),
|
||||
"awaiting_documentation": (
|
||||
"DOCUMENT",
|
||||
|
||||
Reference in New Issue
Block a user