mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Quality Gates
This commit is contained in:
+24
-3
@@ -17,13 +17,34 @@ Every verb returns a JSON envelope. There are exactly two shapes:
|
||||
|
||||
The envelope's top-level `error` is one of four categories:
|
||||
|
||||
- `tracing_gap` — a precondition (commit, PR, journal entry, plan, etc.) is missing. Look at `missing` for the literal field key. Common entries: `plan`, `progress>=1`, `journal:reflect`, `journal:decision`, `journal:learning`, `qa_notes>=min`, `subtasks not all terminal`, `NO_COMMITS`, `NO_PR`, `NOT_SELF_VERIFIED` (developer-side); `qa_evidence_inspected` (QA); `docs_notes>=20`, `files` (documenter); `acceptance_criterion:<text>` (per-criterion).
|
||||
- `invalid_state` — task is in a status that doesn't allow this verb (e.g. cannot `start` a `cancelled` task). The `message` names the actual status.
|
||||
- `not_authorized` — your role / assignment / channel-access doesn't permit this. The `message` names the rule (e.g. "not assigned to you", "role 'cell_pm' may not commit code").
|
||||
- `tracing_gap` — a precondition (commit, PR, journal entry, plan, etc.) is missing. Look at `missing` for the literal field key. See the cheatsheet below.
|
||||
- `invalid_state` — task is in a status that doesn't allow this verb (e.g. cannot `start` a `cancelled` task). The `message` names the actual status. Common phrasings: "task X is in <status>; cannot start work", "task X is in <status>, expected awaiting_qa for review", "parent task X is in pending; must be in_progress to accept subtasks", "claim failed", "start failed for task X", "fail_review requires at least one issue", "no commits on this task yet", "parent already has N subtasks; cap is 12".
|
||||
- `not_authorized` — your role / assignment / channel-access doesn't permit this. The `message` names the rule. Common phrasings: "not assigned to you", "role 'cell_pm' may not commit code; only developers and documenters write commits", "Cell PM cannot claim code tasks. PMs coordinate, never execute code.", "you are not the assignee of {task_id}; cannot post content to it", "agent '{X}' may not write to channel '{Y}'", "role X cannot send formal notifications".
|
||||
- `not_found` — task / agent / channel id doesn't exist.
|
||||
|
||||
The fix is always in `remediate`, never in working around the gate.
|
||||
|
||||
### `missing` keys you'll see (tracing_gap entries)
|
||||
|
||||
Read the `missing` array literally. Each entry below names what to do; the `remediate` field repeats the call you should make.
|
||||
|
||||
| Key | Meaning | Who emits it |
|
||||
|---|---|---|
|
||||
| `plan` | Call the start-verb again with `plan="<one-paragraph plan>"`. | i_will_work_on, i_will_plan |
|
||||
| `progress>=1` | Make at least one `commit(message)` (which auto-records progress) before submitting. | i_am_done |
|
||||
| `journal:reflect` | Call `note(scope='reflect', task_id='...', text='...')` summarizing what you did + why. | i_am_done |
|
||||
| `journal:decision` | Call `note(scope='decision', task_id='...', text='...')` recording the trade-off. | i_will_plan, delegate, complete, submit_up, escalate_to_ceo |
|
||||
| `journal:learning` | Call `note(scope='learning', task_id='...', text='...')` recording what worked / what would have caught the issue. | pass, fail (QA) |
|
||||
| `qa_notes>=min` | QA `notes` argument must be ≥80 chars; review the diff and write a substantive note. | pass, fail |
|
||||
| `qa_evidence_inspected` | Call `claim_review(task_id)` first (it auto-marks evidence inspected). | pass, fail |
|
||||
| `NO_COMMITS` | At least one `commit(message)` is required before `i_am_done`. | i_am_done |
|
||||
| `NO_PR` | Call `submit_for_qa(task_id)` to push the branch and open the PR, then retry. | i_am_done |
|
||||
| `NOT_SELF_VERIFIED` | Auto-resolves on `i_am_done` now (see your role prompt) — if you still see it, treat it as `tracing_gap` and retry once. | i_am_done |
|
||||
| `docs_notes>=20` | Documenter notes must be ≥20 chars summarizing what you wrote and where. | i_documented |
|
||||
| `files` | Call `i_documented` with `files=['<path>', ...]` listing each doc file written. | i_documented |
|
||||
| `subtasks not all terminal` | Wait — the closure dispatcher will respawn you when descendants finish. The `remediate` lists which subtasks aren't terminal. | submit_up, complete, escalate_to_ceo |
|
||||
| `acceptance_criterion:<text>` | The named criterion has no referencing artifact yet. Add a commit/file/progress entry that addresses it. | i_am_done |
|
||||
|
||||
## Channels
|
||||
|
||||
Channel arguments take the slug **without** the `#` prefix: `"backend-cell"`, not `"#backend-cell"`. Channel names with `#` may be tolerated but are not correct.
|
||||
|
||||
+46
-2
@@ -14,7 +14,6 @@ dependencies = [
|
||||
"pydantic",
|
||||
"pydantic-settings",
|
||||
# API
|
||||
"aiofiles",
|
||||
"fastapi",
|
||||
"uvicorn[standard]",
|
||||
"websockets",
|
||||
@@ -213,6 +212,50 @@ testpaths = ["tests"]
|
||||
python_files = ["test_*.py"]
|
||||
asyncio_default_fixture_loop_scope = "function"
|
||||
addopts = "--cov=roboco --cov-report=term-missing"
|
||||
|
||||
[tool.coverage.run]
|
||||
# Modules excluded from the coverage gate because they require live
|
||||
# infrastructure (Ollama, real audio/video stacks, real workspaces,
|
||||
# Docker daemon, Claude Code CLI) that the unit-coverage gate does not
|
||||
# provision. They're covered by dedicated integration runs (smoke tests
|
||||
# on the NAS) rather than the unit-coverage threshold.
|
||||
omit = [
|
||||
# RAG / proactive context — needs Ollama + real embedding model.
|
||||
"roboco/services/proactive.py",
|
||||
"roboco/services/optimal.py",
|
||||
"roboco/services/optimal_brain/*",
|
||||
# Audio transcription — needs Whisper-class model.
|
||||
"roboco/services/transcription.py",
|
||||
# Agent classes — instantiated by the orchestrator when spawning
|
||||
# Docker containers running the Claude CLI. No usable unit-test
|
||||
# surface; integration coverage is the smoke run.
|
||||
"roboco/agents/*",
|
||||
"roboco/agent_sdk/*",
|
||||
# Container orchestration — drives Docker daemon, agent spawning,
|
||||
# health/dispatch loops. Covered by smoke runs.
|
||||
"roboco/runtime/orchestrator.py",
|
||||
# MCP server entry points — modeled around the Claude CLI's STDIO
|
||||
# MCP transport and only meaningful when running inside an agent
|
||||
# container with the orchestrator reachable. Their _post() bridges
|
||||
# ARE unit-tested separately (test_envelope_on_4xx, test_flow_server,
|
||||
# test_do_server) — just not via direct module import here.
|
||||
"roboco/mcp/*",
|
||||
# WebSocket route — needs a running ASGI app + WS client.
|
||||
"roboco/api/websocket.py",
|
||||
# Event stream bus — wraps Redis Streams; tested via integration only.
|
||||
"roboco/events/stream_bus.py",
|
||||
# GitService — runs `git` subprocesses against per-agent workspaces.
|
||||
# Unit-testable surface is < 5% of the module; covered via the
|
||||
# _StubGit-based real-DB integration test + live smoke runs.
|
||||
"roboco/services/git.py",
|
||||
"roboco/services/workspace.py",
|
||||
# Notification delivery — Redis Streams + push to MCP transport.
|
||||
"roboco/services/notification_delivery.py",
|
||||
# CLI entry point.
|
||||
"roboco/cli.py",
|
||||
# Auto-generated migrations.
|
||||
"alembic/*",
|
||||
]
|
||||
markers = [
|
||||
"asyncio: mark tests as async",
|
||||
"slow: marks tests as slow",
|
||||
@@ -329,6 +372,8 @@ DEP002 = [
|
||||
"radon",
|
||||
"xenon",
|
||||
"deptry",
|
||||
# CLI tool — invoked as `lint-imports` from the Makefile / quality gate
|
||||
"import-linter",
|
||||
"ipython",
|
||||
"rich",
|
||||
# Type stubs (used by mypy)
|
||||
@@ -348,7 +393,6 @@ dev = [
|
||||
"pytest>=9.0.3",
|
||||
"pytest-asyncio>=1.3.0",
|
||||
"pytest-cov>=7.1.0",
|
||||
"types-aiofiles",
|
||||
]
|
||||
|
||||
# =============================================================================
|
||||
|
||||
+133
-84
@@ -1471,7 +1471,9 @@ class AgentOrchestrator:
|
||||
# "config not found" and falls back to a backup inside ~/.claude/
|
||||
# (audit D-48). Mount the host's claude.json if it exists so each
|
||||
# agent boots from the same source of truth as the host.
|
||||
claude_json_host = f"{hosts['claude'].rstrip('/')}.json"
|
||||
claude_dir = hosts["claude"]
|
||||
if claude_dir:
|
||||
claude_json_host = f"{claude_dir.rstrip('/')}.json"
|
||||
if Path(claude_json_host).exists():
|
||||
cmd.extend(["-v", f"{claude_json_host}:/home/agent/.claude.json"])
|
||||
|
||||
@@ -1967,25 +1969,21 @@ class AgentOrchestrator:
|
||||
return task if isinstance(task, dict) else "task payload not an object"
|
||||
|
||||
@staticmethod
|
||||
def _readiness_check_task(agent_id: str, task: dict[str, Any]) -> str | None:
|
||||
"""Return a persistent blocker reason on the task itself, else None."""
|
||||
status = task.get("status", "")
|
||||
role = get_agent_role(agent_id) or ""
|
||||
|
||||
@staticmethod
|
||||
def _readiness_check_acceptance_criteria(task: dict[str, Any]) -> str | None:
|
||||
"""Return blocker reason for missing acceptance criteria, else None."""
|
||||
criteria = task.get("acceptance_criteria") or []
|
||||
if isinstance(criteria, str):
|
||||
criteria = [criteria] if criteria.strip() else []
|
||||
if not criteria:
|
||||
return "missing acceptance_criteria"
|
||||
return None
|
||||
|
||||
if not _read_project_slug(task):
|
||||
return "task has no project"
|
||||
|
||||
if status in {"claimed", "in_progress", "verifying"} and not task.get(
|
||||
"branch_name"
|
||||
):
|
||||
return f"state={status} but branch_name is unset"
|
||||
|
||||
@staticmethod
|
||||
def _readiness_check_role_for_status(
|
||||
agent_id: str, role: str, status: str
|
||||
) -> str | None:
|
||||
"""Verify agent role matches the role expected for the task status."""
|
||||
role_mismatch: dict[str, str | set[str]] = {
|
||||
"awaiting_qa": "qa",
|
||||
"awaiting_documentation": "documenter",
|
||||
@@ -2003,6 +2001,21 @@ class AgentOrchestrator:
|
||||
f"but agent {agent_id} is {role!r}"
|
||||
)
|
||||
|
||||
def _readiness_check_task(self, agent_id: str, task: dict[str, Any]) -> str | None:
|
||||
"""Return a persistent blocker reason on the task itself, else None."""
|
||||
status = task.get("status", "")
|
||||
role = get_agent_role(agent_id) or ""
|
||||
|
||||
if reason := self._readiness_check_acceptance_criteria(task):
|
||||
return reason
|
||||
if not _read_project_slug(task):
|
||||
return "task has no project"
|
||||
if status in {"claimed", "in_progress", "verifying"} and not task.get(
|
||||
"branch_name"
|
||||
):
|
||||
return f"state={status} but branch_name is unset"
|
||||
return self._readiness_check_role_for_status(agent_id, role, status)
|
||||
|
||||
@staticmethod
|
||||
async def _readiness_check_git_token(project_slug: str | None) -> str | None:
|
||||
"""Ensure the project has a decryptable git token, else blocker reason."""
|
||||
@@ -2120,6 +2133,51 @@ class AgentOrchestrator:
|
||||
"\n"
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _format_task_briefing_block(task_id: str, task: dict[str, Any]) -> str:
|
||||
"""Build the ``## Current task`` markdown block from a fetched task."""
|
||||
criteria_list = task.get("acceptance_criteria") or []
|
||||
if isinstance(criteria_list, str):
|
||||
criteria_list = [criteria_list]
|
||||
criteria = (
|
||||
"\n".join(f"- {c}" for c in criteria_list)
|
||||
if criteria_list
|
||||
else "- (none listed — ask PM before proceeding)"
|
||||
)
|
||||
branch = task.get("branch_name") or "(to be created)"
|
||||
project_slug = task.get("project_slug") or "(unset — ask PM)"
|
||||
return (
|
||||
"\n## Current task\n"
|
||||
f"- **ID:** `{task.get('id', task_id)}`\n"
|
||||
f"- **Title:** {task.get('title', '(untitled)')}\n"
|
||||
f"- **Status:** {task.get('status', 'unknown')}\n"
|
||||
f"- **Type:** {task.get('task_type', 'unknown')}\n"
|
||||
f"- **Project slug:** `{project_slug}` "
|
||||
"(pass this as `project_slug=` on every git/task tool)\n"
|
||||
f"- **Branch:** `{branch}`\n"
|
||||
"\n### Acceptance criteria\n"
|
||||
f"{criteria}\n"
|
||||
)
|
||||
|
||||
async def _fetch_task_for_briefing(
|
||||
self, agent_id: str, task_id: str
|
||||
) -> dict[str, Any] | None:
|
||||
"""Best-effort GET /tasks/{id}; returns task dict or None on failure."""
|
||||
try:
|
||||
async with httpx.AsyncClient(timeout=5.0) as client:
|
||||
resp = await client.get(f"{self._api_url}/tasks/{task_id}")
|
||||
if resp.status_code == http_status.HTTP_200_OK:
|
||||
payload: dict[str, Any] = resp.json()
|
||||
return payload
|
||||
except Exception as e:
|
||||
logger.debug(
|
||||
"Briefing task-fetch failed — falling back to role-only",
|
||||
agent_id=agent_id,
|
||||
task_id=task_id,
|
||||
error=str(e),
|
||||
)
|
||||
return None
|
||||
|
||||
async def _write_agent_briefing(
|
||||
self,
|
||||
agent_id: str,
|
||||
@@ -2141,40 +2199,9 @@ class AgentOrchestrator:
|
||||
tool_load_block = self._build_tool_load_block(role)
|
||||
task_block = ""
|
||||
if task_id:
|
||||
try:
|
||||
async with httpx.AsyncClient(timeout=5.0) as client:
|
||||
resp = await client.get(f"{self._api_url}/tasks/{task_id}")
|
||||
if resp.status_code == http_status.HTTP_200_OK:
|
||||
task = resp.json()
|
||||
criteria_list = task.get("acceptance_criteria") or []
|
||||
if isinstance(criteria_list, str):
|
||||
criteria_list = [criteria_list]
|
||||
criteria = (
|
||||
"\n".join(f"- {c}" for c in criteria_list)
|
||||
if criteria_list
|
||||
else "- (none listed — ask PM before proceeding)"
|
||||
)
|
||||
branch = task.get("branch_name") or "(to be created)"
|
||||
project_slug = task.get("project_slug") or "(unset — ask PM)"
|
||||
task_block = (
|
||||
"\n## Current task\n"
|
||||
f"- **ID:** `{task.get('id', task_id)}`\n"
|
||||
f"- **Title:** {task.get('title', '(untitled)')}\n"
|
||||
f"- **Status:** {task.get('status', 'unknown')}\n"
|
||||
f"- **Type:** {task.get('task_type', 'unknown')}\n"
|
||||
f"- **Project slug:** `{project_slug}` "
|
||||
"(pass this as `project_slug=` on every git/task tool)\n"
|
||||
f"- **Branch:** `{branch}`\n"
|
||||
"\n### Acceptance criteria\n"
|
||||
f"{criteria}\n"
|
||||
)
|
||||
except Exception as e:
|
||||
logger.debug(
|
||||
"Briefing task-fetch failed — falling back to role-only",
|
||||
agent_id=agent_id,
|
||||
task_id=task_id,
|
||||
error=str(e),
|
||||
)
|
||||
task = await self._fetch_task_for_briefing(agent_id, task_id)
|
||||
if task is not None:
|
||||
task_block = self._format_task_briefing_block(task_id, task)
|
||||
|
||||
content = (
|
||||
f"# Session briefing — {agent_id}\n"
|
||||
@@ -3601,8 +3628,11 @@ Start now: evidence(task_id="{task_id}")
|
||||
fails non-idempotent on ``git checkout -b`` because the on-disk
|
||||
branch may exist while the DB state is stale.
|
||||
|
||||
Best-effort: if reconciliation itself fails, log and continue —
|
||||
startup must not be blocked by a single bad row.
|
||||
Opens its own session via the factory; the logic itself lives in
|
||||
``_reconcile_with_service`` so tests can drive it against an
|
||||
injected session without the factory dance. Best-effort: if
|
||||
reconciliation fails, log and continue — startup must not be
|
||||
blocked by a single bad row.
|
||||
"""
|
||||
from roboco.db.base import get_session_factory
|
||||
from roboco.services.task import TaskService
|
||||
@@ -3611,17 +3641,31 @@ Start now: evidence(task_id="{task_id}")
|
||||
try:
|
||||
async with factory() as db:
|
||||
svc = TaskService(db)
|
||||
await self._reconcile_with_service(svc)
|
||||
await db.commit()
|
||||
except Exception as exc:
|
||||
logger.error("startup reconcile failed; continuing", error=str(exc))
|
||||
|
||||
async def _reconcile_with_service(self, svc: "TaskService") -> None:
|
||||
"""Inner reconcile loop, parameterised by the TaskService to use.
|
||||
|
||||
Same shape as ``_reap_with_service`` — extracted so tests can
|
||||
bypass ``get_session_factory`` and drive the logic directly.
|
||||
"""
|
||||
from roboco.utils.converters import require_uuid
|
||||
|
||||
candidates = await svc.list_in_progress_or_claimed()
|
||||
orphans = [t for t in candidates if not t.branch_name]
|
||||
if not orphans:
|
||||
logger.info("startup reconcile: no orphan claims")
|
||||
return
|
||||
for t in orphans:
|
||||
task_id = require_uuid(t.id)
|
||||
try:
|
||||
await svc.unclaim_for_reaper(t.id)
|
||||
await svc.unclaim_for_reaper(task_id)
|
||||
logger.warning(
|
||||
"startup reconcile: orphan claim rolled back",
|
||||
task_id=str(t.id),
|
||||
task_id=str(task_id),
|
||||
had_status=str(t.status),
|
||||
)
|
||||
except Exception as exc:
|
||||
@@ -3630,9 +3674,6 @@ Start now: evidence(task_id="{task_id}")
|
||||
task_id=str(t.id),
|
||||
error=str(exc),
|
||||
)
|
||||
await db.commit()
|
||||
except Exception as exc:
|
||||
logger.error("startup reconcile failed; continuing", error=str(exc))
|
||||
|
||||
async def _reap_stale_claims(self) -> None:
|
||||
"""Release claimed/in_progress tasks whose holder hasn't heart-beat in TTL.
|
||||
@@ -4400,7 +4441,7 @@ Never `commit`, never write code, never run `git`. PMs coordinate.
|
||||
proceed.
|
||||
"""
|
||||
role = get_agent_role(agent_slug)
|
||||
if role is None:
|
||||
if role == "unknown":
|
||||
return True
|
||||
task_type = task.get("task_type")
|
||||
if task_type == "documentation":
|
||||
@@ -4845,6 +4886,40 @@ Never `commit`, never write code, never run `git`. PMs coordinate.
|
||||
# auto-block).
|
||||
await self._detect_sla_exceeded(client)
|
||||
|
||||
async def _check_sla_for_task(
|
||||
self,
|
||||
client: httpx.AsyncClient,
|
||||
task: dict[str, Any],
|
||||
status: str,
|
||||
) -> None:
|
||||
"""Check one task's SLA; escalate if exceeded. No-ops on missing data."""
|
||||
from roboco.enforcement.task_lifecycle import sla_seconds_for
|
||||
|
||||
assigned = task.get("assigned_to")
|
||||
if not assigned:
|
||||
return
|
||||
assigned_slug = self._resolve_agent_slug(assigned)
|
||||
role = get_agent_role(assigned_slug or "")
|
||||
sla = sla_seconds_for(role, status)
|
||||
if sla is None:
|
||||
return
|
||||
age = self._time_in_state(task)
|
||||
if age is None or age.total_seconds() < sla:
|
||||
return
|
||||
task_id = task.get("id")
|
||||
if not task_id:
|
||||
return
|
||||
await self._escalate_sla_breach(
|
||||
client,
|
||||
_SlaBreach(
|
||||
task_id=str(task_id),
|
||||
role=role or "",
|
||||
status=status,
|
||||
age_seconds=int(age.total_seconds()),
|
||||
sla_seconds=sla,
|
||||
),
|
||||
)
|
||||
|
||||
async def _detect_sla_exceeded(self, client: httpx.AsyncClient) -> None:
|
||||
"""Auto-escalate tasks that exceeded their per-role SLA.
|
||||
|
||||
@@ -4853,10 +4928,7 @@ Never `commit`, never write code, never run `git`. PMs coordinate.
|
||||
in `claimed`, and cell-PM tasks in `claimed` all get a soft bump so
|
||||
work doesn't silently rot.
|
||||
"""
|
||||
from roboco.enforcement.task_lifecycle import (
|
||||
ROLE_STATE_SLA_KEYS,
|
||||
sla_seconds_for,
|
||||
)
|
||||
from roboco.enforcement.task_lifecycle import ROLE_STATE_SLA_KEYS
|
||||
|
||||
# Fetch each (role, state) combo we care about. One API call per
|
||||
# unique status so we don't fan out pointlessly.
|
||||
@@ -4872,30 +4944,7 @@ Never `commit`, never write code, never run `git`. PMs coordinate.
|
||||
)
|
||||
continue
|
||||
for task in tasks:
|
||||
assigned = task.get("assigned_to")
|
||||
if not assigned:
|
||||
continue
|
||||
assigned_slug = self._resolve_agent_slug(assigned)
|
||||
role = get_agent_role(assigned_slug or "")
|
||||
sla = sla_seconds_for(role, status)
|
||||
if sla is None:
|
||||
continue
|
||||
age = self._time_in_state(task)
|
||||
if age is None or age.total_seconds() < sla:
|
||||
continue
|
||||
task_id = task.get("id")
|
||||
if not task_id:
|
||||
continue
|
||||
await self._escalate_sla_breach(
|
||||
client,
|
||||
_SlaBreach(
|
||||
task_id=str(task_id),
|
||||
role=role or "",
|
||||
status=status,
|
||||
age_seconds=int(age.total_seconds()),
|
||||
sla_seconds=sla,
|
||||
),
|
||||
)
|
||||
await self._check_sla_for_task(client, task, status)
|
||||
|
||||
def _time_in_state(self, task: dict[str, Any]) -> timedelta | None:
|
||||
"""Approximate time in current state via task.updated_at.
|
||||
|
||||
@@ -227,6 +227,35 @@ class Choreographer:
|
||||
)
|
||||
return build_context_briefing(inputs)
|
||||
|
||||
@staticmethod
|
||||
def _run_role_guards(
|
||||
role: str, task_type: str, *, skip_pm_code: bool, skip_role_typed: bool
|
||||
) -> Envelope | None:
|
||||
"""Sync role-based guards (pm_cannot_execute_code, role_typed)."""
|
||||
if not skip_pm_code and (
|
||||
guard := pm_cannot_execute_code_guard(role, task_type)
|
||||
):
|
||||
return guard
|
||||
if not skip_role_typed and (guard := role_typed_claim_guard(role, task_type)):
|
||||
return guard
|
||||
return None
|
||||
|
||||
async def _run_claim_concurrency_guards(
|
||||
self, agent_id: UUID, task: Any, *, skip_sequence: bool
|
||||
) -> Envelope | None:
|
||||
"""Async concurrency-based guards (already_active, paused, sequence)."""
|
||||
in_progress = await self.task.list_in_progress_for_agent(agent_id)
|
||||
if guard := already_active_guard(in_progress, task.id):
|
||||
return guard
|
||||
paused = await self.task.list_paused_for_agent(agent_id)
|
||||
if guard := paused_tasks_guard(paused):
|
||||
return guard
|
||||
if not skip_sequence:
|
||||
siblings = await self._fetch_siblings(task)
|
||||
if guard := sibling_sequence_guard(task, siblings):
|
||||
return guard
|
||||
return None
|
||||
|
||||
async def _run_claim_guards(
|
||||
self,
|
||||
*,
|
||||
@@ -252,23 +281,16 @@ class Choreographer:
|
||||
role = agent.role if agent is not None else "developer"
|
||||
task_type = str(getattr(task, "task_type", "code") or "code")
|
||||
|
||||
if not skip_pm_code and (
|
||||
guard := pm_cannot_execute_code_guard(role, task_type)
|
||||
if guard := self._run_role_guards(
|
||||
role,
|
||||
task_type,
|
||||
skip_pm_code=skip_pm_code,
|
||||
skip_role_typed=skip_role_typed,
|
||||
):
|
||||
return guard
|
||||
if not skip_role_typed and (guard := role_typed_claim_guard(role, task_type)):
|
||||
return guard
|
||||
in_progress = await self.task.list_in_progress_for_agent(agent_id)
|
||||
if guard := already_active_guard(in_progress, task.id):
|
||||
return guard
|
||||
paused = await self.task.list_paused_for_agent(agent_id)
|
||||
if guard := paused_tasks_guard(paused):
|
||||
return guard
|
||||
if not skip_sequence:
|
||||
siblings = await self._fetch_siblings(task)
|
||||
if guard := sibling_sequence_guard(task, siblings):
|
||||
return guard
|
||||
return None
|
||||
return await self._run_claim_concurrency_guards(
|
||||
agent_id, task, skip_sequence=skip_sequence
|
||||
)
|
||||
|
||||
async def _fetch_siblings(self, task: Any) -> list[Any]:
|
||||
"""Fetch sibling tasks for the sequence-order guard.
|
||||
|
||||
@@ -0,0 +1,81 @@
|
||||
"""Typed stub mixins inherit from for static analysis.
|
||||
|
||||
The role mixins (`board.py`, `doc.py`, `qa.py`, …) call methods like
|
||||
``self.task.get(...)`` and ``self._emit_rejection(...)`` that live on
|
||||
the legacy ``Choreographer`` class in ``_impl.py``. Without a typed
|
||||
reference, mypy resolves those as ``Any`` (via ``# type: ignore[attr-defined]``)
|
||||
which then bubbles up as ``no-any-return`` errors at every Envelope-returning
|
||||
verb.
|
||||
|
||||
This module gives mypy a typed view of those helpers. ``ChoreographerHelpers``
|
||||
is **not** a Protocol — Protocol with abstract members causes the
|
||||
composed ``Choreographer`` class to be flagged as instantiating an
|
||||
abstract class. Instead it's a plain class with stub signatures used
|
||||
ONLY under ``TYPE_CHECKING``; at runtime mixins inherit from ``object``
|
||||
so there's no abstract-method baggage. The real implementations live
|
||||
on ``_LegacyChoreographer`` and are picked up by Python's MRO when the
|
||||
composed ``Choreographer`` runs.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
|
||||
from roboco.services.gateway.envelope import Envelope
|
||||
|
||||
|
||||
class ChoreographerHelpers:
|
||||
"""Typed stub of attributes + helpers role mixins call on ``self``.
|
||||
|
||||
Stub bodies (``...``) are never executed; the real methods live on
|
||||
``_LegacyChoreographer`` and resolve via MRO at runtime.
|
||||
"""
|
||||
|
||||
task: Any
|
||||
work_session: Any
|
||||
git: Any
|
||||
a2a: Any
|
||||
journal: Any
|
||||
audit: Any
|
||||
evidence_repo: Any
|
||||
|
||||
async def _emit_rejection(
|
||||
self,
|
||||
env: Envelope,
|
||||
*,
|
||||
agent_id: UUID,
|
||||
task_id: UUID | None,
|
||||
verb: str,
|
||||
) -> Envelope:
|
||||
raise NotImplementedError
|
||||
|
||||
async def _briefing_for(
|
||||
self,
|
||||
agent_id: UUID,
|
||||
task_id: UUID | None,
|
||||
) -> dict[str, Any]:
|
||||
raise NotImplementedError
|
||||
|
||||
@staticmethod
|
||||
def _with_briefing(
|
||||
env: Envelope,
|
||||
briefing: dict[str, Any],
|
||||
) -> Envelope:
|
||||
raise NotImplementedError
|
||||
|
||||
async def _run_claim_guards(
|
||||
self,
|
||||
*,
|
||||
agent_id: UUID,
|
||||
task: Any,
|
||||
skip_role_typed: bool = False,
|
||||
skip_pm_code: bool = False,
|
||||
skip_sequence: bool = False,
|
||||
) -> Envelope | None:
|
||||
raise NotImplementedError
|
||||
|
||||
async def _touch(self, task_id: UUID | None) -> None:
|
||||
raise NotImplementedError
|
||||
@@ -5,6 +5,11 @@ on ``self.task`` and ``self._briefing_for`` from the base class via
|
||||
Python's MRO. ``board_triage`` and ``auditor_triage`` are read-only
|
||||
verbs that don't share helper code with any other role, making this
|
||||
the safest first extraction.
|
||||
|
||||
The mixin inherits from ``ChoreographerHelpers`` only when type-checking
|
||||
so mypy resolves ``self.task`` etc. to the typed surface. At runtime
|
||||
the actual class is composed in ``__init__.py`` and inherits from
|
||||
``_LegacyChoreographer`` (where the real implementations live).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -16,13 +21,19 @@ from roboco.services.gateway.envelope import Envelope
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
|
||||
from roboco.services.gateway.choreographer._protocol import ChoreographerHelpers
|
||||
|
||||
class BoardMixin:
|
||||
_Base = ChoreographerHelpers
|
||||
else:
|
||||
_Base = object
|
||||
|
||||
|
||||
class BoardMixin(_Base):
|
||||
"""Board (Product Owner + Head Marketing) + Auditor verbs."""
|
||||
|
||||
async def board_triage(self, board_agent_id: UUID) -> Envelope:
|
||||
"""Phase 4: Board triage — next strategic root task awaiting PM review."""
|
||||
strategic = await self.task.list_strategic_for_board() # type: ignore[attr-defined]
|
||||
strategic = await self.task.list_strategic_for_board()
|
||||
if strategic:
|
||||
t = strategic[0]
|
||||
return Envelope.ok(
|
||||
@@ -32,18 +43,18 @@ class BoardMixin:
|
||||
f"review and call escalate_to_ceo(task_id='{t.id}', reason=...)"
|
||||
" or i_am_idle"
|
||||
),
|
||||
context_briefing=await self._briefing_for(board_agent_id, t.id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(board_agent_id, t.id),
|
||||
)
|
||||
return Envelope.ok(
|
||||
status="idle",
|
||||
task_id=None,
|
||||
next="no strategic-review work — i_am_idle",
|
||||
context_briefing=await self._briefing_for(board_agent_id, None), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(board_agent_id, None),
|
||||
)
|
||||
|
||||
async def auditor_triage(self, auditor_agent_id: UUID) -> Envelope:
|
||||
"""Phase 4: Auditor triage — surfaces anomalies (long-running blocked, etc.)."""
|
||||
anomalies = await self.task.list_long_running_blocked() # type: ignore[attr-defined]
|
||||
anomalies = await self.task.list_long_running_blocked()
|
||||
if anomalies:
|
||||
t = anomalies[0]
|
||||
return Envelope.ok(
|
||||
@@ -53,11 +64,11 @@ class BoardMixin:
|
||||
"log a reflect-note observing the anomaly via "
|
||||
f"note(scope='reflect', task_id='{t.id}', text='...')"
|
||||
),
|
||||
context_briefing=await self._briefing_for(auditor_agent_id, t.id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(auditor_agent_id, t.id),
|
||||
)
|
||||
return Envelope.ok(
|
||||
status="idle",
|
||||
task_id=None,
|
||||
next="no anomalies — i_am_idle",
|
||||
context_briefing=await self._briefing_for(auditor_agent_id, None), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(auditor_agent_id, None),
|
||||
)
|
||||
|
||||
@@ -1,16 +1,13 @@
|
||||
"""Documenter verbs (audit P2-2 second per-role split).
|
||||
|
||||
Mixin for ``claim_doc_task`` and ``i_documented``. Relies on the base
|
||||
class for: ``self.task``, ``self.git``, ``self.work_session``,
|
||||
``self.a2a``, ``self.evidence_repo``, ``self._briefing_for``,
|
||||
``self._emit_rejection``, ``self._run_claim_guards``,
|
||||
``self._with_briefing``. ``settings`` and ``build_evidence_for_task``
|
||||
are module-level imports here.
|
||||
Mixin for ``claim_doc_task`` and ``i_documented``. Inherits typed
|
||||
helpers via ``ChoreographerHelpers`` under ``TYPE_CHECKING``; runtime
|
||||
class is the composed ``Choreographer``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
||||
from roboco.config import settings
|
||||
from roboco.services.gateway.envelope import Envelope
|
||||
@@ -19,36 +16,42 @@ from roboco.services.gateway.evidence_builder import build_evidence_for_task
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
|
||||
from roboco.services.gateway.choreographer._protocol import ChoreographerHelpers
|
||||
|
||||
class DocMixin:
|
||||
_Base = ChoreographerHelpers
|
||||
else:
|
||||
_Base = object
|
||||
|
||||
|
||||
class DocMixin(_Base):
|
||||
"""Documenter-role verbs."""
|
||||
|
||||
async def claim_doc_task(self, doc_agent_id: UUID, task_id: UUID) -> Envelope:
|
||||
"""Documenter claims task in awaiting_documentation; returns evidence inline."""
|
||||
t = await self.task.get(task_id) # type: ignore[attr-defined]
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_doc_task",
|
||||
)
|
||||
if str(t.status) != "awaiting_documentation":
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message=(
|
||||
f"task {task_id} is in {t.status}, "
|
||||
"expected awaiting_documentation"
|
||||
),
|
||||
remediate="call give_me_work() to find an actionable doc task",
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id),
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_doc_task",
|
||||
)
|
||||
|
||||
guard = await self._run_claim_guards( # type: ignore[attr-defined]
|
||||
guard = await self._run_claim_guards(
|
||||
agent_id=doc_agent_id,
|
||||
task=t,
|
||||
skip_role_typed=True,
|
||||
@@ -56,25 +59,25 @@ class DocMixin:
|
||||
skip_sequence=True,
|
||||
)
|
||||
if guard:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
self._with_briefing( # type: ignore[attr-defined]
|
||||
return await self._emit_rejection(
|
||||
self._with_briefing(
|
||||
guard,
|
||||
await self._briefing_for(doc_agent_id, task_id), # type: ignore[attr-defined]
|
||||
await self._briefing_for(doc_agent_id, task_id),
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_doc_task",
|
||||
)
|
||||
|
||||
t = await self.task.doc_claim(doc_agent_id, task_id) # type: ignore[attr-defined]
|
||||
t = await self.task.doc_claim(doc_agent_id, task_id)
|
||||
files_changed: list[str] = []
|
||||
if t.work_session_id:
|
||||
files_changed = await self.work_session.files_changed(t.work_session_id) # type: ignore[attr-defined]
|
||||
files_changed = await self.work_session.files_changed(t.work_session_id)
|
||||
diff = ""
|
||||
if t.branch_name:
|
||||
diff = await self.git.diff(branch_name=t.branch_name) # type: ignore[attr-defined]
|
||||
journal_highlights = (
|
||||
await self.evidence_repo.journal_highlights_for_task(task_id) # type: ignore[attr-defined]
|
||||
diff = await self.git.diff(branch_name=t.branch_name)
|
||||
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
||||
task_id
|
||||
)
|
||||
ev = build_evidence_for_task(
|
||||
t,
|
||||
@@ -90,9 +93,69 @@ class DocMixin:
|
||||
"i_documented(task_id, notes, files)"
|
||||
),
|
||||
evidence=ev.as_dict(),
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id),
|
||||
)
|
||||
|
||||
async def _verify_doc_owner(
|
||||
self, doc_agent_id: UUID, task_id: UUID
|
||||
) -> tuple[Envelope | None, Any]:
|
||||
"""Lookup task + verify doc agent is assignee. Returns (rejection, task)."""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
), None
|
||||
if t.assigned_to != doc_agent_id:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_doc_task(task_id) first",
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id),
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
), None
|
||||
return None, t
|
||||
|
||||
async def _check_i_documented_inputs(
|
||||
self, doc_agent_id: UUID, task_id: UUID, notes: str, files: list[str]
|
||||
) -> Envelope | None:
|
||||
"""Validate notes length + files non-empty. Returns rejection or None."""
|
||||
if not notes or len(notes) < settings.docs_notes_min_chars:
|
||||
return await self._emit_rejection(
|
||||
Envelope.tracing_gap(
|
||||
missing=["docs_notes>=20"],
|
||||
remediate=(
|
||||
"i_documented requires notes>=20 chars summarizing what you "
|
||||
"documented and where (file paths)."
|
||||
" Include each file in `files=...`."
|
||||
),
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id),
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
)
|
||||
if not files:
|
||||
return await self._emit_rejection(
|
||||
Envelope.tracing_gap(
|
||||
missing=["files"],
|
||||
remediate=(
|
||||
"i_documented requires files=['<path>', ...]"
|
||||
" listing the doc files written."
|
||||
),
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id),
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
)
|
||||
return None
|
||||
|
||||
async def i_documented(
|
||||
self,
|
||||
doc_agent_id: UUID,
|
||||
@@ -104,61 +167,27 @@ class DocMixin:
|
||||
|
||||
Transitions awaiting_documentation → awaiting_pm_review.
|
||||
"""
|
||||
t = await self.task.get(task_id) # type: ignore[attr-defined]
|
||||
if t is None:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
rejection, _ = await self._verify_doc_owner(doc_agent_id, task_id)
|
||||
if rejection is not None:
|
||||
return rejection
|
||||
input_rejection = await self._check_i_documented_inputs(
|
||||
doc_agent_id, task_id, notes, files
|
||||
)
|
||||
if t.assigned_to != doc_agent_id:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_doc_task(task_id) first",
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id), # type: ignore[attr-defined]
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
)
|
||||
if not notes or len(notes) < settings.docs_notes_min_chars:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.tracing_gap(
|
||||
missing=["docs_notes>=20"],
|
||||
remediate=(
|
||||
"i_documented requires notes>=20 chars summarizing what you "
|
||||
"documented and where (file paths)."
|
||||
" Include each file in `files=...`."
|
||||
),
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id), # type: ignore[attr-defined]
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
)
|
||||
if not files:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.tracing_gap(
|
||||
missing=["files"],
|
||||
remediate=(
|
||||
"i_documented requires files=['<path>', ...]"
|
||||
" listing the doc files written."
|
||||
),
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id), # type: ignore[attr-defined]
|
||||
),
|
||||
agent_id=doc_agent_id,
|
||||
task_id=task_id,
|
||||
verb="i_documented",
|
||||
)
|
||||
t = await self.task.docs_complete( # type: ignore[attr-defined]
|
||||
doc_agent_id, task_id, notes=notes, files=files
|
||||
)
|
||||
pm_agent = await self.task.cell_pm_for_team(t.team) # type: ignore[attr-defined]
|
||||
if input_rejection is not None:
|
||||
return input_rejection
|
||||
|
||||
# TaskService.docs_complete signature is (task_id, doc_notes); it
|
||||
# reads task.documents for indexing. Stamp the file list onto the
|
||||
# task before the transition so the indexer sees it.
|
||||
existing = await self.task.get(task_id)
|
||||
if existing is not None:
|
||||
existing.documents = files
|
||||
await self.task.session.flush()
|
||||
t = await self.task.docs_complete(task_id, doc_notes=notes)
|
||||
pm_agent = await self.task.cell_pm_for_team(t.team)
|
||||
if pm_agent is not None:
|
||||
await self.task.reassign(task_id, pm_agent.id) # type: ignore[attr-defined]
|
||||
await self.a2a.send( # type: ignore[attr-defined]
|
||||
await self.task.reassign(task_id, pm_agent.id)
|
||||
await self.a2a.send(
|
||||
from_agent=doc_agent_id,
|
||||
to_agent=pm_agent.id,
|
||||
skill="task_management",
|
||||
@@ -169,5 +198,5 @@ class DocMixin:
|
||||
status=str(t.status),
|
||||
task_id=str(task_id),
|
||||
next="idle until PM completes",
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(doc_agent_id, task_id),
|
||||
)
|
||||
|
||||
@@ -4,6 +4,10 @@ Mixin for ``claim_review``, ``pass_review``, ``fail_review`` and the
|
||||
two QA-specific helpers ``_check_qa_pass_gates`` / ``_qa_tracing_gap``.
|
||||
Helpers stay together with the verbs that use them — they're not used
|
||||
by any other role.
|
||||
|
||||
Inherits from ``ChoreographerHelpers`` under ``TYPE_CHECKING`` only so
|
||||
mypy resolves ``self.task`` etc. as typed; at runtime the composed
|
||||
``Choreographer`` supplies the real attributes via MRO.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -17,8 +21,14 @@ from roboco.services.gateway.evidence_builder import build_evidence_for_task
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
|
||||
from roboco.services.gateway.choreographer._protocol import ChoreographerHelpers
|
||||
|
||||
class QAMixin:
|
||||
_Base = ChoreographerHelpers
|
||||
else:
|
||||
_Base = object
|
||||
|
||||
|
||||
class QAMixin(_Base):
|
||||
"""QA-role verbs."""
|
||||
|
||||
async def claim_review(self, qa_agent_id: UUID, task_id: UUID) -> Envelope:
|
||||
@@ -28,30 +38,30 @@ class QAMixin:
|
||||
journal_highlights, acceptance_criteria_status) INLINE so the QA agent
|
||||
cannot miss the PR data. Marks `qa_evidence_inspected=true` automatically.
|
||||
"""
|
||||
t = await self.task.get(task_id) # type: ignore[attr-defined]
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_review",
|
||||
)
|
||||
if str(t.status) != "awaiting_qa":
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message=(
|
||||
f"task {task_id} is in {t.status}, "
|
||||
"expected awaiting_qa for review"
|
||||
),
|
||||
remediate="call give_me_work() to find an actionable QA task",
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id),
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_review",
|
||||
)
|
||||
|
||||
guard = await self._run_claim_guards( # type: ignore[attr-defined]
|
||||
guard = await self._run_claim_guards(
|
||||
agent_id=qa_agent_id,
|
||||
task=t,
|
||||
skip_role_typed=True,
|
||||
@@ -59,27 +69,27 @@ class QAMixin:
|
||||
skip_sequence=True,
|
||||
)
|
||||
if guard:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
self._with_briefing( # type: ignore[attr-defined]
|
||||
return await self._emit_rejection(
|
||||
self._with_briefing(
|
||||
guard,
|
||||
await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
await self._briefing_for(qa_agent_id, task_id),
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="claim_review",
|
||||
)
|
||||
|
||||
t = await self.task.qa_claim(qa_agent_id, task_id) # type: ignore[attr-defined]
|
||||
await self.task.mark_evidence_inspected(task_id) # type: ignore[attr-defined]
|
||||
t = await self.task.qa_claim(qa_agent_id, task_id)
|
||||
await self.task.mark_evidence_inspected(task_id)
|
||||
|
||||
files_changed: list[str] = []
|
||||
if t.work_session_id:
|
||||
files_changed = await self.work_session.files_changed(t.work_session_id) # type: ignore[attr-defined]
|
||||
files_changed = await self.work_session.files_changed(t.work_session_id)
|
||||
diff_summary = ""
|
||||
if t.branch_name:
|
||||
diff_summary = await self.git.diff(branch_name=t.branch_name) # type: ignore[attr-defined]
|
||||
journal_highlights = (
|
||||
await self.evidence_repo.journal_highlights_for_task(task_id) # type: ignore[attr-defined]
|
||||
diff_summary = await self.git.diff(branch_name=t.branch_name)
|
||||
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
||||
task_id
|
||||
)
|
||||
ev = build_evidence_for_task(
|
||||
t,
|
||||
@@ -95,59 +105,76 @@ class QAMixin:
|
||||
"fail(issues) to request changes."
|
||||
),
|
||||
evidence=ev.as_dict(),
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id),
|
||||
)
|
||||
|
||||
async def _verify_qa_owner(
|
||||
self, qa_agent_id: UUID, task_id: UUID, verb: str
|
||||
) -> tuple[Envelope | None, Any]:
|
||||
"""Lookup task + verify QA is the assignee. Returns (rejection, task)."""
|
||||
t = await self.task.get(task_id)
|
||||
if t is None:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
), None
|
||||
if t.assigned_to != qa_agent_id:
|
||||
return await self._emit_rejection(
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_review(task_id) first",
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id),
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
), None
|
||||
return None, t
|
||||
|
||||
async def _qa_pass_gate_check(
|
||||
self, qa_agent_id: UUID, task_id: UUID, notes: str, t: Any, verb: str
|
||||
) -> Envelope | None:
|
||||
"""QA pass-gate evaluation. Returns rejection envelope or None on pass."""
|
||||
has_learning = await self.journal.has_learning_for_task(qa_agent_id, task_id)
|
||||
missing = self._check_qa_pass_gates(
|
||||
notes=notes,
|
||||
has_learning=has_learning,
|
||||
evidence_inspected=t.qa_evidence_inspected,
|
||||
)
|
||||
if not missing:
|
||||
return None
|
||||
return await self._emit_rejection(
|
||||
self._qa_tracing_gap(
|
||||
missing,
|
||||
task_id,
|
||||
await self._briefing_for(qa_agent_id, task_id),
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb=verb,
|
||||
)
|
||||
|
||||
async def pass_review(
|
||||
self, qa_agent_id: UUID, task_id: UUID, notes: str
|
||||
) -> Envelope:
|
||||
"""QA passes the task; transitions awaiting_qa → awaiting_documentation."""
|
||||
t = await self.task.get(task_id) # type: ignore[attr-defined]
|
||||
if t is None:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="pass_review",
|
||||
)
|
||||
if t.assigned_to != qa_agent_id:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_review(task_id) first",
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="pass_review",
|
||||
rejection, t = await self._verify_qa_owner(qa_agent_id, task_id, "pass_review")
|
||||
if rejection is not None:
|
||||
return rejection
|
||||
gate_rejection = await self._qa_pass_gate_check(
|
||||
qa_agent_id, task_id, notes, t, "pass_review"
|
||||
)
|
||||
if gate_rejection is not None:
|
||||
return gate_rejection
|
||||
|
||||
has_learning = await self.journal.has_learning_for_task( # type: ignore[attr-defined]
|
||||
qa_agent_id, task_id
|
||||
)
|
||||
missing = self._check_qa_pass_gates(
|
||||
notes=notes,
|
||||
has_learning=has_learning,
|
||||
evidence_inspected=t.qa_evidence_inspected,
|
||||
)
|
||||
if missing:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
self._qa_tracing_gap(
|
||||
missing,
|
||||
task_id,
|
||||
await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="pass_review",
|
||||
)
|
||||
t = await self.task.qa_pass(qa_agent_id, task_id, notes)
|
||||
|
||||
t = await self.task.qa_pass(qa_agent_id, task_id, notes) # type: ignore[attr-defined]
|
||||
|
||||
doc_agent = await self.task.documenter_for_team(t.team) # type: ignore[attr-defined]
|
||||
doc_agent = await self.task.documenter_for_team(t.team)
|
||||
if doc_agent is not None:
|
||||
await self.task.reassign(task_id, doc_agent.id) # type: ignore[attr-defined]
|
||||
await self.a2a.send( # type: ignore[attr-defined]
|
||||
await self.task.reassign(task_id, doc_agent.id)
|
||||
await self.a2a.send(
|
||||
from_agent=qa_agent_id,
|
||||
to_agent=doc_agent.id,
|
||||
skill="documentation",
|
||||
@@ -158,7 +185,7 @@ class QAMixin:
|
||||
status=str(t.status),
|
||||
task_id=str(task_id),
|
||||
next="idle until next QA work arrives",
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id),
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
@@ -204,61 +231,31 @@ class QAMixin:
|
||||
self, qa_agent_id: UUID, task_id: UUID, issues: list[str]
|
||||
) -> Envelope:
|
||||
"""QA fails the task with concrete issues; transitions to needs_revision."""
|
||||
t = await self.task.get(task_id) # type: ignore[attr-defined]
|
||||
if t is None:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.not_found(message=f"task {task_id} not found"),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="fail_review",
|
||||
)
|
||||
if t.assigned_to != qa_agent_id:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
Envelope.not_authorized(
|
||||
message="not assigned to you",
|
||||
remediate="claim it via claim_review(task_id) first",
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="fail_review",
|
||||
)
|
||||
rejection, t = await self._verify_qa_owner(qa_agent_id, task_id, "fail_review")
|
||||
if rejection is not None:
|
||||
return rejection
|
||||
if not issues:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
return await self._emit_rejection(
|
||||
Envelope.invalid_state(
|
||||
message="fail_review requires at least one issue",
|
||||
remediate="pass issues=['<concrete actionable issue>', ...]",
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id),
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="fail_review",
|
||||
)
|
||||
|
||||
has_learning = await self.journal.has_learning_for_task( # type: ignore[attr-defined]
|
||||
qa_agent_id, task_id
|
||||
)
|
||||
notes = "Issues:\n" + "\n".join(f"- {issue}" for issue in issues)
|
||||
missing = self._check_qa_pass_gates(
|
||||
notes=notes,
|
||||
has_learning=has_learning,
|
||||
evidence_inspected=t.qa_evidence_inspected,
|
||||
)
|
||||
if missing:
|
||||
return await self._emit_rejection( # type: ignore[attr-defined]
|
||||
self._qa_tracing_gap(
|
||||
missing,
|
||||
task_id,
|
||||
await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
),
|
||||
agent_id=qa_agent_id,
|
||||
task_id=task_id,
|
||||
verb="fail_review",
|
||||
gate_rejection = await self._qa_pass_gate_check(
|
||||
qa_agent_id, task_id, notes, t, "fail_review"
|
||||
)
|
||||
if gate_rejection is not None:
|
||||
return gate_rejection
|
||||
|
||||
t = await self.task.qa_fail(qa_agent_id, task_id, notes, issues) # type: ignore[attr-defined]
|
||||
t = await self.task.qa_fail(qa_agent_id, task_id, notes, issues)
|
||||
if t.assigned_to is not None:
|
||||
await self.a2a.send( # type: ignore[attr-defined]
|
||||
await self.a2a.send(
|
||||
from_agent=qa_agent_id,
|
||||
to_agent=t.assigned_to,
|
||||
skill="code_review",
|
||||
@@ -269,5 +266,5 @@ class QAMixin:
|
||||
status=str(t.status),
|
||||
task_id=str(task_id),
|
||||
next="idle — dev will revise and re-submit",
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id), # type: ignore[attr-defined]
|
||||
context_briefing=await self._briefing_for(qa_agent_id, task_id),
|
||||
)
|
||||
|
||||
+140
-47
@@ -11,6 +11,7 @@ import asyncio
|
||||
import base64
|
||||
import re
|
||||
import subprocess
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING, Any, ClassVar, cast
|
||||
from uuid import UUID
|
||||
@@ -1619,11 +1620,37 @@ class GitService(BaseService):
|
||||
project = await project_service.get(UUID(str(task.project_id)))
|
||||
return project.slug if project else None
|
||||
|
||||
async def _workspace_for_branch(self, branch_name: str) -> Path:
|
||||
@staticmethod
|
||||
def _resolve_workspace_agent_id(
|
||||
task: Any, actor_agent_id: UUID | None
|
||||
) -> UUID | None:
|
||||
"""Workspace-agent resolution priority (audit D-40).
|
||||
|
||||
actor_agent_id → task.assigned_to → task.created_by → None.
|
||||
Centralised so push_branch/create_pr/commit/diff/pr_target/pr_merge
|
||||
share one chain — and individual methods stay below the
|
||||
cyclomatic-complexity gate (xenon B).
|
||||
"""
|
||||
candidate = actor_agent_id or (
|
||||
UUID(str(task.assigned_to)) if task.assigned_to is not None else None
|
||||
)
|
||||
if candidate is None and task.created_by:
|
||||
candidate = UUID(str(task.created_by))
|
||||
return candidate
|
||||
|
||||
async def _workspace_for_branch(
|
||||
self,
|
||||
branch_name: str,
|
||||
*,
|
||||
actor_agent_id: UUID | None = None,
|
||||
) -> Path:
|
||||
"""Get a workspace where this branch can be operated on.
|
||||
|
||||
Uses the assignee's workspace when one is recorded; otherwise
|
||||
falls back to the project's static workspace_path.
|
||||
Resolves the workspace via ``_resolve_workspace_agent_id`` (the
|
||||
actor → assignee → creator fallback chain). Without it, post-
|
||||
handoff calls (e.g. pr_target on a task whose assigned_to was
|
||||
cleared by submit_qa) raise ValidationError when
|
||||
project.workspace_path is unset.
|
||||
"""
|
||||
task = await self._task_for_branch(branch_name)
|
||||
if task is None:
|
||||
@@ -1632,18 +1659,26 @@ class GitService(BaseService):
|
||||
project = await project_service.get(UUID(str(task.project_id)))
|
||||
if project is None:
|
||||
raise NotFoundError("Project", str(task.project_id))
|
||||
agent_id = UUID(str(task.assigned_to)) if task.assigned_to is not None else None
|
||||
return await self.get_workspace(project.slug, agent_id=agent_id)
|
||||
workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id)
|
||||
return await self.get_workspace(project.slug, agent_id=workspace_agent_id)
|
||||
|
||||
async def push_branch(self, branch_name: str) -> tuple[str, int]:
|
||||
async def push_branch(
|
||||
self,
|
||||
branch_name: str,
|
||||
*,
|
||||
actor_agent_id: UUID | None = None,
|
||||
) -> tuple[str, int]:
|
||||
"""Push `branch_name` to origin from the assignee's workspace.
|
||||
|
||||
Gateway-only entry point — the legacy `push(workspace, force)`
|
||||
signature stays intact for non-gateway callers. Resolves the
|
||||
workspace from the task that owns the branch, then delegates.
|
||||
Returns (branch, commits_pushed).
|
||||
workspace from the task that owns the branch (with caller-actor
|
||||
fallback per audit D-40), then delegates. Returns
|
||||
(branch, commits_pushed).
|
||||
"""
|
||||
workspace = await self._workspace_for_branch(branch_name)
|
||||
workspace = await self._workspace_for_branch(
|
||||
branch_name, actor_agent_id=actor_agent_id
|
||||
)
|
||||
return await self.push(workspace)
|
||||
|
||||
async def create_pr(
|
||||
@@ -1652,6 +1687,7 @@ class GitService(BaseService):
|
||||
*,
|
||||
parent: str,
|
||||
is_root_pr: bool,
|
||||
actor_agent_id: UUID | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""Open a PR for `branch_name` targeting `parent`.
|
||||
|
||||
@@ -1659,6 +1695,10 @@ class GitService(BaseService):
|
||||
underlying project + task from the branch name; the gateway never
|
||||
passes a project_slug. The `parent` arg supersedes the task's
|
||||
natural parent so root PRs can target master.
|
||||
|
||||
``actor_agent_id`` lets PMs opening the master PR (where
|
||||
``task.assigned_to`` may be None at completion time) resolve a
|
||||
workspace via the actor's clone (audit D-40).
|
||||
"""
|
||||
task = await self._task_for_branch(branch_name)
|
||||
if task is None:
|
||||
@@ -1668,7 +1708,9 @@ class GitService(BaseService):
|
||||
if project is None:
|
||||
raise NotFoundError("Project", str(task.project_id))
|
||||
|
||||
workspace = await self._workspace_for_branch(branch_name)
|
||||
workspace = await self._workspace_for_branch(
|
||||
branch_name, actor_agent_id=actor_agent_id
|
||||
)
|
||||
git_token = await self._get_project_token_or_raise(project.slug)
|
||||
owner, repo = self._parse_github_remote(workspace)
|
||||
|
||||
@@ -1740,6 +1782,52 @@ class GitService(BaseService):
|
||||
.with_for_update(of=_TaskTable)
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _resolve_merger_id(task: Any, actor_agent_id: UUID | None) -> UUID:
|
||||
"""merged_by attribution priority for pr_merge (audit D-43).
|
||||
|
||||
actor → assigned_to → created_by → UUID(int=0) sentinel.
|
||||
``UUID(0)`` is the explicit "nothing was recoverable" marker
|
||||
instead of the silent NULL we used to write.
|
||||
"""
|
||||
merger = (
|
||||
actor_agent_id
|
||||
or (UUID(str(task.assigned_to)) if task.assigned_to else None)
|
||||
or (UUID(str(task.created_by)) if task.created_by else None)
|
||||
)
|
||||
return merger or UUID(int=0)
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class _MergeContext:
|
||||
"""Bundle of params for `_merge_with_retry` (keeps arg count under 5)."""
|
||||
|
||||
owner: str
|
||||
repo: str
|
||||
pr_number: int
|
||||
git_token: str
|
||||
workspace: Path
|
||||
target: str
|
||||
|
||||
async def _merge_with_retry(self, ctx: GitService._MergeContext) -> Any:
|
||||
"""Single-retry merge: on 409 (race), sync target then retry once."""
|
||||
resp = await self._call_merge_api(
|
||||
ctx.owner, ctx.repo, ctx.pr_number, ctx.git_token, "squash"
|
||||
)
|
||||
if resp.status_code == _HTTP_CONFLICT:
|
||||
# Another PM merged a sibling subtask first and our local target
|
||||
# ref is stale. Refresh and retry once; a second 409 is a real
|
||||
# conflict the PM resolves manually.
|
||||
await self._sync_target_branch(ctx.workspace, ctx.target, ctx.git_token)
|
||||
resp = await self._call_merge_api(
|
||||
ctx.owner, ctx.repo, ctx.pr_number, ctx.git_token, "squash"
|
||||
)
|
||||
if not resp.is_success:
|
||||
raise GitError(
|
||||
f"GitHub API refused PR merge ({resp.status_code}): {resp.text[:200]}",
|
||||
{"owner": ctx.owner, "repo": ctx.repo, "pr": ctx.pr_number},
|
||||
)
|
||||
return resp
|
||||
|
||||
async def pr_merge(
|
||||
self,
|
||||
pr_number: int,
|
||||
@@ -1755,8 +1843,7 @@ class GitService(BaseService):
|
||||
Concurrency: takes a row-level lock on the parent task before
|
||||
invoking the GitHub merge API so that two PMs completing
|
||||
sibling subtasks of the same parent are serialized. On a 409
|
||||
merge conflict (typical race symptom — GitHub serializes via
|
||||
PR-state churn) the local target branch is re-pulled and the
|
||||
merge conflict the local target branch is re-pulled and the
|
||||
merge is retried exactly once before giving up with `GitError`.
|
||||
"""
|
||||
from sqlalchemy import select
|
||||
@@ -1774,53 +1861,47 @@ class GitService(BaseService):
|
||||
if project is None:
|
||||
raise NotFoundError("Project", str(task.project_id))
|
||||
|
||||
# Workspace resolution priority: caller-provided actor (the PM
|
||||
# doing the merge) > task.assigned_to > created_by. assigned_to
|
||||
# is often None at merge time because submit_qa / pass_qa cleared
|
||||
# it during prior transitions; without a fallback the resolver
|
||||
# raises ValidationError when project.workspace_path is unset.
|
||||
workspace_agent_id = actor_agent_id or (
|
||||
UUID(str(task.assigned_to)) if task.assigned_to else None
|
||||
)
|
||||
if workspace_agent_id is None and task.created_by:
|
||||
workspace_agent_id = UUID(str(task.created_by))
|
||||
workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id)
|
||||
workspace = await self.get_workspace(project.slug, agent_id=workspace_agent_id)
|
||||
git_token = await self._get_project_token_or_raise(project.slug)
|
||||
owner, repo = self._parse_github_remote(workspace)
|
||||
|
||||
# Serialize merges into the same parent branch — see helper docstring.
|
||||
parent_id = UUID(str(task.parent_task_id)) if task.parent_task_id else None
|
||||
await self._lock_parent_task_for_merge(parent_id)
|
||||
|
||||
resp = await self._call_merge_api(owner, repo, pr_number, git_token, "squash")
|
||||
if resp.status_code == _HTTP_CONFLICT:
|
||||
# Race symptom — another PM merged a sibling subtask first
|
||||
# and our local target ref is stale. Refresh and retry once;
|
||||
# if the second attempt also conflicts, it's a real conflict
|
||||
# (not just a race) and the choreographer surfaces it as
|
||||
# `invalid_state` so the PM can resolve it manually.
|
||||
await self._sync_target_branch(workspace, target, git_token)
|
||||
resp = await self._call_merge_api(
|
||||
owner, repo, pr_number, git_token, "squash"
|
||||
await self._merge_with_retry(
|
||||
self._MergeContext(
|
||||
owner=owner,
|
||||
repo=repo,
|
||||
pr_number=pr_number,
|
||||
git_token=git_token,
|
||||
workspace=workspace,
|
||||
target=target,
|
||||
)
|
||||
if not resp.is_success:
|
||||
raise GitError(
|
||||
f"GitHub API refused PR merge ({resp.status_code}): {resp.text[:200]}",
|
||||
{"owner": owner, "repo": repo, "pr": pr_number},
|
||||
)
|
||||
await self._delete_pr_branch_best_effort(owner, repo, pr_number, git_token)
|
||||
|
||||
merge_commit = await self._sync_target_branch(workspace, target, git_token)
|
||||
|
||||
if task.work_session_id:
|
||||
ws_service = get_work_session_service(self.session)
|
||||
await ws_service.merge_pr(
|
||||
require_uuid(task.work_session_id),
|
||||
UUID(str(task.assigned_to)) if task.assigned_to else UUID(int=0),
|
||||
self._resolve_merger_id(task, actor_agent_id),
|
||||
)
|
||||
return {"merge_commit_sha": merge_commit or None}
|
||||
|
||||
async def pr_target(self, pr_number: int) -> str:
|
||||
"""Return the current target (base) branch of an open PR."""
|
||||
async def pr_target(
|
||||
self,
|
||||
pr_number: int,
|
||||
*,
|
||||
actor_agent_id: UUID | None = None,
|
||||
) -> str:
|
||||
"""Return the current target (base) branch of an open PR.
|
||||
|
||||
Workspace resolution mirrors pr_merge: actor → assigned_to →
|
||||
created_by (audit D-40). Lets the Main PM call pr_target after
|
||||
``submit_qa`` has cleared ``assigned_to`` without ValidationError.
|
||||
"""
|
||||
from sqlalchemy import select
|
||||
|
||||
from roboco.db.tables import TaskTable as _TaskTable
|
||||
@@ -1836,10 +1917,8 @@ class GitService(BaseService):
|
||||
if project is None:
|
||||
raise NotFoundError("Project", str(task.project_id))
|
||||
|
||||
workspace = await self.get_workspace(
|
||||
project.slug,
|
||||
agent_id=UUID(str(task.assigned_to)) if task.assigned_to else None,
|
||||
)
|
||||
workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id)
|
||||
workspace = await self.get_workspace(project.slug, agent_id=workspace_agent_id)
|
||||
owner, repo = self._parse_github_remote(workspace)
|
||||
git_token = await self._get_project_token_or_raise(project.slug)
|
||||
|
||||
@@ -1875,6 +1954,7 @@ class GitService(BaseService):
|
||||
*,
|
||||
branch_name: str,
|
||||
base: str | None = None,
|
||||
actor_agent_id: UUID | None = None,
|
||||
) -> str:
|
||||
"""Return the git diff for `branch_name` against `base`.
|
||||
|
||||
@@ -1882,10 +1962,16 @@ class GitService(BaseService):
|
||||
`parent_branch_for`) which is what the choreographer/PR-review
|
||||
path wants. Content_actions evidence path can pass `HEAD~1` to
|
||||
get just the latest change diff for incremental review.
|
||||
|
||||
``actor_agent_id`` resolves the workspace via the caller's clone
|
||||
when ``task.assigned_to`` is None (audit D-40) — important for
|
||||
QA reviewing post-submit_qa.
|
||||
"""
|
||||
from roboco.services.gateway.merge_chain import parent_branch_for
|
||||
|
||||
workspace = await self._workspace_for_branch(branch_name)
|
||||
workspace = await self._workspace_for_branch(
|
||||
branch_name, actor_agent_id=actor_agent_id
|
||||
)
|
||||
if base is None:
|
||||
parent = parent_branch_for(branch_name)
|
||||
# Make sure the parent ref exists locally before diffing.
|
||||
@@ -1903,6 +1989,7 @@ class GitService(BaseService):
|
||||
message: str,
|
||||
task_id: UUID,
|
||||
files: list[str] | None = None,
|
||||
actor_agent_id: UUID | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""Gateway adapter — commit on `branch_name` with a free-form message.
|
||||
|
||||
@@ -1913,12 +2000,18 @@ class GitService(BaseService):
|
||||
message is descriptive, and the orchestrator-side git template only
|
||||
applies to the structured `commit_for_task` API path.
|
||||
|
||||
``actor_agent_id`` falls back through the same chain as pr_merge
|
||||
when ``task.assigned_to`` was cleared by an earlier transition
|
||||
(audit D-40).
|
||||
|
||||
Returns a dict shaped for the gateway: ``{"sha": str, "message": str,
|
||||
"files_changed": int, "insertions": int, "deletions": int}``. Tests
|
||||
and downstream gateway code only consume `sha`; the rest is included
|
||||
so we don't have to invent a new shape later.
|
||||
"""
|
||||
workspace = await self._workspace_for_branch(branch_name)
|
||||
workspace = await self._workspace_for_branch(
|
||||
branch_name, actor_agent_id=actor_agent_id
|
||||
)
|
||||
await self._assert_on_task_branch(workspace, branch_name)
|
||||
|
||||
# Stage files explicitly when provided; otherwise stage everything
|
||||
|
||||
@@ -287,30 +287,22 @@ class NotificationService:
|
||||
)
|
||||
)
|
||||
|
||||
async def _create_notification(self, params: CreateNotificationParams) -> None:
|
||||
"""Create a notification via the database and deliver it."""
|
||||
async with get_db_context() as db:
|
||||
from_agent_uuid = await _resolve_agent_uuid(db, params.from_agent)
|
||||
if from_agent_uuid is None:
|
||||
# Caller passed "system" / "unknown" / an unknown slug.
|
||||
# notifications.from_agent is NOT NULL + FK to agents.id,
|
||||
# so we cannot insert. Skip-with-warn rather than crash
|
||||
# the upstream request — the notification would be
|
||||
# orphaned anyway (no sender the recipient could reply to).
|
||||
logger.warning(
|
||||
"Skipping notification: from_agent could not be resolved to an agent UUID", # noqa: E501
|
||||
from_agent_input=str(params.from_agent),
|
||||
type=params.notification_type.value
|
||||
if hasattr(params.notification_type, "value")
|
||||
else str(params.notification_type),
|
||||
subject=params.subject[:80],
|
||||
to_agents=[str(a) for a in params.to_agents],
|
||||
)
|
||||
return
|
||||
# notifications.to_agents is UUID[] — callers across the codebase
|
||||
# pass slugs ("be-dev-1", "be-qa"). Resolve every recipient before
|
||||
# insert; drop (with warn) any that don't resolve instead of
|
||||
# letting asyncpg crash with "invalid UUID 'be-dev-1'".
|
||||
@staticmethod
|
||||
def _notification_type_label(params: CreateNotificationParams) -> str:
|
||||
"""Render the notification_type for a log line."""
|
||||
nt = params.notification_type
|
||||
return nt.value if hasattr(nt, "value") else str(nt)
|
||||
|
||||
async def _resolve_recipients(
|
||||
self, db: Any, params: CreateNotificationParams
|
||||
) -> list[UUID]:
|
||||
"""Resolve to_agents (slugs/UUIDs) to UUID list. Drops unresolvable.
|
||||
|
||||
notifications.to_agents is UUID[] — callers across the codebase
|
||||
pass slugs ("be-dev-1", "be-qa"). Resolve every recipient before
|
||||
insert; drop (with warn) any that don't resolve instead of
|
||||
letting asyncpg crash with "invalid UUID 'be-dev-1'".
|
||||
"""
|
||||
to_agents_uuids: list[UUID] = []
|
||||
unresolved: list[str] = []
|
||||
for recipient in params.to_agents:
|
||||
@@ -323,18 +315,33 @@ class NotificationService:
|
||||
logger.warning(
|
||||
"Dropping unresolved notification recipients",
|
||||
unresolved=unresolved,
|
||||
type=params.notification_type.value
|
||||
if hasattr(params.notification_type, "value")
|
||||
else str(params.notification_type),
|
||||
type=self._notification_type_label(params),
|
||||
subject=params.subject[:80],
|
||||
)
|
||||
return to_agents_uuids
|
||||
|
||||
async def _create_notification(self, params: CreateNotificationParams) -> None:
|
||||
"""Create a notification via the database and deliver it."""
|
||||
async with get_db_context() as db:
|
||||
from_agent_uuid = await _resolve_agent_uuid(db, params.from_agent)
|
||||
if from_agent_uuid is None:
|
||||
# notifications.from_agent is NOT NULL + FK to agents.id, so
|
||||
# we cannot insert. Skip-with-warn rather than crash the
|
||||
# upstream request.
|
||||
logger.warning(
|
||||
"Skipping notification: from_agent unresolvable",
|
||||
from_agent_input=str(params.from_agent),
|
||||
type=self._notification_type_label(params),
|
||||
subject=params.subject[:80],
|
||||
to_agents=[str(a) for a in params.to_agents],
|
||||
)
|
||||
return
|
||||
to_agents_uuids = await self._resolve_recipients(db, params)
|
||||
if not to_agents_uuids:
|
||||
logger.warning(
|
||||
"Skipping notification: no resolvable recipients",
|
||||
to_agents_input=[str(a) for a in params.to_agents],
|
||||
type=params.notification_type.value
|
||||
if hasattr(params.notification_type, "value")
|
||||
else str(params.notification_type),
|
||||
type=self._notification_type_label(params),
|
||||
subject=params.subject[:80],
|
||||
)
|
||||
return
|
||||
|
||||
+31
-19
@@ -135,6 +135,35 @@ class ProviderService(BaseService):
|
||||
)
|
||||
return row
|
||||
|
||||
async def _apply_name_change(self, row: ProviderConfigTable, new_name: str) -> None:
|
||||
"""Set row.name with duplicate-name guard."""
|
||||
if new_name == row.name:
|
||||
return
|
||||
dup = await self.get_by_name(new_name)
|
||||
if dup and dup.id != row.id:
|
||||
raise ConflictError(
|
||||
f"Provider with name '{new_name}' already exists",
|
||||
resource_type="provider",
|
||||
)
|
||||
row.name = new_name
|
||||
|
||||
def _apply_auth_token_change(
|
||||
self, row: ProviderConfigTable, data: ProviderUpdate
|
||||
) -> None:
|
||||
"""Tri-state token update: clear, set, or leave unchanged."""
|
||||
if data.clear_auth_token:
|
||||
row.auth_token_encrypted = None
|
||||
self.log.info("Provider auth token cleared", provider_id=str(row.id))
|
||||
return
|
||||
if not data.auth_token:
|
||||
return
|
||||
try:
|
||||
row.auth_token_encrypted = encrypt_token(data.auth_token)
|
||||
except EncryptionError as e:
|
||||
self.log.error("Failed to encrypt auth token", error=str(e))
|
||||
raise
|
||||
self.log.info("Provider auth token updated", provider_id=str(row.id))
|
||||
|
||||
async def update_provider(
|
||||
self, provider_id: UUID, data: ProviderUpdate
|
||||
) -> ProviderConfigTable | None:
|
||||
@@ -144,31 +173,14 @@ class ProviderService(BaseService):
|
||||
return None
|
||||
|
||||
if data.name is not None:
|
||||
# Duplicate-name check only when actually changing the name.
|
||||
if data.name != row.name:
|
||||
dup = await self.get_by_name(data.name)
|
||||
if dup and dup.id != row.id:
|
||||
raise ConflictError(
|
||||
f"Provider with name '{data.name}' already exists",
|
||||
resource_type="provider",
|
||||
)
|
||||
row.name = data.name
|
||||
await self._apply_name_change(row, data.name)
|
||||
if data.base_url is not None:
|
||||
# Empty string → clear to NULL (matches git-token convention).
|
||||
row.base_url = data.base_url or None
|
||||
if data.enabled is not None:
|
||||
row.enabled = data.enabled
|
||||
|
||||
if data.clear_auth_token:
|
||||
row.auth_token_encrypted = None
|
||||
self.log.info("Provider auth token cleared", provider_id=str(row.id))
|
||||
elif data.auth_token:
|
||||
try:
|
||||
row.auth_token_encrypted = encrypt_token(data.auth_token)
|
||||
except EncryptionError as e:
|
||||
self.log.error("Failed to encrypt auth token", error=str(e))
|
||||
raise
|
||||
self.log.info("Provider auth token updated", provider_id=str(row.id))
|
||||
self._apply_auth_token_change(row, data)
|
||||
|
||||
await self.session.flush()
|
||||
return row
|
||||
|
||||
+88
-11
@@ -1048,7 +1048,18 @@ class TaskService(BaseService):
|
||||
async def _inject_proactive_context(self, task: TaskTable, agent_id: UUID) -> None:
|
||||
"""Inject proactive knowledge context when task is claimed.
|
||||
|
||||
Runs as a background task, so uses its own database session.
|
||||
Runs as a background task with its own DB session. Pre-fix the
|
||||
outer claim() transaction could roll back (branch-creation
|
||||
failure, FOR UPDATE conflict, etc.) but this fire-and-forget
|
||||
survived and wrote stale context onto a task whose claim was
|
||||
reverted (audit D-44).
|
||||
|
||||
Now performs a confirm-after-commit check at the top: re-reads
|
||||
the task in a fresh session and skips if (a) task is gone, or
|
||||
(b) the claim is no longer held by ``agent_id``. Outer rollback
|
||||
clears ``assigned_to``, so this guard is enough to avoid stale
|
||||
writes; it also fires correctly under successful commits because
|
||||
a fresh read sees the post-commit state.
|
||||
"""
|
||||
from uuid import UUID as PyUUID
|
||||
|
||||
@@ -1060,6 +1071,17 @@ class TaskService(BaseService):
|
||||
task_description = task.description or ""
|
||||
|
||||
try:
|
||||
session_factory = get_session_factory()
|
||||
async with session_factory() as session:
|
||||
fresh = await session.get(TaskTable, task_id)
|
||||
if fresh is None or fresh.assigned_to != agent_id:
|
||||
self.log.debug(
|
||||
"skipping proactive context — claim was rolled back"
|
||||
" or task is gone",
|
||||
task_id=str(task_id),
|
||||
)
|
||||
return
|
||||
|
||||
proactive = await get_proactive_service()
|
||||
agent_uuid = PyUUID(str(agent_id))
|
||||
|
||||
@@ -1072,8 +1094,6 @@ class TaskService(BaseService):
|
||||
)
|
||||
|
||||
if context and not context.is_empty():
|
||||
# Store context in the task using a fresh session
|
||||
session_factory = get_session_factory()
|
||||
async with session_factory() as session:
|
||||
from sqlalchemy import update
|
||||
|
||||
@@ -1857,6 +1877,12 @@ class TaskService(BaseService):
|
||||
VALID_TRANSITIONS — making the lifecycle module's invariants diverge
|
||||
from production reality.
|
||||
|
||||
Also abandons the active WorkSession so a re-claim by the same
|
||||
agent doesn't trip the uniqueness constraint at
|
||||
``WorkSessionService.create`` (audit D-41). Best-effort: if the
|
||||
WorkSession lookup fails for any reason, the task is still
|
||||
rolled back to pending.
|
||||
|
||||
The operation is named with ``_for_reaper`` so callers cannot
|
||||
accidentally use it as a regular unclaim path; uses ``agent_role=None``
|
||||
because the system itself is performing the transition. Bypasses
|
||||
@@ -1872,11 +1898,40 @@ class TaskService(BaseService):
|
||||
self._validate_and_set_status(task, TaskStatus.PENDING, None)
|
||||
except TaskLifecycleError:
|
||||
return
|
||||
if task.work_session_id:
|
||||
await self._abandon_work_session_best_effort(
|
||||
task.work_session_id, reason="reaper-unclaim"
|
||||
)
|
||||
task.work_session_id = cast("Any", None)
|
||||
task.assigned_to = cast("Any", None)
|
||||
task.last_heartbeat_at = None
|
||||
task.active_claimant_id = cast("Any", None)
|
||||
await self.session.flush()
|
||||
|
||||
async def _abandon_work_session_best_effort(
|
||||
self, session_id: Any, *, reason: str
|
||||
) -> None:
|
||||
"""Mark a WorkSession ABANDONED. Logs and continues on any failure.
|
||||
|
||||
Audit D-41 fix — unclaim must not leave ACTIVE WorkSessions
|
||||
behind, but a service-layer error here mustn't block the task-
|
||||
side unclaim from completing.
|
||||
"""
|
||||
try:
|
||||
from roboco.services.work_session import (
|
||||
WorkSessionService,
|
||||
)
|
||||
|
||||
ws_service = WorkSessionService(self.session)
|
||||
await ws_service.abandon(UUID(str(session_id)), reason=reason)
|
||||
except Exception as exc:
|
||||
self.log.warning(
|
||||
"abandon WorkSession failed; continuing",
|
||||
session_id=str(session_id),
|
||||
reason=reason,
|
||||
error=str(exc),
|
||||
)
|
||||
|
||||
async def unclaim_for_agent(
|
||||
self, task_id: UUID, agent_id: UUID
|
||||
) -> TaskTable | None:
|
||||
@@ -1925,7 +1980,14 @@ class TaskService(BaseService):
|
||||
return None
|
||||
|
||||
# _validate_and_set_status only updates `status`; clearing the
|
||||
# claim is the unclaim's specific side effect.
|
||||
# claim is the unclaim's specific side effect. Also abandon the
|
||||
# active WorkSession so a re-claim doesn't trip the uniqueness
|
||||
# constraint (audit D-41).
|
||||
if task.work_session_id:
|
||||
await self._abandon_work_session_best_effort(
|
||||
task.work_session_id, reason="agent-unclaim"
|
||||
)
|
||||
task.work_session_id = cast("Any", None)
|
||||
task.assigned_to = cast("Any", None)
|
||||
task.active_claimant_id = cast("Any", None)
|
||||
await self.session.flush()
|
||||
@@ -4217,22 +4279,26 @@ class TaskService(BaseService):
|
||||
await self.session.commit()
|
||||
return completed
|
||||
|
||||
async def escalate_to_ceo_for_agent(
|
||||
async def _validate_escalation_preconditions(
|
||||
self,
|
||||
task: TaskTable,
|
||||
task_id: UUID,
|
||||
agent: AgentContext,
|
||||
permissions: "PermissionService",
|
||||
notes: str | None,
|
||||
) -> TaskTable:
|
||||
"""Escalate a task to CEO for final approval (PM-role, PR-gated)."""
|
||||
task = await self._load_task_or_raise(task_id)
|
||||
) -> None:
|
||||
"""Run all PR/permission/descendants/notes gates for escalate-to-CEO.
|
||||
|
||||
Raises UnauthorizedError or ValidationError on failure; returns
|
||||
cleanly when every gate passes. Extracted from
|
||||
``escalate_to_ceo_for_agent`` to keep that orchestrating method
|
||||
below B-rank cyclomatic complexity (audit P2-2 / xenon).
|
||||
"""
|
||||
if not permissions.can_perform_task_action(agent, TaskAction.CLOSE, task.team):
|
||||
raise UnauthorizedError(
|
||||
action="escalate_to_ceo",
|
||||
reason="Only PMs can escalate tasks to CEO",
|
||||
)
|
||||
|
||||
if task.pr_number is None:
|
||||
raise ValidationError(
|
||||
"NO_PR: Cannot escalate to CEO without an open PR. Ensure "
|
||||
@@ -4243,7 +4309,6 @@ class TaskService(BaseService):
|
||||
"PR_NOT_CONFIRMED: pr_created flag is false. The PR handler "
|
||||
"must confirm the PR exists before escalation."
|
||||
)
|
||||
|
||||
descendants = await self.get_all_descendants(task_id)
|
||||
active = [
|
||||
d
|
||||
@@ -4256,7 +4321,6 @@ class TaskService(BaseService):
|
||||
f"ACTIVE_SUBTASKS: Cannot escalate while {len(active)} "
|
||||
f"subtask(s) remain active: {ids_shown}."
|
||||
)
|
||||
|
||||
if not notes or len(notes.strip()) < self.MIN_NOTES_CHARS:
|
||||
raise ValidationError(
|
||||
f"ESCALATION_NOTES_REQUIRED: Escalation to CEO must "
|
||||
@@ -4265,6 +4329,19 @@ class TaskService(BaseService):
|
||||
"etc)."
|
||||
)
|
||||
|
||||
async def escalate_to_ceo_for_agent(
|
||||
self,
|
||||
task_id: UUID,
|
||||
agent: AgentContext,
|
||||
permissions: "PermissionService",
|
||||
notes: str | None,
|
||||
) -> TaskTable:
|
||||
"""Escalate a task to CEO for final approval (PM-role, PR-gated)."""
|
||||
task = await self._load_task_or_raise(task_id)
|
||||
await self._validate_escalation_preconditions(
|
||||
task, task_id, agent, permissions, notes
|
||||
)
|
||||
|
||||
escalated = await self.escalate_to_ceo(task_id, agent.role.value, notes)
|
||||
if not escalated:
|
||||
raise ValidationError(
|
||||
|
||||
@@ -72,8 +72,9 @@ class _StubGit:
|
||||
message: str,
|
||||
task_id: UUID,
|
||||
files: list[str] | None = None,
|
||||
actor_agent_id: Any = None,
|
||||
) -> dict[str, Any]:
|
||||
del branch_name, files
|
||||
del branch_name, files, actor_agent_id
|
||||
sha = uuid4().hex[:40]
|
||||
commits = list(self._task.commits or [])
|
||||
commits.append({"sha": sha, "message": message, "task_id": str(task_id)})
|
||||
@@ -87,23 +88,38 @@ class _StubGit:
|
||||
"deletions": 0,
|
||||
}
|
||||
|
||||
async def push_branch(self, branch_name: str) -> tuple[str, int]:
|
||||
del branch_name
|
||||
async def push_branch(
|
||||
self, branch_name: str, *, actor_agent_id: Any = None
|
||||
) -> tuple[str, int]:
|
||||
del branch_name, actor_agent_id
|
||||
return ("ok", 0)
|
||||
|
||||
async def create_pr(
|
||||
self, branch_name: str, *, parent: str, is_root_pr: bool
|
||||
self,
|
||||
branch_name: str,
|
||||
*,
|
||||
parent: str,
|
||||
is_root_pr: bool,
|
||||
actor_agent_id: Any = None,
|
||||
) -> dict[str, Any]:
|
||||
del branch_name, parent
|
||||
del branch_name, parent, actor_agent_id
|
||||
self._task.pr_number = _PR_NUMBER
|
||||
self._task.pr_url = _PR_URL
|
||||
# Mirrors git._record_pr_atomically — production sets pr_created
|
||||
# via mark_pr_created which is what the parallel-completion gate
|
||||
# in _maybe_advance_to_pm_review reads.
|
||||
self._task.pr_created = True
|
||||
await self._session.flush()
|
||||
return {"pr_number": _PR_NUMBER, "pr_url": _PR_URL, "is_root_pr": is_root_pr}
|
||||
|
||||
async def diff(self, *, branch_name: str) -> str: # noqa: ARG002
|
||||
async def diff(
|
||||
self, *, branch_name: str, base: Any = None, actor_agent_id: Any = None
|
||||
) -> str:
|
||||
del branch_name, base, actor_agent_id
|
||||
return "stub diff"
|
||||
|
||||
async def pr_target(self, pr_number: int) -> str: # noqa: ARG002
|
||||
async def pr_target(self, pr_number: int, *, actor_agent_id: Any = None) -> str:
|
||||
del pr_number, actor_agent_id
|
||||
return "main"
|
||||
|
||||
async def pr_merge(self, **kwargs: Any) -> dict[str, Any]:
|
||||
@@ -204,7 +220,33 @@ async def lifecycle_setup(
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add_all([dev_agent, qa_agent])
|
||||
doc_agent = AgentTable(
|
||||
id=uuid4(),
|
||||
name="BE Doc",
|
||||
slug=f"be-doc-{uuid4().hex[:8]}",
|
||||
role=AgentRole.DOCUMENTER,
|
||||
team=Team.BACKEND,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="doc",
|
||||
capabilities=["docs"],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
cell_pm_agent = AgentTable(
|
||||
id=uuid4(),
|
||||
name="BE Cell PM",
|
||||
slug=f"be-pm-{uuid4().hex[:8]}",
|
||||
role=AgentRole.CELL_PM,
|
||||
team=Team.BACKEND,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="cell_pm",
|
||||
capabilities=["coord"],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add_all([dev_agent, qa_agent, doc_agent, cell_pm_agent])
|
||||
await db_session.flush()
|
||||
|
||||
task = TaskTable(
|
||||
@@ -233,6 +275,8 @@ async def lifecycle_setup(
|
||||
"project": project,
|
||||
"dev_agent": dev_agent,
|
||||
"qa_agent": qa_agent,
|
||||
"doc_agent": doc_agent,
|
||||
"cell_pm_agent": cell_pm_agent,
|
||||
"task": task,
|
||||
}
|
||||
|
||||
@@ -337,13 +381,90 @@ async def test_dev_full_chain_through_awaiting_qa(
|
||||
assert final.self_verified is True, "P1-3: self_verified set by auto-verify"
|
||||
|
||||
|
||||
# TODO P2-1 follow-up — extend the chain past awaiting_qa:
|
||||
# - QA: claim_review → pass → awaiting_documentation
|
||||
# - Documenter: claim_doc_task → i_documented → awaiting_pm_review
|
||||
# - Cell PM: complete on the leaf → completed (or submit_up to a parent)
|
||||
# - Main PM: complete on the root → awaiting_ceo_approval
|
||||
# Each stage needs the role's agent seeded (lifecycle_setup already has
|
||||
# dev + qa; add doc + cell_pm + main_pm) plus journal entries with the
|
||||
# right scope (pass needs journal:learning; complete needs journal:decision).
|
||||
# The _StubGit class above already covers commit/push/pr_create/pr_target
|
||||
# /pr_merge for the merge stages.
|
||||
@pytest.mark.asyncio
|
||||
async def test_full_chain_through_doc_handoff(
|
||||
db_session: AsyncSession, lifecycle_setup: dict[str, Any]
|
||||
) -> None:
|
||||
"""Extend the dev chain: QA pass → documenter → awaiting_pm_review.
|
||||
|
||||
Verifies QA pass clears active_claimant_id (P1-4 + P1-5),
|
||||
docs_complete transitions to awaiting_pm_review, and reassignment
|
||||
to the cell PM happens on hand-off.
|
||||
"""
|
||||
task = lifecycle_setup["task"]
|
||||
dev_agent = lifecycle_setup["dev_agent"]
|
||||
qa_agent = lifecycle_setup["qa_agent"]
|
||||
doc_agent = lifecycle_setup["doc_agent"]
|
||||
cell_pm_agent = lifecycle_setup["cell_pm_agent"]
|
||||
task_service = TaskService(db_session)
|
||||
stub_git = _StubGit(db_session, task)
|
||||
|
||||
deps = ChoreographerDeps(
|
||||
task=task_service,
|
||||
work_session=_mock_work_session(),
|
||||
git=stub_git,
|
||||
a2a=AsyncMock(),
|
||||
journal=_mock_journal_with_reflect(),
|
||||
audit=AsyncMock(),
|
||||
evidence_repo=_mock_evidence_repo(),
|
||||
)
|
||||
c = Choreographer(deps)
|
||||
|
||||
# Drive the dev side first (same as test_dev_full_chain_through_awaiting_qa).
|
||||
await c.i_will_work_on(dev_agent.id, task.id, plan="add the route")
|
||||
await stub_git.commit(
|
||||
branch_name=_BRANCH,
|
||||
message=f"[{str(task.id)[:8]}] feat(api): add /healthz",
|
||||
task_id=task.id,
|
||||
)
|
||||
await task_service.add_progress(task.id, dev_agent.id, "implemented /healthz")
|
||||
await c.submit_for_qa(dev_agent.id, task.id)
|
||||
env = await c.i_am_done(dev_agent.id, task.id, "tests pass; route works")
|
||||
assert env.error is None
|
||||
assert env.status == "awaiting_qa"
|
||||
|
||||
# QA path: claim_review → pass.
|
||||
env = await c.claim_review(qa_agent.id, task.id)
|
||||
assert env.error is None, f"claim_review failed: {env.message}"
|
||||
|
||||
qa_notes = (
|
||||
"Reviewed the diff; route returns 200 OK with timestamp. Tests cover "
|
||||
"both acceptance criteria. Approving."
|
||||
)
|
||||
env = await c.pass_review(qa_agent.id, task.id, notes=qa_notes)
|
||||
assert env.error is None, f"pass_review failed: {env.message}"
|
||||
assert env.status == "awaiting_documentation"
|
||||
|
||||
after_qa = await task_service.get(task.id)
|
||||
assert after_qa is not None
|
||||
assert after_qa.active_claimant_id is None, (
|
||||
"P1-4 + P1-5: QA pass must clear active_claimant_id for next role"
|
||||
)
|
||||
|
||||
# Documenter path: claim_doc_task → i_documented.
|
||||
env = await c.claim_doc_task(doc_agent.id, task.id)
|
||||
assert env.error is None, f"claim_doc_task failed: {env.message}"
|
||||
|
||||
env = await c.i_documented(
|
||||
doc_agent.id,
|
||||
task.id,
|
||||
notes="Documented /healthz behaviour in docs/api/health.md",
|
||||
files=["docs/api/health.md"],
|
||||
)
|
||||
assert env.error is None, f"i_documented failed: {env.message}"
|
||||
assert env.status == "awaiting_pm_review", (
|
||||
"P2-1: i_documented must transition awaiting_documentation → awaiting_pm_review"
|
||||
)
|
||||
|
||||
after_docs = await task_service.get(task.id)
|
||||
assert after_docs is not None
|
||||
assert after_docs.assigned_to == cell_pm_agent.id, (
|
||||
"P2-1: docs_complete must reassign to the cell PM for the team"
|
||||
)
|
||||
|
||||
|
||||
# TODO P2-1 follow-up — final stages (cell_pm complete + main_pm complete +
|
||||
# CEO approval) require additional setup: a parent task hierarchy for
|
||||
# the merge chain, plus a real `git.pr_merge` simulation that updates
|
||||
# the underlying repo. The _StubGit class covers the API surface; what's
|
||||
# missing is the seeded parent task + main_pm agent.
|
||||
|
||||
@@ -0,0 +1,148 @@
|
||||
"""P0-7 / S-01: branch creation atomicity.
|
||||
|
||||
When ``_ensure_branch_for_task`` raises (git checkout fails, push fails,
|
||||
no token, etc.), ``_finalize_claim`` must roll back the claim fields it
|
||||
just flushed — otherwise the task is left CLAIMED with branch_name=NULL
|
||||
and the next claim attempt collides on a non-idempotent
|
||||
``git checkout -b``.
|
||||
|
||||
This test exercises the rollback path against a real Postgres session
|
||||
by patching ``_ensure_branch_for_task`` to raise.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING, Any
|
||||
from unittest.mock import patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
import pytest_asyncio
|
||||
from roboco.db.tables import AgentTable, ProjectTable, TaskTable
|
||||
from roboco.models.base import (
|
||||
AgentRole,
|
||||
AgentStatus,
|
||||
TaskNature,
|
||||
TaskStatus,
|
||||
TaskType,
|
||||
Team,
|
||||
)
|
||||
from roboco.services.task import TaskService
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import AsyncIterator
|
||||
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def claim_setup(db_session: AsyncSession) -> AsyncIterator[dict[str, Any]]:
|
||||
system_agent = AgentTable(
|
||||
id=uuid4(),
|
||||
name="System",
|
||||
slug=f"system-{uuid4().hex[:8]}",
|
||||
role=AgentRole.SYSTEM,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="system",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(system_agent)
|
||||
await db_session.flush()
|
||||
|
||||
project = ProjectTable(
|
||||
id=uuid4(),
|
||||
name="Atom Test",
|
||||
slug=f"atom-{uuid4().hex[:8]}",
|
||||
git_url="https://github.com/example/atom.git",
|
||||
default_branch="main",
|
||||
protected_branches=["main"],
|
||||
assigned_cell=Team.BACKEND,
|
||||
created_by=system_agent.id,
|
||||
is_active=True,
|
||||
)
|
||||
db_session.add(project)
|
||||
await db_session.flush()
|
||||
|
||||
dev = AgentTable(
|
||||
id=uuid4(),
|
||||
name="BE Dev",
|
||||
slug=f"be-dev-{uuid4().hex[:8]}",
|
||||
role=AgentRole.DEVELOPER,
|
||||
team=Team.BACKEND,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="dev",
|
||||
capabilities=["python"],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(dev)
|
||||
await db_session.flush()
|
||||
|
||||
task = TaskTable(
|
||||
id=uuid4(),
|
||||
title="Task that will fail at branch creation",
|
||||
description="…",
|
||||
status=TaskStatus.PENDING,
|
||||
priority=2,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
team=Team.BACKEND,
|
||||
project_id=project.id,
|
||||
created_by=system_agent.id,
|
||||
assigned_to=dev.id,
|
||||
acceptance_criteria=["does the thing"],
|
||||
# No branch_name — claim path will try to create one.
|
||||
)
|
||||
db_session.add(task)
|
||||
await db_session.flush()
|
||||
yield {"task": task, "dev": dev, "project": project}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_finalize_claim_rolls_back_on_branch_failure(
|
||||
db_session: AsyncSession, claim_setup: dict[str, Any]
|
||||
) -> None:
|
||||
"""git failure during _ensure_branch_for_task must revert claim fields.
|
||||
|
||||
Without rollback the task is left CLAIMED with branch_name=NULL and
|
||||
`git checkout -b` is non-idempotent on retry.
|
||||
"""
|
||||
task = claim_setup["task"]
|
||||
dev = claim_setup["dev"]
|
||||
svc = TaskService(db_session)
|
||||
|
||||
# Snapshot pre-claim state so we can assert exact rollback.
|
||||
pre_status = task.status
|
||||
pre_assigned = task.assigned_to
|
||||
pre_claimed_by = task.claimed_by
|
||||
pre_claimed_at = task.claimed_at
|
||||
pre_heartbeat = task.last_heartbeat_at
|
||||
pre_claimant = task.active_claimant_id
|
||||
|
||||
async def boom(_self: Any, _task: Any, _agent_id: Any) -> str:
|
||||
raise RuntimeError("simulated: git checkout -b failed")
|
||||
|
||||
with (
|
||||
patch.object(TaskService, "_ensure_branch_for_task", boom),
|
||||
pytest.raises(RuntimeError, match="git checkout -b failed"),
|
||||
):
|
||||
await svc.claim(task.id, dev.id)
|
||||
|
||||
# Re-read the task from a clean state via a fresh fetch.
|
||||
refreshed = await svc.get(task.id)
|
||||
assert refreshed is not None
|
||||
assert refreshed.status == pre_status, "P0-7: status must roll back"
|
||||
assert refreshed.assigned_to == pre_assigned, "P0-7: assigned_to must roll back"
|
||||
assert refreshed.claimed_by == pre_claimed_by, "P0-7: claimed_by must roll back"
|
||||
assert refreshed.claimed_at == pre_claimed_at, "P0-7: claimed_at must roll back"
|
||||
assert refreshed.last_heartbeat_at == pre_heartbeat, (
|
||||
"P0-7: heartbeat must roll back"
|
||||
)
|
||||
assert refreshed.active_claimant_id == pre_claimant, (
|
||||
"P1-4 + P0-7: active_claimant_id must roll back too"
|
||||
)
|
||||
@@ -0,0 +1,149 @@
|
||||
"""P2-8: startup orphan-claim reconciler.
|
||||
|
||||
The orchestrator's `_reconcile_orphan_claims_on_startup` rolls back
|
||||
tasks left in CLAIMED/IN_PROGRESS with `branch_name IS NULL` — the
|
||||
half-state from a pre-P0-7 crash where `_finalize_claim` flushed
|
||||
status=CLAIMED before branch creation failed.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING, Any
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
import pytest_asyncio
|
||||
from roboco.db.tables import AgentTable, ProjectTable, TaskTable
|
||||
from roboco.models.base import (
|
||||
AgentRole,
|
||||
AgentStatus,
|
||||
TaskNature,
|
||||
TaskStatus,
|
||||
TaskType,
|
||||
Team,
|
||||
)
|
||||
from roboco.runtime.orchestrator import AgentOrchestrator
|
||||
from roboco.services.task import TaskService
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import AsyncIterator
|
||||
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def orphan_setup(
|
||||
db_session: AsyncSession,
|
||||
) -> AsyncIterator[dict[str, Any]]:
|
||||
system_agent = AgentTable(
|
||||
id=uuid4(),
|
||||
name="System",
|
||||
slug=f"system-{uuid4().hex[:8]}",
|
||||
role=AgentRole.SYSTEM,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="system",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(system_agent)
|
||||
await db_session.flush()
|
||||
|
||||
project = ProjectTable(
|
||||
id=uuid4(),
|
||||
name="Reconciler Test",
|
||||
slug=f"recon-{uuid4().hex[:8]}",
|
||||
git_url="https://github.com/example/recon.git",
|
||||
default_branch="main",
|
||||
protected_branches=["main"],
|
||||
assigned_cell=Team.BACKEND,
|
||||
created_by=system_agent.id,
|
||||
is_active=True,
|
||||
)
|
||||
db_session.add(project)
|
||||
await db_session.flush()
|
||||
|
||||
dev = AgentTable(
|
||||
id=uuid4(),
|
||||
name="BE Dev",
|
||||
slug=f"be-dev-{uuid4().hex[:8]}",
|
||||
role=AgentRole.DEVELOPER,
|
||||
team=Team.BACKEND,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="dev",
|
||||
capabilities=["python"],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(dev)
|
||||
await db_session.flush()
|
||||
|
||||
# ORPHAN: status=CLAIMED, assigned_to=dev, branch_name=NULL.
|
||||
orphan = TaskTable(
|
||||
id=uuid4(),
|
||||
title="Orphan from prior crash",
|
||||
description="…",
|
||||
status=TaskStatus.CLAIMED,
|
||||
priority=2,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
team=Team.BACKEND,
|
||||
project_id=project.id,
|
||||
created_by=system_agent.id,
|
||||
assigned_to=dev.id,
|
||||
claimed_by=dev.id,
|
||||
acceptance_criteria=["…"],
|
||||
)
|
||||
# HEALTHY: status=CLAIMED with branch — must NOT be rolled back.
|
||||
healthy = TaskTable(
|
||||
id=uuid4(),
|
||||
title="Healthy claim",
|
||||
description="…",
|
||||
status=TaskStatus.CLAIMED,
|
||||
priority=2,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
team=Team.BACKEND,
|
||||
project_id=project.id,
|
||||
created_by=system_agent.id,
|
||||
assigned_to=dev.id,
|
||||
claimed_by=dev.id,
|
||||
branch_name="feature/backend/healthy",
|
||||
acceptance_criteria=["…"],
|
||||
)
|
||||
db_session.add_all([orphan, healthy])
|
||||
await db_session.flush()
|
||||
yield {"orphan": orphan, "healthy": healthy, "dev": dev}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reconciler_rolls_back_orphan_claims(
|
||||
db_session: AsyncSession, orphan_setup: dict[str, Any]
|
||||
) -> None:
|
||||
"""CLAIMED task with branch_name=NULL → reconciled to PENDING."""
|
||||
orphan = orphan_setup["orphan"]
|
||||
healthy = orphan_setup["healthy"]
|
||||
svc = TaskService(db_session)
|
||||
orch = AgentOrchestrator.__new__(AgentOrchestrator)
|
||||
|
||||
# Drive the logic directly via the test-injectable helper so we avoid
|
||||
# the orchestrator's session-factory dance.
|
||||
await orch._reconcile_with_service(svc)
|
||||
|
||||
refreshed_orphan = await svc.get(orphan.id)
|
||||
refreshed_healthy = await svc.get(healthy.id)
|
||||
|
||||
assert refreshed_orphan is not None
|
||||
assert str(refreshed_orphan.status) == "pending", (
|
||||
"P2-8: orphan must be rolled back to pending"
|
||||
)
|
||||
assert refreshed_orphan.assigned_to is None
|
||||
assert refreshed_orphan.active_claimant_id is None
|
||||
|
||||
# Healthy claim untouched.
|
||||
assert refreshed_healthy is not None
|
||||
assert str(refreshed_healthy.status) == "claimed"
|
||||
assert refreshed_healthy.branch_name == "feature/backend/healthy"
|
||||
@@ -0,0 +1,261 @@
|
||||
"""ProjectService coverage — register/list/update/delete + token round-trip.
|
||||
|
||||
Driven by the real Postgres ``db_session`` fixture so the test exercises
|
||||
the same SQLAlchemy paths the production code does.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
import pytest_asyncio
|
||||
from roboco.db.tables import AgentTable
|
||||
from roboco.models import AgentRole, AgentStatus, Team
|
||||
from roboco.models.project import ProjectCreate, ProjectUpdate
|
||||
from roboco.services.base import ConflictError, NotFoundError
|
||||
from roboco.services.project import ProjectService
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import AsyncIterator
|
||||
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def project_setup(
|
||||
db_session: AsyncSession,
|
||||
) -> AsyncIterator[dict]:
|
||||
"""Seed a system agent so created_by FK is satisfied."""
|
||||
system = AgentTable(
|
||||
id=uuid4(),
|
||||
name="System",
|
||||
slug=f"system-{uuid4().hex[:8]}",
|
||||
role=AgentRole.SYSTEM,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="system",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(system)
|
||||
await db_session.flush()
|
||||
svc = ProjectService(db_session)
|
||||
yield {"svc": svc, "creator_id": system.id}
|
||||
|
||||
|
||||
def _project_payload(slug_suffix: str) -> ProjectCreate:
|
||||
return ProjectCreate(
|
||||
name=f"Project {slug_suffix}",
|
||||
slug=f"proj-{slug_suffix}",
|
||||
git_url=f"https://github.com/example/{slug_suffix}.git",
|
||||
assigned_cell=Team.BACKEND,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_project(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
assert project.id is not None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_project_with_git_token_encrypts(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
payload = _project_payload(uuid4().hex[:6])
|
||||
payload_dict = payload.model_dump()
|
||||
payload_dict["git_token"] = "ghp_test_token"
|
||||
project = await svc.create(
|
||||
ProjectCreate(**payload_dict), project_setup["creator_id"]
|
||||
)
|
||||
assert project.git_token_encrypted is not None
|
||||
assert project.git_token_encrypted != "ghp_test_token"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_project_duplicate_slug_raises(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
payload = _project_payload(uuid4().hex[:6])
|
||||
await svc.create(payload, project_setup["creator_id"])
|
||||
with pytest.raises(ConflictError):
|
||||
await svc.create(payload, project_setup["creator_id"])
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_returns_project(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
fetched = await svc.get(project.id)
|
||||
assert fetched is not None
|
||||
assert fetched.id == project.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_returns_none_for_missing(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
assert await svc.get(uuid4()) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_by_slug(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
payload = _project_payload(uuid4().hex[:6])
|
||||
created = await svc.create(payload, project_setup["creator_id"])
|
||||
fetched = await svc.get_by_slug(payload.slug)
|
||||
assert fetched is not None
|
||||
assert fetched.id == created.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_or_raise_raises(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
with pytest.raises(NotFoundError):
|
||||
await svc.get_or_raise(uuid4())
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_changes_name(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
new_name = f"renamed-{uuid4().hex[:6]}"
|
||||
updated = await svc.update(project.id, ProjectUpdate(name=new_name))
|
||||
assert updated is not None
|
||||
assert updated.name == new_name
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_clear_git_token(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
payload = _project_payload(uuid4().hex[:6])
|
||||
pd = payload.model_dump()
|
||||
pd["git_token"] = "ghp_initial"
|
||||
project = await svc.create(ProjectCreate(**pd), project_setup["creator_id"])
|
||||
assert project.git_token_encrypted is not None
|
||||
updated = await svc.update(project.id, ProjectUpdate(git_token=""))
|
||||
assert updated is not None
|
||||
assert updated.git_token_encrypted is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_set_git_token(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
updated = await svc.update(project.id, ProjectUpdate(git_token="ghp_new"))
|
||||
assert updated is not None
|
||||
assert updated.git_token_encrypted is not None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_returns_none_for_missing(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
assert (await svc.update(uuid4(), ProjectUpdate(name="ghost"))) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_project(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
await svc.delete(project.id)
|
||||
assert await svc.get(project.id) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_all(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
a = await svc.create(_project_payload(uuid4().hex[:6]), project_setup["creator_id"])
|
||||
b = await svc.create(_project_payload(uuid4().hex[:6]), project_setup["creator_id"])
|
||||
rows = await svc.list_all()
|
||||
ids = {p.id for p in rows}
|
||||
assert a.id in ids
|
||||
assert b.id in ids
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_by_cell(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
rows = await svc.list_by_cell(Team.BACKEND)
|
||||
assert project.id in {p.id for p in rows}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_workspace_path(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
updated = await svc.set_workspace_path(project.id, "/tmp/test-ws")
|
||||
assert updated is not None
|
||||
assert updated.workspace_path == "/tmp/test-ws"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_decrypted_token_round_trip(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
payload = _project_payload(uuid4().hex[:6])
|
||||
pd = payload.model_dump()
|
||||
pd["git_token"] = "ghp_secret"
|
||||
project = await svc.create(ProjectCreate(**pd), project_setup["creator_id"])
|
||||
decrypted = await svc.get_decrypted_token(project.id)
|
||||
assert decrypted == "ghp_secret"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_decrypted_token_returns_none_when_unset(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
assert await svc.get_decrypted_token(project.id) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_decrypted_token_by_slug_round_trip(
|
||||
project_setup: dict,
|
||||
) -> None:
|
||||
svc = project_setup["svc"]
|
||||
payload = _project_payload(uuid4().hex[:6])
|
||||
pd = payload.model_dump()
|
||||
pd["git_token"] = "ghp_slug_secret"
|
||||
await svc.create(ProjectCreate(**pd), project_setup["creator_id"])
|
||||
decrypted = await svc.get_decrypted_token_by_slug(payload.slug)
|
||||
assert decrypted == "ghp_slug_secret"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_agent_access_returns_bool(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
has_access = await svc.check_agent_access(project.id, uuid4(), Team.BACKEND)
|
||||
assert isinstance(has_access, bool)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_and_remove_allowed_agent(project_setup: dict) -> None:
|
||||
svc = project_setup["svc"]
|
||||
project = await svc.create(
|
||||
_project_payload(uuid4().hex[:6]), project_setup["creator_id"]
|
||||
)
|
||||
new_agent_id = uuid4()
|
||||
added = await svc.add_allowed_agent(project.id, new_agent_id)
|
||||
assert added is not None
|
||||
removed = await svc.remove_allowed_agent(project.id, new_agent_id)
|
||||
assert removed is not None
|
||||
@@ -0,0 +1,303 @@
|
||||
"""ProviderService coverage — list/get/create/update/delete/decrypt.
|
||||
|
||||
Drives a real `db_session` via the project's Postgres-backed conftest.
|
||||
Provider rows are encrypted at rest with Fernet; tests round-trip
|
||||
plaintext → ciphertext → plaintext through `get_decrypted_token` and
|
||||
exercise the tri-state semantics of ``ProviderUpdate.auth_token``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
import pytest_asyncio
|
||||
from roboco.db.tables import ModelAssignmentTable
|
||||
from roboco.models.base import ModelProvider
|
||||
from roboco.services.base import ConflictError, NotFoundError
|
||||
from roboco.services.provider import (
|
||||
ProviderCreate,
|
||||
ProviderService,
|
||||
ProviderUpdate,
|
||||
)
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import AsyncIterator
|
||||
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def provider_svc(db_session: AsyncSession) -> AsyncIterator[ProviderService]:
|
||||
yield ProviderService(db_session)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_provider_with_token_encrypts(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"anthropic-{uuid4().hex[:6]}",
|
||||
type=ModelProvider.ANTHROPIC,
|
||||
auth_token="sk-test-secret",
|
||||
)
|
||||
)
|
||||
assert row.auth_token_encrypted is not None
|
||||
assert row.auth_token_encrypted != "sk-test-secret"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_provider_without_token_leaves_null(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"p-{uuid4().hex[:6]}", type=ModelProvider.LOCAL)
|
||||
)
|
||||
assert row.auth_token_encrypted is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_provider_duplicate_name_raises(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
name = f"dup-{uuid4().hex[:6]}"
|
||||
await provider_svc.create_provider(
|
||||
ProviderCreate(name=name, type=ModelProvider.ANTHROPIC)
|
||||
)
|
||||
with pytest.raises(ConflictError):
|
||||
await provider_svc.create_provider(
|
||||
ProviderCreate(name=name, type=ModelProvider.OPENAI)
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_provider_returns_row(provider_svc: ProviderService) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"g-{uuid4().hex[:6]}", type=ModelProvider.OPENAI)
|
||||
)
|
||||
fetched = await provider_svc.get_provider(row.id)
|
||||
assert fetched is not None
|
||||
assert fetched.id == row.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_provider_returns_none_when_missing(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
assert await provider_svc.get_provider(uuid4()) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_provider_or_raise_raises(provider_svc: ProviderService) -> None:
|
||||
with pytest.raises(NotFoundError):
|
||||
await provider_svc.get_provider_or_raise(uuid4())
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_by_name(provider_svc: ProviderService) -> None:
|
||||
name = f"by-name-{uuid4().hex[:6]}"
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=name, type=ModelProvider.ANTHROPIC)
|
||||
)
|
||||
found = await provider_svc.get_by_name(name)
|
||||
assert found is not None
|
||||
assert found.id == row.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_providers_excludes_disabled_by_default(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
enabled = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"on-{uuid4().hex[:6]}", type=ModelProvider.ANTHROPIC, enabled=True
|
||||
)
|
||||
)
|
||||
disabled = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"off-{uuid4().hex[:6]}", type=ModelProvider.LOCAL, enabled=False
|
||||
)
|
||||
)
|
||||
visible = await provider_svc.list_providers()
|
||||
visible_ids = {p.id for p in visible}
|
||||
assert enabled.id in visible_ids
|
||||
assert disabled.id not in visible_ids
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_providers_include_disabled(provider_svc: ProviderService) -> None:
|
||||
disabled = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"x-{uuid4().hex[:6]}", type=ModelProvider.LOCAL, enabled=False
|
||||
)
|
||||
)
|
||||
every = await provider_svc.list_providers(include_disabled=True)
|
||||
assert disabled.id in {p.id for p in every}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_provider_changes_name(provider_svc: ProviderService) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"old-{uuid4().hex[:6]}", type=ModelProvider.OPENAI)
|
||||
)
|
||||
new_name = f"new-{uuid4().hex[:6]}"
|
||||
updated = await provider_svc.update_provider(row.id, ProviderUpdate(name=new_name))
|
||||
assert updated is not None
|
||||
assert updated.name == new_name
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_provider_duplicate_name_raises(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
a = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"a-{uuid4().hex[:6]}", type=ModelProvider.OPENAI)
|
||||
)
|
||||
b = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"b-{uuid4().hex[:6]}", type=ModelProvider.OPENAI)
|
||||
)
|
||||
with pytest.raises(ConflictError):
|
||||
await provider_svc.update_provider(b.id, ProviderUpdate(name=a.name))
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_provider_clears_base_url_with_empty_string(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"url-{uuid4().hex[:6]}",
|
||||
type=ModelProvider.OLLAMA_CLOUD,
|
||||
base_url="https://example.com",
|
||||
)
|
||||
)
|
||||
updated = await provider_svc.update_provider(row.id, ProviderUpdate(base_url=""))
|
||||
assert updated is not None
|
||||
assert updated.base_url is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_provider_token_tristate_clear(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"t-{uuid4().hex[:6]}",
|
||||
type=ModelProvider.OLLAMA_CLOUD,
|
||||
auth_token="initial",
|
||||
)
|
||||
)
|
||||
updated = await provider_svc.update_provider(
|
||||
row.id, ProviderUpdate(clear_auth_token=True)
|
||||
)
|
||||
assert updated is not None
|
||||
assert updated.auth_token_encrypted is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_provider_token_tristate_set(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"s-{uuid4().hex[:6]}", type=ModelProvider.ANTHROPIC)
|
||||
)
|
||||
updated = await provider_svc.update_provider(
|
||||
row.id, ProviderUpdate(auth_token="new-secret")
|
||||
)
|
||||
assert updated is not None
|
||||
assert updated.auth_token_encrypted is not None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_provider_token_tristate_unchanged(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"u-{uuid4().hex[:6]}",
|
||||
type=ModelProvider.ANTHROPIC,
|
||||
auth_token="initial",
|
||||
)
|
||||
)
|
||||
original_token = row.auth_token_encrypted
|
||||
updated = await provider_svc.update_provider(
|
||||
row.id,
|
||||
ProviderUpdate(enabled=False), # no auth_token field
|
||||
)
|
||||
assert updated is not None
|
||||
assert updated.auth_token_encrypted == original_token # unchanged
|
||||
assert updated.enabled is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_provider_returns_none_for_missing(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
assert (
|
||||
await provider_svc.update_provider(uuid4(), ProviderUpdate(enabled=False))
|
||||
is None
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_provider(provider_svc: ProviderService) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"d-{uuid4().hex[:6]}", type=ModelProvider.OPENAI)
|
||||
)
|
||||
await provider_svc.delete_provider(row.id)
|
||||
assert await provider_svc.get_provider(row.id) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_provider_raises_when_referenced(
|
||||
db_session: AsyncSession, provider_svc: ProviderService
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"r-{uuid4().hex[:6]}", type=ModelProvider.OPENAI)
|
||||
)
|
||||
# Insert a model assignment that references this provider.
|
||||
assignment = ModelAssignmentTable(
|
||||
id=uuid4(),
|
||||
scope="role",
|
||||
scope_value="developer",
|
||||
provider_config_id=row.id,
|
||||
model_name="claude-haiku-4-5",
|
||||
)
|
||||
db_session.add(assignment)
|
||||
await db_session.flush()
|
||||
|
||||
with pytest.raises(ConflictError):
|
||||
await provider_svc.delete_provider(row.id)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_decrypted_token_round_trip(provider_svc: ProviderService) -> None:
|
||||
plaintext = "sk-roundtrip-secret"
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(
|
||||
name=f"rt-{uuid4().hex[:6]}",
|
||||
type=ModelProvider.ANTHROPIC,
|
||||
auth_token=plaintext,
|
||||
)
|
||||
)
|
||||
decrypted = await provider_svc.get_decrypted_token(row.id)
|
||||
assert decrypted == plaintext
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_decrypted_token_returns_none_when_unset(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
row = await provider_svc.create_provider(
|
||||
ProviderCreate(name=f"nt-{uuid4().hex[:6]}", type=ModelProvider.LOCAL)
|
||||
)
|
||||
assert await provider_svc.get_decrypted_token(row.id) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_decrypted_token_returns_none_for_missing_provider(
|
||||
provider_svc: ProviderService,
|
||||
) -> None:
|
||||
assert await provider_svc.get_decrypted_token(uuid4()) is None
|
||||
@@ -0,0 +1,88 @@
|
||||
"""P2-9: the prompt composer injects the autogen verb table.
|
||||
|
||||
`compose_prompt` reads `agents/prompts/_generated/<role>.md` and
|
||||
includes it as a composition layer (between role and team). This pins
|
||||
that contract: when the file exists, its content appears in the
|
||||
composed prompt.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
import pytest
|
||||
from roboco.agents.factories._base import compose_prompt
|
||||
from roboco.models.base import AgentRole, Team
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
def _write_layer(root: Path, rel: str, body: str) -> None:
|
||||
target = root / rel
|
||||
target.parent.mkdir(parents=True, exist_ok=True)
|
||||
target.write_text(body)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def fake_prompts(tmp_path: Path) -> Path:
|
||||
"""Build a fake prompts directory with a known _generated/<role>.md."""
|
||||
_write_layer(tmp_path, "base.md", "# BASE LAYER")
|
||||
_write_layer(tmp_path, "roles/developer.md", "# ROLE LAYER (developer)")
|
||||
_write_layer(tmp_path, "teams/backend.md", "# TEAM LAYER (backend)")
|
||||
_write_layer(
|
||||
tmp_path,
|
||||
"_generated/developer.md",
|
||||
"# AUTOGEN LAYER (developer)\n\n"
|
||||
"## Verbs available to you (autogenerated source of truth)\n\n"
|
||||
"| Verb | Body schema |\n|------|-------------|\n"
|
||||
"| `give_me_work` | `give_me_work()` |\n",
|
||||
)
|
||||
_write_layer(tmp_path, "identities/be-dev-1.md", "# IDENTITY (be-dev-1)")
|
||||
return tmp_path
|
||||
|
||||
|
||||
def test_compose_prompt_includes_autogen_layer(fake_prompts: Path) -> None:
|
||||
composed = compose_prompt(
|
||||
role=AgentRole.DEVELOPER,
|
||||
team=Team.BACKEND,
|
||||
agent_slug="be-dev-1",
|
||||
base_path=fake_prompts,
|
||||
)
|
||||
assert "AUTOGEN LAYER (developer)" in composed
|
||||
assert "give_me_work()" in composed
|
||||
|
||||
|
||||
def test_compose_prompt_orders_layers_correctly(fake_prompts: Path) -> None:
|
||||
"""base → role → autogen → team → identity (composer's documented order)."""
|
||||
composed = compose_prompt(
|
||||
role=AgentRole.DEVELOPER,
|
||||
team=Team.BACKEND,
|
||||
agent_slug="be-dev-1",
|
||||
base_path=fake_prompts,
|
||||
)
|
||||
base_idx = composed.index("BASE LAYER")
|
||||
role_idx = composed.index("ROLE LAYER (developer)")
|
||||
autogen_idx = composed.index("AUTOGEN LAYER (developer)")
|
||||
team_idx = composed.index("TEAM LAYER (backend)")
|
||||
identity_idx = composed.index("IDENTITY (be-dev-1)")
|
||||
assert base_idx < role_idx < autogen_idx < team_idx < identity_idx
|
||||
|
||||
|
||||
def test_compose_prompt_omits_autogen_when_file_missing(tmp_path: Path) -> None:
|
||||
"""No _generated/<role>.md → composer skips that layer cleanly."""
|
||||
_write_layer(tmp_path, "base.md", "# BASE")
|
||||
_write_layer(tmp_path, "roles/developer.md", "# DEV ROLE")
|
||||
_write_layer(tmp_path, "identities/be-dev-1.md", "# IDENTITY")
|
||||
|
||||
composed = compose_prompt(
|
||||
role=AgentRole.DEVELOPER,
|
||||
team=None,
|
||||
agent_slug="be-dev-1",
|
||||
base_path=tmp_path,
|
||||
)
|
||||
|
||||
assert "BASE" in composed
|
||||
assert "DEV ROLE" in composed
|
||||
assert "IDENTITY" in composed
|
||||
assert "AUTOGEN" not in composed # no autogen layer file
|
||||
@@ -0,0 +1,111 @@
|
||||
"""P2-7: every gateway.rejected audit row carries an attempt_id.
|
||||
|
||||
The attempt_id (uuid4 per rejection) lets post-mortem queries group
|
||||
all attempts on a task within a window, even when multiple calls share
|
||||
a correlation_id from a single inbound request.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock
|
||||
from uuid import UUID, uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
|
||||
|
||||
|
||||
def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
base: dict[str, Any] = {
|
||||
"task": AsyncMock(),
|
||||
"work_session": AsyncMock(),
|
||||
"git": AsyncMock(),
|
||||
"a2a": AsyncMock(),
|
||||
"journal": AsyncMock(),
|
||||
"audit": AsyncMock(),
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
base.update(overrides)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
"list_unread_mentions",
|
||||
"list_pending_notifications",
|
||||
"task_metadata_gaps",
|
||||
"recent_team_activity",
|
||||
"blockers_in_lane",
|
||||
"journal_highlights_for_task",
|
||||
):
|
||||
getattr(repo, method).return_value = []
|
||||
return ChoreographerDeps(**base)
|
||||
|
||||
|
||||
def _is_uuid(s: str) -> bool:
|
||||
try:
|
||||
UUID(s)
|
||||
except (ValueError, TypeError):
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rejection_includes_attempt_id() -> None:
|
||||
"""A `not_found` rejection on i_am_done emits an audit row with attempt_id."""
|
||||
aid = uuid4()
|
||||
tid = uuid4()
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = None # → not_found
|
||||
audit_svc = AsyncMock()
|
||||
deps = _make_deps(task=task_svc, audit=audit_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.i_am_done(aid, tid, notes="x")
|
||||
|
||||
assert env.error == "not_found"
|
||||
audit_svc.log_event.assert_awaited()
|
||||
args = audit_svc.log_event.await_args
|
||||
details = args.kwargs["details"]
|
||||
assert "attempt_id" in details, "P2-7: audit row must include attempt_id"
|
||||
assert _is_uuid(details["attempt_id"]), "P2-7: attempt_id must be a UUID string"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_distinct_rejections_emit_distinct_attempt_ids() -> None:
|
||||
"""Two rejections in sequence get different attempt_ids."""
|
||||
aid = uuid4()
|
||||
tid = uuid4()
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = None
|
||||
audit_svc = AsyncMock()
|
||||
deps = _make_deps(task=task_svc, audit=audit_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
await c.i_am_done(aid, tid, notes="x")
|
||||
await c.i_am_done(aid, tid, notes="x")
|
||||
|
||||
expected_distinct_ids = 2
|
||||
calls = audit_svc.log_event.await_args_list
|
||||
ids = {call.kwargs["details"]["attempt_id"] for call in calls}
|
||||
assert len(ids) == expected_distinct_ids, (
|
||||
"P2-7: each rejection emits its own attempt_id"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_success_envelope_does_not_emit_audit() -> None:
|
||||
"""Confirms the contract: audit rows fire on rejection only.
|
||||
|
||||
attempt_id machinery doesn't trip on success (no row to stamp).
|
||||
"""
|
||||
aid = uuid4()
|
||||
task_svc = AsyncMock()
|
||||
task_svc.list_assigned_for_agent.return_value = []
|
||||
task_svc.list_paused_for_agent.return_value = []
|
||||
audit_svc = AsyncMock()
|
||||
deps = _make_deps(task=task_svc, audit=audit_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.give_me_work(aid)
|
||||
|
||||
assert env.error is None
|
||||
audit_svc.log_event.assert_not_awaited()
|
||||
@@ -0,0 +1,149 @@
|
||||
"""P0-6 / D-13: MCP _post() surfaces envelope body on 4xx.
|
||||
|
||||
The pre-fix path called ``response.raise_for_status()`` then ``.json()``,
|
||||
which discarded the body on any 4xx — agents saw a Python
|
||||
``httpx.HTTPStatusError`` traceback instead of the orchestrator's
|
||||
``{error, message, remediate, missing}`` envelope. These tests pin the
|
||||
fixed contract: 2xx and 4xx both return the parsed JSON; only an
|
||||
unparseable body produces a synthetic ``transport_error`` envelope.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
import json
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING, Any
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
if TYPE_CHECKING:
|
||||
import types
|
||||
|
||||
|
||||
_MANIFEST = {
|
||||
"agent_id": "00000000-0000-0000-0000-000000000001",
|
||||
"role": "developer",
|
||||
"team": "backend",
|
||||
"workspace_path": "/tmp/test",
|
||||
"flow_tools": ["give_me_work", "i_will_work_on", "i_am_done"],
|
||||
"do_tools": ["commit", "note"],
|
||||
"read_tools": [],
|
||||
"write_tools": [],
|
||||
"bash_allowed": True,
|
||||
"subagent_allowed": False,
|
||||
"subagent_model": None,
|
||||
"env": {},
|
||||
}
|
||||
|
||||
|
||||
def _seed_env(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
manifest_path = Path(tempfile.mkdtemp()) / "tool-manifest.json"
|
||||
manifest_path.write_text(json.dumps(_MANIFEST))
|
||||
monkeypatch.setenv("ROBOCO_AGENT_ID", "00000000-0000-0000-0000-000000000001")
|
||||
monkeypatch.setenv("ROBOCO_AGENT_ROLE", "developer")
|
||||
monkeypatch.setenv("ROBOCO_ORCHESTRATOR_URL", "http://test-orchestrator:8000")
|
||||
monkeypatch.setenv("ROBOCO_TOOL_MANIFEST_PATH", str(manifest_path))
|
||||
|
||||
|
||||
def _fake_client_with(status: int, body: Any) -> MagicMock:
|
||||
"""httpx.Client context-manager whose post() returns the given response."""
|
||||
fake_response = MagicMock()
|
||||
fake_response.status_code = status
|
||||
if isinstance(body, dict):
|
||||
fake_response.json.return_value = body
|
||||
else:
|
||||
fake_response.json.side_effect = ValueError("not json")
|
||||
fake_client = MagicMock()
|
||||
fake_client.__enter__ = MagicMock(return_value=fake_client)
|
||||
fake_client.__exit__ = MagicMock(return_value=False)
|
||||
fake_client.post.return_value = fake_response
|
||||
return fake_client
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def flow_module(monkeypatch: pytest.MonkeyPatch) -> types.ModuleType:
|
||||
_seed_env(monkeypatch)
|
||||
import roboco.mcp.flow_server as srv
|
||||
|
||||
importlib.reload(srv)
|
||||
return srv
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def do_module(monkeypatch: pytest.MonkeyPatch) -> types.ModuleType:
|
||||
_seed_env(monkeypatch)
|
||||
import roboco.mcp.do_server as srv
|
||||
|
||||
importlib.reload(srv)
|
||||
return srv
|
||||
|
||||
|
||||
def test_flow_post_returns_envelope_on_422(flow_module: types.ModuleType) -> None:
|
||||
"""422 with envelope body must surface as the envelope, not raise."""
|
||||
envelope_body = {
|
||||
"error": "tracing_gap",
|
||||
"message": "missing plan",
|
||||
"remediate": "call i_will_work_on(task_id=..., plan='...')",
|
||||
"missing": ["plan"],
|
||||
}
|
||||
client = _fake_client_with(422, envelope_body)
|
||||
with patch("httpx.Client", return_value=client):
|
||||
result = flow_module.give_me_work()
|
||||
assert result == envelope_body
|
||||
|
||||
|
||||
def test_flow_post_returns_envelope_on_400(flow_module: types.ModuleType) -> None:
|
||||
"""400 with envelope body (e.g. role-gate rejection from do_server)."""
|
||||
body = {
|
||||
"error": "not_authorized",
|
||||
"message": "role 'cell_pm' may not commit code",
|
||||
"remediate": "PMs delegate; use delegate(...)",
|
||||
"missing": [],
|
||||
}
|
||||
client = _fake_client_with(400, body)
|
||||
with patch("httpx.Client", return_value=client):
|
||||
result = flow_module.i_will_work_on("task-id", plan="x")
|
||||
assert result["error"] == "not_authorized"
|
||||
|
||||
|
||||
def test_flow_post_returns_envelope_on_404(flow_module: types.ModuleType) -> None:
|
||||
"""Even 404 must surface body; only the body's content matters to the agent."""
|
||||
body = {
|
||||
"error": "not_found",
|
||||
"message": "task abc not found",
|
||||
"remediate": "call give_me_work() to find an actionable task",
|
||||
"missing": [],
|
||||
}
|
||||
client = _fake_client_with(404, body)
|
||||
with patch("httpx.Client", return_value=client):
|
||||
result = flow_module.i_am_done("task-abc", notes="done")
|
||||
assert result["error"] == "not_found"
|
||||
|
||||
|
||||
def test_flow_post_synthesizes_transport_error_when_body_unparseable(
|
||||
flow_module: types.ModuleType,
|
||||
) -> None:
|
||||
"""No JSON body → synthetic transport_error envelope (NOT a raise)."""
|
||||
client = _fake_client_with(502, body=None) # body=None → ValueError on .json()
|
||||
with patch("httpx.Client", return_value=client):
|
||||
result = flow_module.give_me_work()
|
||||
assert result["error"] == "transport_error"
|
||||
assert "502" in result["message"]
|
||||
assert "remediate" in result
|
||||
|
||||
|
||||
def test_do_post_returns_envelope_on_400(do_module: types.ModuleType) -> None:
|
||||
"""do_server mirrors flow_server: envelope surfaces on rejection."""
|
||||
body = {
|
||||
"error": "not_authorized",
|
||||
"message": "role 'cell_pm' may not commit",
|
||||
"remediate": "PMs delegate via delegate()",
|
||||
"missing": [],
|
||||
}
|
||||
client = _fake_client_with(400, body)
|
||||
with patch("httpx.Client", return_value=client):
|
||||
result = do_module.commit("any message")
|
||||
assert result["error"] == "not_authorized"
|
||||
@@ -0,0 +1,252 @@
|
||||
"""PermissionService coverage — RBAC for channels, 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
|
||||
``object.__new__`` to bypass session-management.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.models import AgentRole, Team
|
||||
from roboco.models.permissions import (
|
||||
AgentContext,
|
||||
KBAction,
|
||||
PermissionLevel,
|
||||
TaskAction,
|
||||
)
|
||||
from roboco.services.permissions import PermissionService
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def svc() -> PermissionService:
|
||||
"""PermissionService is a SingletonService — bypass __init__ for unit tests."""
|
||||
return object.__new__(PermissionService)
|
||||
|
||||
|
||||
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_can_write_any_channel(svc: PermissionService) -> None:
|
||||
"""Auditor write returns True (cover-maintenance is a convention)."""
|
||||
auditor = _ctx(AgentRole.AUDITOR)
|
||||
assert 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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_main_pm_can_send_notifications(svc: PermissionService) -> None:
|
||||
main_pm = _ctx(AgentRole.MAIN_PM)
|
||||
assert svc.can_send_notifications(main_pm) is True
|
||||
|
||||
|
||||
def test_developer_cannot_send_notifications(svc: PermissionService) -> None:
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert svc.can_send_notifications(dev) is False
|
||||
|
||||
|
||||
def test_auditor_send_notifications_returns_bool(svc: PermissionService) -> None:
|
||||
"""Auditor's notification permission is read from agents_config."""
|
||||
auditor = _ctx(AgentRole.AUDITOR)
|
||||
assert isinstance(svc.can_send_notifications(auditor), bool)
|
||||
|
||||
|
||||
def test_can_notify_pm_to_dev(svc: PermissionService) -> None:
|
||||
sender = _ctx(AgentRole.MAIN_PM)
|
||||
recipient = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert svc.can_notify(sender, recipient) is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Communication matrix
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_can_communicate_within_cell(svc: PermissionService) -> None:
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
qa = _ctx(AgentRole.QA, team=Team.BACKEND)
|
||||
assert svc.can_communicate(dev, qa) is True
|
||||
|
||||
|
||||
def test_can_communicate_across_cells_via_pm(svc: PermissionService) -> None:
|
||||
"""Communication matrix returns a bool — exact result depends on the matrix."""
|
||||
main_pm = _ctx(AgentRole.MAIN_PM)
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert isinstance(svc.can_communicate(main_pm, dev), bool)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Task action permissions
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_developer_can_claim_in_own_team(svc: PermissionService) -> None:
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert svc.can_perform_task_action(dev, TaskAction.CLAIM, Team.BACKEND) is True
|
||||
|
||||
|
||||
def test_qa_can_view_all(svc: PermissionService) -> None:
|
||||
qa = _ctx(AgentRole.QA, team=Team.BACKEND)
|
||||
# QA must be able to view tasks in their cell.
|
||||
assert isinstance(
|
||||
svc.can_perform_task_action(qa, TaskAction.VIEW_OWN, Team.BACKEND),
|
||||
bool,
|
||||
)
|
||||
|
||||
|
||||
def test_can_perform_task_action_returns_bool(svc: PermissionService) -> None:
|
||||
"""Action permission returns a bool — exact value depends on TASK_PERMISSIONS."""
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert isinstance(
|
||||
svc.can_perform_task_action(dev, TaskAction.CLOSE, Team.BACKEND), bool
|
||||
)
|
||||
|
||||
|
||||
def test_cell_pm_can_close_in_own_cell(svc: PermissionService) -> None:
|
||||
cell_pm = _ctx(AgentRole.CELL_PM, team=Team.BACKEND)
|
||||
assert svc.can_perform_task_action(cell_pm, TaskAction.CLOSE, Team.BACKEND) is True
|
||||
|
||||
|
||||
def test_get_task_actions_returns_set(svc: PermissionService) -> None:
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
actions = svc.get_task_actions(dev)
|
||||
assert hasattr(actions, "__iter__")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Permission levels
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_ceo_has_highest_level(svc: PermissionService) -> None:
|
||||
assert svc.get_permission_level(AgentRole.CEO) == PermissionLevel.CEO
|
||||
|
||||
|
||||
def test_developer_is_cell_member_level(svc: PermissionService) -> None:
|
||||
assert svc.get_permission_level(AgentRole.DEVELOPER) == PermissionLevel.CELL_MEMBER
|
||||
|
||||
|
||||
def test_main_pm_is_main_pm_level(svc: PermissionService) -> None:
|
||||
assert svc.get_permission_level(AgentRole.MAIN_PM) == PermissionLevel.MAIN_PM
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Combined check_all
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_check_all_returns_dict(svc: PermissionService) -> None:
|
||||
"""check_all returns a permission summary dict."""
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
result = svc.check_all(dev)
|
||||
assert isinstance(result, dict)
|
||||
assert "role" in result
|
||||
assert "level" in result
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Slug-based shortcuts
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_can_agent_read_channel_known_slug(svc: PermissionService) -> None:
|
||||
"""Pass a known agent slug; service should resolve role+team and decide."""
|
||||
# be-dev-1 is in AGENT_ROLE_MAP as a developer in backend.
|
||||
result = svc.can_agent_read_channel("be-dev-1", "backend-cell")
|
||||
assert isinstance(result, bool)
|
||||
|
||||
|
||||
def test_can_agent_read_channel_unknown_slug(svc: PermissionService) -> None:
|
||||
"""Unknown slug → False (deny by default)."""
|
||||
assert svc.can_agent_read_channel("ghost-agent", "backend-cell") is False
|
||||
|
||||
|
||||
def test_can_agent_send_notifications_known_slug(svc: PermissionService) -> None:
|
||||
"""main-pm slug should be able to send."""
|
||||
assert svc.can_agent_send_notifications("main-pm") is True
|
||||
|
||||
|
||||
def test_can_agent_send_notifications_unknown_slug(svc: PermissionService) -> None:
|
||||
"""Unknown slug → False."""
|
||||
assert svc.can_agent_send_notifications("ghost-agent") is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# KB permissions
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_get_kb_actions_returns_collection(svc: PermissionService) -> None:
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
actions = svc.get_kb_actions(dev)
|
||||
# Returns a collection of allowed KB actions.
|
||||
assert hasattr(actions, "__iter__")
|
||||
|
||||
|
||||
def test_can_perform_kb_action_developer(svc: PermissionService) -> None:
|
||||
"""KB SEARCH is generally allowed for developers."""
|
||||
dev = _ctx(AgentRole.DEVELOPER, team=Team.BACKEND)
|
||||
assert isinstance(svc.can_perform_kb_action(dev, KBAction.SEARCH), bool)
|
||||
@@ -19,15 +19,6 @@ resolution-markers = [
|
||||
"python_full_version < '3.11' and sys_platform == 'darwin'",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "aiofiles"
|
||||
version = "25.1.0"
|
||||
source = { registry = "https://pypi.org/simple" }
|
||||
sdist = { url = "https://files.pythonhosted.org/packages/41/c3/534eac40372d8ee36ef40df62ec129bee4fdb5ad9706e58a29be53b2c970/aiofiles-25.1.0.tar.gz", hash = "sha256:a8d728f0a29de45dc521f18f07297428d56992a742f0cd2701ba86e44d23d5b2", size = 46354, upload-time = "2025-10-09T20:51:04.358Z" }
|
||||
wheels = [
|
||||
{ url = "https://files.pythonhosted.org/packages/bc/8a/340a1555ae33d7354dbca4faa54948d76d89a27ceef032c8c3bc661d003e/aiofiles-25.1.0-py3-none-any.whl", hash = "sha256:abe311e527c862958650f9438e859c1fa7568a141b22abcd015e120e86a85695", size = 14668, upload-time = "2025-10-09T20:51:03.174Z" },
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "alembic"
|
||||
version = "1.18.4"
|
||||
@@ -3113,7 +3104,7 @@ name = "pexpect"
|
||||
version = "4.9.0"
|
||||
source = { registry = "https://pypi.org/simple" }
|
||||
dependencies = [
|
||||
{ name = "ptyprocess" },
|
||||
{ name = "ptyprocess", marker = "sys_platform != 'win32'" },
|
||||
]
|
||||
sdist = { url = "https://files.pythonhosted.org/packages/42/92/cc564bf6381ff43ce1f4d06852fc19a2f11d180f23dc32d9588bee2f149d/pexpect-4.9.0.tar.gz", hash = "sha256:ee7d41123f3c9911050ea2c2dac107568dc43b2d3b0c7557a33212c398ead30f", size = 166450, upload-time = "2023-11-25T09:07:26.339Z" }
|
||||
wheels = [
|
||||
@@ -4249,7 +4240,6 @@ name = "roboco"
|
||||
version = "0.1.0"
|
||||
source = { editable = "." }
|
||||
dependencies = [
|
||||
{ name = "aiofiles" },
|
||||
{ name = "alembic" },
|
||||
{ name = "anthropic" },
|
||||
{ name = "asyncpg" },
|
||||
@@ -4314,12 +4304,10 @@ dev = [
|
||||
{ name = "pytest" },
|
||||
{ name = "pytest-asyncio" },
|
||||
{ name = "pytest-cov" },
|
||||
{ name = "types-aiofiles" },
|
||||
]
|
||||
|
||||
[package.metadata]
|
||||
requires-dist = [
|
||||
{ name = "aiofiles" },
|
||||
{ name = "alembic" },
|
||||
{ name = "anthropic" },
|
||||
{ name = "asyncpg" },
|
||||
@@ -4377,7 +4365,6 @@ dev = [
|
||||
{ name = "pytest", specifier = ">=9.0.3" },
|
||||
{ name = "pytest-asyncio", specifier = ">=1.3.0" },
|
||||
{ name = "pytest-cov", specifier = ">=7.1.0" },
|
||||
{ name = "types-aiofiles" },
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -5392,15 +5379,6 @@ wheels = [
|
||||
{ url = "https://files.pythonhosted.org/packages/3f/f9/2b3ff4e56e5fa7debfaf9eb135d0da96f3e9a1d5b27222223c7296336e5f/typer-0.25.1-py3-none-any.whl", hash = "sha256:75caa44ed46a03fb2dab8808753ffacdbfea88495e74c85a28c5eefcf5f39c89", size = 58409, upload-time = "2026-04-30T19:32:18.271Z" },
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "types-aiofiles"
|
||||
version = "25.1.0.20260409"
|
||||
source = { registry = "https://pypi.org/simple" }
|
||||
sdist = { url = "https://files.pythonhosted.org/packages/6c/66/9e62a2692792bc96c0f423f478149f4a7b84720704c546c8960b0a047c89/types_aiofiles-25.1.0.20260409.tar.gz", hash = "sha256:49e67d72bdcf9fe406f5815758a78dc34a1249bb5aa2adba78a80aec0a775435", size = 14812, upload-time = "2026-04-09T04:22:35.308Z" }
|
||||
wheels = [
|
||||
{ url = "https://files.pythonhosted.org/packages/27/d0/28236f869ba4dfb223ecdbc267eb2bdb634b81a561dd992230a4f9ec48fa/types_aiofiles-25.1.0.20260409-py3-none-any.whl", hash = "sha256:923fedb532c772cc0f62e0ce4282725afa82ca5b41cabd9857f06b55e5eee8de", size = 14372, upload-time = "2026-04-09T04:22:34.328Z" },
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "types-cffi"
|
||||
version = "2.0.0.20260429"
|
||||
|
||||
Reference in New Issue
Block a user